| Message ID | 20220411133837.318876-8-troglobit@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: bridge: forwarding of unknown IPv4/IPv6/MAC BUM traffic | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On Mon, Apr 11, 2022 at 03:38:31PM +0200, Joachim Wiberg wrote: > Test per-port flood control flags of unknown BUM traffic by injecting > bc/uc/mc on one bridge port and verifying it being forwarded to both > the bridge itself and another regular bridge port. > > Signed-off-by: Joachim Wiberg <troglobit@gmail.com> > --- > .../testing/selftests/net/forwarding/Makefile | 3 +- > .../selftests/net/forwarding/bridge_flood.sh | 170 ++++++++++++++++++ > 2 files changed, 172 insertions(+), 1 deletion(-) > create mode 100755 tools/testing/selftests/net/forwarding/bridge_flood.sh > > diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile > index ae80c2aef577..873fa61d1ee1 100644 > --- a/tools/testing/selftests/net/forwarding/Makefile > +++ b/tools/testing/selftests/net/forwarding/Makefile > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0+ OR MIT > > -TEST_PROGS = bridge_igmp.sh \ > +TEST_PROGS = bridge_flood.sh \ > + bridge_igmp.sh \ > bridge_locked_port.sh \ > bridge_mdb.sh \ > bridge_port_isolation.sh \ > diff --git a/tools/testing/selftests/net/forwarding/bridge_flood.sh b/tools/testing/selftests/net/forwarding/bridge_flood.sh > new file mode 100755 > index 000000000000..1966c960d705 > --- /dev/null > +++ b/tools/testing/selftests/net/forwarding/bridge_flood.sh > @@ -0,0 +1,170 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Verify per-port flood control flags of unknown BUM traffic. > +# > +# br0 > +# / \ > +# h1 h2 I think the picture is slightly inaccurate. From it I understand that h1 and h2 are bridge ports, but they are stations attached to the real bridge ports, swp1 and swp2. Maybe it would be good to draw all interfaces. > +# > +# We inject bc/uc/mc on h1, toggle the three flood flags for > +# both br0 and h2, then verify that traffic is flooded as per > +# the flags, and nowhere else. > +# > +#set -x stray debug line > + > +ALL_TESTS="br_flood_unknown_bc_test br_flood_unknown_uc_test br_flood_unknown_mc_test" > +NUM_NETIFS=4 > + > +SRC_MAC="00:de:ad:be:ef:00" > +GRP_IP4="225.1.2.3" > +GRP_MAC="01:00:01:c0:ff:ee" > +GRP_IP6="ff02::42" > + > +BC_PKT="ff:ff:ff:ff:ff:ff $SRC_MAC 00:04 48:45:4c:4f" HELO to you too > +UC_PKT="02:00:01:c0:ff:ee $SRC_MAC 00:04 48:45:4c:4f" > +MC_PKT="01:00:5e:01:02:03 $SRC_MAC 08:00 45:00 00:20 c2:10 00:00 ff 11 12:b2 01:02:03:04 e1:01:02:03 04:d2 10:e1 00:0c 6e:84 48:45:4c:4f" > + > +# Disable promisc to ensure we only receive flooded frames > +export TCPDUMP_EXTRA_FLAGS="-pl" Exporting should be required only for sub-shells, doesn't apply when you source a script. > + > +source lib.sh > + > +h1=${NETIFS[p1]} > +h2=${NETIFS[p3]} > +swp1=${NETIFS[p2]} > +swp2=${NETIFS[p4]} > + > +# > +# Port mappings and flood flag pattern to set/detect > +# > +declare -A ports=([br0]=br0 [$swp1]=$h1 [$swp2]=$h2) Maybe you could populate the "ports" and the "flagN" arrays in the same order, i.e. bridge first for all? Also, to be honest, a generic name like "ports" is hard to digest, especially since you have another generic variable name "iface". Maybe "brports" and "station" is a little bit more specific? > +declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off) > +declare -A flag2=([$swp1]=off [$swp2]=on [br0]=off) > +declare -A flag3=([$swp1]=off [$swp2]=on [br0]=on ) > +declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on ) If it's not too much, maybe these could be called "flags_pass1", etc. Again, it was a bit hard to digest on first read. > + > +switch_create() > +{ > + ip link add dev br0 type bridge > + > + for port in ${!ports[@]}; do > + [ "$port" != "br0" ] && ip link set dev $port master br0 > + ip link set dev $port up > + done > +} > + > +switch_destroy() > +{ > + for port in ${!ports[@]}; do > + ip link set dev $port down > + done > + ip link del dev br0 > +} > + > +setup_prepare() > +{ > + vrf_prepare > + > + let i=1 > + for iface in ${ports[@]}; do > + [ "$iface" = "br0" ] && continue > + simple_if_init $iface 192.0.2.$i/24 2001:db8:1::$i/64 > + let i=$((i + 1)) > + done > + > + switch_create > +} > + > +cleanup() > +{ > + pre_cleanup > + switch_destroy > + > + let i=1 > + for iface in ${ports[@]}; do > + [ "$iface" = "br0" ] && continue > + simple_if_fini $iface 192.0.2.$i/24 2001:db8:1::$i/64 > + let i=$((i + 1)) > + done > + > + vrf_cleanup > +} > + > +do_flood_unknown() > +{ > + local type=$1 > + local pass=$2 > + local flag=$3 > + local pkt=$4 > + local -n flags=$5 I find it slightly less confusing if "flag" and "flags" are next to each other in the parameter list, since they're related. > + > + RET=0 > + for port in ${!ports[@]}; do > + if [ "$port" = "br0" ]; then > + self="self" > + else > + self="" > + fi > + bridge link set dev $port $flag ${flags[$port]} $self > + check_err $? "Failed setting $port $flag ${flags[$port]}" > + done > + > + for iface in ${ports[@]}; do > + tcpdump_start $iface > + done > + > + $MZ -q $h1 "$pkt" > + sleep 1 > + > + for iface in ${ports[@]}; do > + tcpdump_stop $iface > + done > + > + for port in ${!ports[@]}; do > + iface=${ports[$port]} > + > +# echo "Dumping PCAP from $iface, expecting ${flags[$port]}:" > +# tcpdump_show $iface Do something about the commented lines. > + tcpdump_show $iface |grep -q "$SRC_MAC" Space between pipe and grep. > + rc=$? > + > + check_err_fail "${flags[$port]} = on" $? "failed flooding from $h1 to port $port" I think the "failed" word here is superfluous, since check_err_fail already says "$what succeeded, but should have failed". > + check_err_fail "${flags[$port]} = off" $? "flooding from $h1 to port $port" > + done > + > + log_test "flood unknown $type pass $pass/4" > +} > + > +br_flood_unknown_bc_test() > +{ > + do_flood_unknown BC 1 bcast_flood "$BC_PKT" flag1 > + do_flood_unknown BC 2 bcast_flood "$BC_PKT" flag2 > + do_flood_unknown BC 3 bcast_flood "$BC_PKT" flag3 > + do_flood_unknown BC 4 bcast_flood "$BC_PKT" flag4 > +} > + > +br_flood_unknown_uc_test() > +{ > + do_flood_unknown UC 1 flood "$UC_PKT" flag1 > + do_flood_unknown UC 2 flood "$UC_PKT" flag2 > + do_flood_unknown UC 3 flood "$UC_PKT" flag3 > + do_flood_unknown UC 4 flood "$UC_PKT" flag4 > +} > + > +br_flood_unknown_mc_test() > +{ > + do_flood_unknown MC 1 mcast_flood "$MC_PKT" flag1 > + do_flood_unknown MC 2 mcast_flood "$MC_PKT" flag2 > + do_flood_unknown MC 3 mcast_flood "$MC_PKT" flag3 > + do_flood_unknown MC 4 mcast_flood "$MC_PKT" flag4 > +} > + > +trap cleanup EXIT > + > +setup_prepare > +setup_wait > + > +tests_run > + > +exit $EXIT_STATUS > -- > 2.25.1 >
On Mon, Apr 11, 2022 at 20:21, Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > On Mon, Apr 11, 2022 at 03:38:31PM +0200, Joachim Wiberg wrote: >> +# Verify per-port flood control flags of unknown BUM traffic. >> +# >> +# br0 >> +# / \ >> +# h1 h2 > > I think the picture is slightly inaccurate. From it I understand that h1 > and h2 are bridge ports, but they are stations attached to the real > bridge ports, swp1 and swp2. Maybe it would be good to draw all interfaces. Hmm, yeah either that or drop it entirely. I sort of assumed everyone knew about the h<-[veth]->swp (or actual cable) setup, but you're right this is a bit unclear. Me and Tobias have internally used h<-->p (for host<-->bridge-port) and other similar nomenclature. Finding a good name that fits easily, and is still readable, in ASCII drawings is hard. I'll give it a go in the next drop, thanks! >> +#set -x > stray debug line thx >> +# Disable promisc to ensure we only receive flooded frames >> +export TCPDUMP_EXTRA_FLAGS="-pl" > Exporting should be required only for sub-shells, doesn't apply when you > source a script. Ah thanks, will fix! >> +# Port mappings and flood flag pattern to set/detect >> +declare -A ports=([br0]=br0 [$swp1]=$h1 [$swp2]=$h2) > Maybe you could populate the "ports" and the "flagN" arrays in the same > order, i.e. bridge first for all? Good point, thanks! > Also, to be honest, a generic name like "ports" is hard to digest, > especially since you have another generic variable name "iface". > Maybe "brports" and "station" is a little bit more specific? Is there a common naming standard between bridge tests, or is it more important to be consistent the test overview (test heading w/ picture)? Anyway, I'll have a look at the naming for the next drop. >> +declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off) >> +declare -A flag2=([$swp1]=off [$swp2]=on [br0]=off) >> +declare -A flag3=([$swp1]=off [$swp2]=on [br0]=on ) >> +declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on ) > If it's not too much, maybe these could be called "flags_pass1", etc. > Again, it was a bit hard to digest on first read. More like flags_pass_fail, but since its the flooding flags, maybe flood_patternN would be better? >> +do_flood_unknown() >> +{ >> + local type=$1 >> + local pass=$2 >> + local flag=$3 >> + local pkt=$4 >> + local -n flags=$5 > I find it slightly less confusing if "flag" and "flags" are next to each > other in the parameter list, since they're related. Hmm, OK. >> +# echo "Dumping PCAP from $iface, expecting ${flags[$port]}:" >> +# tcpdump_show $iface > Do something about the commented lines. Oups, thanks! >> + tcpdump_show $iface |grep -q "$SRC_MAC" > Space between pipe and grep. Will fix! >> + check_err_fail "${flags[$port]} = on" $? "failed flooding from $h1 to port $port" > I think the "failed" word here is superfluous, since check_err_fail > already says "$what succeeded, but should have failed". Ah, good point! Thank you for the review! <3 /J
On Tue, Apr 12, 2022 at 09:55:31AM +0200, Joachim Wiberg wrote: > On Mon, Apr 11, 2022 at 20:21, Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Mon, Apr 11, 2022 at 03:38:31PM +0200, Joachim Wiberg wrote: > >> +# Verify per-port flood control flags of unknown BUM traffic. > >> +# > >> +# br0 > >> +# / \ > >> +# h1 h2 > > > > I think the picture is slightly inaccurate. From it I understand that h1 > > and h2 are bridge ports, but they are stations attached to the real > > bridge ports, swp1 and swp2. Maybe it would be good to draw all interfaces. > > Hmm, yeah either that or drop it entirely. I sort of assumed everyone > knew about the h<-[veth]->swp (or actual cable) setup, but you're right > this is a bit unclear. Me and Tobias have internally used h<-->p (for > host<-->bridge-port) and other similar nomenclature. Finding a good > name that fits easily, and is still readable, in ASCII drawings is hard. > I'll give it a go in the next drop, thanks! I wasn't thinking of anything too fancy, this would do I guess. br0 / \ h1 --- swp1 swp2 --- h2 > > Also, to be honest, a generic name like "ports" is hard to digest, > > especially since you have another generic variable name "iface". > > Maybe "brports" and "station" is a little bit more specific? > > Is there a common naming standard between bridge tests, or is it more > important to be consistent the test overview (test heading w/ picture)? > > Anyway, I'll have a look at the naming for the next drop. Even if there is a common naming standard in the selftests I wouldn't know it. I just found the naming here to be vague enough that it is confusing. If there are other examples of "port" + "iface" please feel free to disregard. > >> +declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off) > >> +declare -A flag2=([$swp1]=off [$swp2]=on [br0]=off) > >> +declare -A flag3=([$swp1]=off [$swp2]=on [br0]=on ) > >> +declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on ) > > If it's not too much, maybe these could be called "flags_pass1", etc. > > Again, it was a bit hard to digest on first read. > > More like flags_pass_fail, but since its the flooding flags, maybe > flood_patternN would be better? This works.
diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile index ae80c2aef577..873fa61d1ee1 100644 --- a/tools/testing/selftests/net/forwarding/Makefile +++ b/tools/testing/selftests/net/forwarding/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0+ OR MIT -TEST_PROGS = bridge_igmp.sh \ +TEST_PROGS = bridge_flood.sh \ + bridge_igmp.sh \ bridge_locked_port.sh \ bridge_mdb.sh \ bridge_port_isolation.sh \ diff --git a/tools/testing/selftests/net/forwarding/bridge_flood.sh b/tools/testing/selftests/net/forwarding/bridge_flood.sh new file mode 100755 index 000000000000..1966c960d705 --- /dev/null +++ b/tools/testing/selftests/net/forwarding/bridge_flood.sh @@ -0,0 +1,170 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# Verify per-port flood control flags of unknown BUM traffic. +# +# br0 +# / \ +# h1 h2 +# +# We inject bc/uc/mc on h1, toggle the three flood flags for +# both br0 and h2, then verify that traffic is flooded as per +# the flags, and nowhere else. +# +#set -x + +ALL_TESTS="br_flood_unknown_bc_test br_flood_unknown_uc_test br_flood_unknown_mc_test" +NUM_NETIFS=4 + +SRC_MAC="00:de:ad:be:ef:00" +GRP_IP4="225.1.2.3" +GRP_MAC="01:00:01:c0:ff:ee" +GRP_IP6="ff02::42" + +BC_PKT="ff:ff:ff:ff:ff:ff $SRC_MAC 00:04 48:45:4c:4f" +UC_PKT="02:00:01:c0:ff:ee $SRC_MAC 00:04 48:45:4c:4f" +MC_PKT="01:00:5e:01:02:03 $SRC_MAC 08:00 45:00 00:20 c2:10 00:00 ff 11 12:b2 01:02:03:04 e1:01:02:03 04:d2 10:e1 00:0c 6e:84 48:45:4c:4f" + +# Disable promisc to ensure we only receive flooded frames +export TCPDUMP_EXTRA_FLAGS="-pl" + +source lib.sh + +h1=${NETIFS[p1]} +h2=${NETIFS[p3]} +swp1=${NETIFS[p2]} +swp2=${NETIFS[p4]} + +# +# Port mappings and flood flag pattern to set/detect +# +declare -A ports=([br0]=br0 [$swp1]=$h1 [$swp2]=$h2) +declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off) +declare -A flag2=([$swp1]=off [$swp2]=on [br0]=off) +declare -A flag3=([$swp1]=off [$swp2]=on [br0]=on ) +declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on ) + +switch_create() +{ + ip link add dev br0 type bridge + + for port in ${!ports[@]}; do + [ "$port" != "br0" ] && ip link set dev $port master br0 + ip link set dev $port up + done +} + +switch_destroy() +{ + for port in ${!ports[@]}; do + ip link set dev $port down + done + ip link del dev br0 +} + +setup_prepare() +{ + vrf_prepare + + let i=1 + for iface in ${ports[@]}; do + [ "$iface" = "br0" ] && continue + simple_if_init $iface 192.0.2.$i/24 2001:db8:1::$i/64 + let i=$((i + 1)) + done + + switch_create +} + +cleanup() +{ + pre_cleanup + switch_destroy + + let i=1 + for iface in ${ports[@]}; do + [ "$iface" = "br0" ] && continue + simple_if_fini $iface 192.0.2.$i/24 2001:db8:1::$i/64 + let i=$((i + 1)) + done + + vrf_cleanup +} + +do_flood_unknown() +{ + local type=$1 + local pass=$2 + local flag=$3 + local pkt=$4 + local -n flags=$5 + + RET=0 + for port in ${!ports[@]}; do + if [ "$port" = "br0" ]; then + self="self" + else + self="" + fi + bridge link set dev $port $flag ${flags[$port]} $self + check_err $? "Failed setting $port $flag ${flags[$port]}" + done + + for iface in ${ports[@]}; do + tcpdump_start $iface + done + + $MZ -q $h1 "$pkt" + sleep 1 + + for iface in ${ports[@]}; do + tcpdump_stop $iface + done + + for port in ${!ports[@]}; do + iface=${ports[$port]} + +# echo "Dumping PCAP from $iface, expecting ${flags[$port]}:" +# tcpdump_show $iface + tcpdump_show $iface |grep -q "$SRC_MAC" + rc=$? + + check_err_fail "${flags[$port]} = on" $? "failed flooding from $h1 to port $port" + check_err_fail "${flags[$port]} = off" $? "flooding from $h1 to port $port" + done + + log_test "flood unknown $type pass $pass/4" +} + +br_flood_unknown_bc_test() +{ + do_flood_unknown BC 1 bcast_flood "$BC_PKT" flag1 + do_flood_unknown BC 2 bcast_flood "$BC_PKT" flag2 + do_flood_unknown BC 3 bcast_flood "$BC_PKT" flag3 + do_flood_unknown BC 4 bcast_flood "$BC_PKT" flag4 +} + +br_flood_unknown_uc_test() +{ + do_flood_unknown UC 1 flood "$UC_PKT" flag1 + do_flood_unknown UC 2 flood "$UC_PKT" flag2 + do_flood_unknown UC 3 flood "$UC_PKT" flag3 + do_flood_unknown UC 4 flood "$UC_PKT" flag4 +} + +br_flood_unknown_mc_test() +{ + do_flood_unknown MC 1 mcast_flood "$MC_PKT" flag1 + do_flood_unknown MC 2 mcast_flood "$MC_PKT" flag2 + do_flood_unknown MC 3 mcast_flood "$MC_PKT" flag3 + do_flood_unknown MC 4 mcast_flood "$MC_PKT" flag4 +} + +trap cleanup EXIT + +setup_prepare +setup_wait + +tests_run + +exit $EXIT_STATUS
Test per-port flood control flags of unknown BUM traffic by injecting bc/uc/mc on one bridge port and verifying it being forwarded to both the bridge itself and another regular bridge port. Signed-off-by: Joachim Wiberg <troglobit@gmail.com> --- .../testing/selftests/net/forwarding/Makefile | 3 +- .../selftests/net/forwarding/bridge_flood.sh | 170 ++++++++++++++++++ 2 files changed, 172 insertions(+), 1 deletion(-) create mode 100755 tools/testing/selftests/net/forwarding/bridge_flood.sh