Discussion:
kern/48104: Incorrect forwarding of broadcast packets by bridge(4)
(too old to reply)
Lloyd Parkes
2014-02-21 00:25:58 UTC
Permalink
Hi all,
I finally got around to running the last test on my changes to bridge(4) and it all works well.

The only real problem I had to think about was that the point in the network stack for tapping off bridged packets was above the point in the network stack where packets are injected. This means that when multicasting packets up the network stack you have to do something to prevent a packet storm. My initial implementation used a link local mbuf flag, but bridging is inherently not link local, so I discarded that method.

I settled on moving the point at which the bridge taps into the network stack down the stack so that it is now below the point at which packets are injected into the stack. In particular, I removed the bridge code from ether_input() and made the bridge update the struct ifnet if_input field when an interface is added to or removed from a bridge. The code for updating the if_input field is purposefully simple and conservative.

There are some interface drivers that call ether_input() directly instead of indirecting through struct ifnet if_input and that seems to be a requirement that was missed when porting the drivers from FreeBSD. I’ll PR those drivers once I finish this email and send this patch to kern/48104.

Overall this patch removes 107 of old code and adds 85 lines of new code. I know it’s a crude code metric, but I do like increased functionality with less code.

Cheers,
Lloyd
Ryota Ozaki
2014-02-24 10:39:30 UTC
Permalink
Post by Lloyd Parkes
Hi all,
I finally got around to running the last test on my changes to bridge(4) and it all works well.
The only real problem I had to think about was that the point in the network stack for tapping off bridged packets was above the point in the network stack where packets are injected. This means that when multicasting packets up the network stack you have to do something to prevent a packet storm. My initial implementation used a link local mbuf flag, but bridging is inherently not link local, so I discarded that method.
I settled on moving the point at which the bridge taps into the network stack down the stack so that it is now below the point at which packets are injected into the stack. In particular, I removed the bridge code from ether_input() and made the bridge update the struct ifnet if_input field when an interface is added to or removed from a bridge. The code for updating the if_input field is purposefully simple and conservative.
I prefer this reconstruction of ether_input and bridge_input.
Actually I (and my coworker) are thinking the approach to make
the part simple.

I'm checking and testing your patch.
Post by Lloyd Parkes
There are some interface drivers that call ether_input() directly instead of indirecting through struct ifnet if_input and that seems to be a requirement that was missed when porting the drivers from FreeBSD. I’ll PR those drivers once I finish this email and send this patch to kern/48104.
Overall this patch removes 107 of old code and adds 85 lines of new code. I know it’s a crude code metric, but I do like increased functionality with less code.
me too :)

Regards,
ozaki-r
Post by Lloyd Parkes
Cheers,
Lloyd
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Ryota Ozaki
2014-02-26 10:03:22 UTC
Permalink
Hi Lloyd,
Post by Ryota Ozaki
Post by Lloyd Parkes
Hi all,
I finally got around to running the last test on my changes to bridge(4) and it all works well.
The only real problem I had to think about was that the point in the network stack for tapping off bridged packets was above the point in the network stack where packets are injected. This means that when multicasting packets up the network stack you have to do something to prevent a packet storm. My initial implementation used a link local mbuf flag, but bridging is inherently not link local, so I discarded that method.
I settled on moving the point at which the bridge taps into the network stack down the stack so that it is now below the point at which packets are injected into the stack. In particular, I removed the bridge code from ether_input() and made the bridge update the struct ifnet if_input field when an interface is added to or removed from a bridge. The code for updating the if_input field is purposefully simple and conservative.
I prefer this reconstruction of ether_input and bridge_input.
Actually I (and my coworker) are thinking the approach to make
the part simple.
I'm checking and testing your patch.
I've got a problem that iperf -s on a tap with a bridge doesn't work
with your patch. It works w/o your patch. The instructions are
as follows:


# ifconfig bridge0 create
# ifconfig bridge0 up
# ifconfig tap0 create
# ifconfig tap0 192.168.122.67/24 up
# brconfig bridge0 add tap0
# brconfig bridge0 add vioif0
# iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 32.0 KByte (default)
------------------------------------------------------------
[ 4] local 192.168.122.67 port 5001 connected with 192.168.122.1 port 56964
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55


Do you have any idea on the error?

Regards,
ozaki-r
Post by Ryota Ozaki
Post by Lloyd Parkes
There are some interface drivers that call ether_input() directly instead of indirecting through struct ifnet if_input and that seems to be a requirement that was missed when porting the drivers from FreeBSD. I'll PR those drivers once I finish this email and send this patch to kern/48104.
Overall this patch removes 107 of old code and adds 85 lines of new code. I know it's a crude code metric, but I do like increased functionality with less code.
me too :)
Regards,
ozaki-r
Post by Lloyd Parkes
Cheers,
Lloyd
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Lloyd Parkes
2014-02-27 22:38:15 UTC
Permalink
Post by Ryota Ozaki
I've got a problem that iperf -s on a tap with a bridge doesn't work
with your patch. It works w/o your patch. The instructions are
# ifconfig bridge0 create
# ifconfig bridge0 up
# ifconfig tap0 create
# ifconfig tap0 192.168.122.67/24 up
# brconfig bridge0 add tap0
# brconfig bridge0 add vioif0
# iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 32.0 KByte (default)
------------------------------------------------------------
[ 4] local 192.168.122.67 port 5001 connected with 192.168.122.1 port 56964
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
Do you have any idea on the error?
Thanks for testing the code. I have no idea what might be causing this problem because the unicast path isn’t changed much it shouldn’t allocating mbufs any differently from before. I’ll look in to right now.

Cheers,
Lloyd
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Ryota Ozaki
2014-03-23 11:10:33 UTC
Permalink
On Fri, Feb 28, 2014 at 7:38 AM, Lloyd Parkes
Post by Ryota Ozaki
I've got a problem that iperf -s on a tap with a bridge doesn't work
with your patch. It works w/o your patch. The instructions are
# ifconfig bridge0 create
# ifconfig bridge0 up
# ifconfig tap0 create
# ifconfig tap0 192.168.122.67/24 up
# brconfig bridge0 add tap0
# brconfig bridge0 add vioif0
# iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 32.0 KByte (default)
------------------------------------------------------------
[ 4] local 192.168.122.67 port 5001 connected with 192.168.122.1 port 56964
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
vioif0: rx mbuf allocation failed, error code 55
Do you have any idea on the error?
Thanks for testing the code. I have no idea what might be causing this problem because the unicast path isn't changed much it shouldn't allocating mbufs any differently from before. I'll look in to right now.
I'm sorry for late replying.

Finally I had time to test this patch again and found some bugs.

http://www.netbsd.org/~ozaki-r/bridge-fix.patch

This is a patch fixing the above problem and others:
- check bif_state in bridge_input only if STP enabled
- don't overwrite bif in bridge_input that would be used afterward
- fix used in pridge_broadcast to be accidentally reset

And also your patch breaks the NetBSD coding style (*), so please follow it.

(*) http://cvsweb.netbsd.org/bsdweb.cgi/src/share/misc/style?rev=HEAD&content-type=text/x-cvsweb-markup

Then, the patch will be ok for me, although I want some other
networking guys to check the patch more.

Regards,
ozaki-r
Cheers,
Lloyd
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Loading...