On Wed, Mar 10, 2021 at 00:01, Vladimir Oltean <olte...@gmail.com> wrote:
> On Tue, Mar 09, 2021 at 10:28:11PM +0100, Tobias Waldekranz wrote:
>> On Tue, Mar 09, 2021 at 12:40, Florian Fainelli <f.faine...@gmail.com> wrote:
>> > On 3/9/21 10:42 AM, Tobias Waldekranz wrote:
>> >> There are three kinds of events that have an inpact on VLAN
>> >> configuration of DSA ports:
>> >>
>> >> - Adding of stacked VLANs
>> >> (ip link add dev swp0.1 link swp0 type vlan id 1)
>> >>
>> >> - Adding of bridged VLANs
>> >> (bridge vlan add dev swp0 vid 1)
>> >>
>> >> - Changes to a bridge's VLAN filtering setting
>> >> (ip link set dev br0 type bridge vlan_filtering 1)
>> >>
>> >> For all of these events, we want to ensure that some invariants are
>> >> upheld:
>> >>
>> >> - For hardware where VLAN filtering is a global setting, either all
>> >> bridges must use VLAN filtering, or no bridge can.
>> >
>> > I suppose that is true, given that a non-VLAN filtering bridge must not
>> > perform ingress VID checking, OK.
>> >
>> >>
>> >> - For all filtering bridges, no stacked VLAN on any port may be
>> >> configured on multiple ports.
>> >
>> > You need to qualify multiple ports a bit more here, are you saying
>> > multiple ports that are part of said bridge, or?
>>
>> Yeah sorry, I can imagine that makes no sense whatsoever without the
>> context of the recent discussions. It is basically guarding against this
>> situation:
>>
>> .100 br0 .100
>> \ / \ /
>> lan0 lan1
>>
>> $ ip link add dev br0 type bridge vlan_filtering 1
>> $ ip link add dev lan0.100 link lan0 type vlan id 100
>> $ ip link add dev lan1.100 link lan1 type vlan id 100
>> $ ip link set dev lan0 master br0
>> $ ip link set dev lan1 master br0 # This should fail
>>
>> >> - For all filtering bridges, no stacked VLAN may be configured in the
>> >> bridge.
>> >
>> > Being stacked in the bridge does not really compute for me, you mean, no
>> > VLAN upper must be configured on the bridge master device(s)? Why would
>> > that be a problem though?
>>
>> Again sorry, I relize that this message needs a lot of work. It guards
>> against this scenario:
>>
>> .100 br0
>> \ / \
>> lan0 lan1
>>
>> $ ip link add dev br0 type bridge vlan_filtering 1
>> $ ip link add dev lan0.100 link lan0 type vlan id 100
>> $ ip link set dev lan0 master br0
>> $ ip link set dev lan1 master br0
>> $ bridge vlan add dev lan1 vid 100 # This should fail
>>
>> >> Move the validation of these invariants to a central function, and use
>> >> it from all sites where these events are handled. This way, we ensure
>> >> that all invariants are always checked, avoiding certain configs being
>> >> allowed or disallowed depending on the order in which commands are
>> >> given.
>> >>
>> >> Signed-off-by: Tobias Waldekranz <tob...@waldekranz.com>
>> >> ---
>> >>
>> >> There is still testing left to do on this, but I wanted to send early
>> >> in order show what I meant by "generic" VLAN validation in this
>> >> discussion:
>> >>
>> >> https://lore.kernel.org/netdev/87mtvdp97q....@waldekranz.com/
>> >>
>> >> This is basically an alternative implementation of 1/4 and 2/4 from
>> >> this series by Vladimir:
>> >>
>> >> https://lore.kernel.org/netdev/20210309021657.3639745-1-olte...@gmail.com/
>> >
>> > I really have not been able to keep up with your discussion, and I am
>> > not sure if I will given how quickly you guys can spin patches (not a
>> > criticism, this is welcome).
>>
>> Yeah I know, it has been a bit of a whirlwind.
>>
>> Maybe I should just have posted this inline in the other thread, since
>> it was mostly to show Vladimir my idea, and it seemed easier to write it
>> in C than in English :)
>
> I like it, I think it has good potential.
> I wrote up this battery of tests, there is still one condition which you
> are not catching, but you should be able to add it. If you find more
Thanks for taking the time to write all these!
> corner cases please feel free to add them to this list. Then you can
> clean up the patch and send it, I think.
>
> -----------------------------[ cut here ]-----------------------------
> From 9fcfccb6a38a9769962b098ba19d50e576710b5b Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.olt...@nxp.com>
> Date: Tue, 9 Mar 2021 23:51:01 +0200
> Subject: [PATCH] selftests: net: dsa: add checks for all VLAN configurations
> known to mankind that should fail
>
> Offloading VLANs from two different directions is no easy feat,
> especially since we can toggle the VLAN filtering property at runtime,
> and even per port!
>
> Try to capture the combinations of commands that should be rejected by
> DSA, in the attempt of creating a validation procedure that catches them
> all.
>
> Note that this patch moves the irritating "require_command" for mausezahn
> outside the main net forwarding lib logic, into the functions that
> actually make use of it. My testing system doesn't have mausezahn, and
> this test doesn't even require it.
>
> Signed-off-by: Vladimir Oltean <vladimir.olt...@nxp.com>
> ---
> .../drivers/net/dsa/vlan_validation.sh | 316 ++++++++++++++++++
> tools/testing/selftests/net/forwarding/lib.sh | 9 +-
> 2 files changed, 324 insertions(+), 1 deletion(-)
> create mode 100755 tools/testing/selftests/drivers/net/dsa/vlan_validation.sh
>
> diff --git a/tools/testing/selftests/drivers/net/dsa/vlan_validation.sh
> b/tools/testing/selftests/drivers/net/dsa/vlan_validation.sh
> new file mode 100755
> index 000000000000..445ce17cb925
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/dsa/vlan_validation.sh
> @@ -0,0 +1,316 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +NUM_NETIFS=2
> +lib_dir=$(dirname $0)/../../../net/forwarding
> +source $lib_dir/lib.sh
> +
> +eth0=${NETIFS[p1]}
> +eth1=${NETIFS[p2]}
> +
> +test_bridge_vlan_when_port_has_that_vlan_as_upper()
> +{
> + ip link add br0 type bridge vlan_filtering 1
> + ip link set ${eth0} master br0
> + bridge vlan add dev ${eth0} vid 100 master
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.100 > /dev/null 2>&1 || :
> + ip link del br0
> +
> + log_test "Add bridge VLAN when port has that VLAN as upper already"
> +}
> +
> +test_bridge_vlan_when_port_has_that_vlan_as_upper_but_is_initially_unaware()
> +{
> + ip link add br0 type bridge vlan_filtering 0
> + ip link set ${eth0} master br0
> + bridge vlan add dev ${eth0} vid 100 master
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link set br0 type bridge vlan_filtering 1
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.100
> + ip link del br0
> +
> + log_test "Add bridge VLAN when port has that VLAN as upper already, but
> bridge is initially VLAN-unaware"
> +}
> +
> +test_bridge_vlan_when_other_bridge_port_has_that_vlan_as_upper()
> +{
> + ip link add br0 type bridge vlan_filtering 1
> + ip link set ${eth0} master br0
> + ip link set ${eth1} master br0
> + bridge vlan add dev ${eth0} vid 100 master
> + ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth1}.100 > /dev/null 2>&1 || :
> + ip link del br0
> +
> + log_test "Add bridge VLAN when another bridge port has that VLAN as
> upper already"
> +}
> +
> +test_bridge_vlan_when_other_bridge_port_has_that_vlan_as_upper_but_is_initially_unaware()
> +{
> + ip link add br0 type bridge vlan_filtering 0
> + ip link set ${eth0} master br0
> + ip link set ${eth1} master br0
> + bridge vlan add dev ${eth0} vid 100 master
> + ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> + ip link set br0 type bridge vlan_filtering 1
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth1}.100
> + ip link del br0
> +
> + log_test "Add bridge VLAN when another bridge port has that VLAN as
> upper already, but bridge is initially VLAN-unaware"
> +}
> +
> +test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_vlan_upper()
> +{
> + ip link add br0 type bridge vlan_filtering 1
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> + ip link set ${eth0} master br0
> + ip link set ${eth1} master br0
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.100
> + ip link del ${eth1}.100
> + ip link del br0
> +
> + log_test "Bridge join when new port has VLAN upper with same VID as
> another port's VLAN upper"
> +}
> +
> +test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_vlan_upper_initially_unaware()
> +{
> + ip link add br0 type bridge vlan_filtering 0
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> + ip link set ${eth0} master br0
> + ip link set ${eth1} master br0
> + ip link set br0 type bridge vlan_filtering 1
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.100
> + ip link del ${eth1}.100
> + ip link del br0
> +
> + log_test "Bridge join when new port has VLAN upper with same VID as
> another port's VLAN upper, and bridge is initially unaware"
> +}
> +
> +test_bridge_join_when_new_port_has_vlan_upper_equal_to_pvid()
> +{
> + ip link add br0 type bridge vlan_filtering 1
> + ip link add link ${eth0} name ${eth0}.1 type vlan id 1
> + ip link set ${eth0} master br0
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.1
> + ip link del br0
> +
> + log_test "Bridge join when new port has VLAN upper equal to the PVID"
> +}
> +
> +test_bridge_join_when_new_port_has_vlan_upper_equal_to_pvid_but_initially_unaware()
> +{
> + ip link add br0 type bridge vlan_filtering 0
> + ip link add link ${eth0} name ${eth0}.1 type vlan id 1
> + ip link set ${eth0} master br0
> + ip link set br0 type bridge vlan_filtering 1
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.1
> + ip link del br0
> +
> + log_test "Bridge join when new port has VLAN upper equal to the PVID,
> but bridge is initially VLAN-unaware"
> +}
> +
> +test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_bridge_vlan()
> +{
> + ip link add br0 type bridge vlan_filtering 1
> + ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> + ip link set ${eth0} master br0
> + bridge vlan add dev ${eth0} vid 100 master
> + ip link set ${eth1} master br0
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth1}.100
> + ip link del br0
> +
> + log_test "Bridge join when new port has VLAN upper with same VID as
> another port's bridge VLAN"
> +}
> +
> +test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_bridge_vlan_initially_unaware()
> +{
> + ip link add br0 type bridge vlan_filtering 0
> + ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> + ip link set ${eth0} master br0
> + bridge vlan add dev ${eth0} vid 100 master
> + ip link set ${eth1} master br0
> + ip link set br0 type bridge vlan_filtering 1
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth1}.100
> + ip link del br0
> +
> + log_test "Bridge join when new port has VLAN upper with same VID as
> another port's bridge VLAN, but bridge is initially unaware"
> +}
> +
> +test_vlan_upper_on_bridge_port_when_another_port_has_upper_with_same_vid()
> +{
> + ip link add br0 type bridge vlan_filtering 1
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link set ${eth0} master br0
> + ip link set ${eth1} master br0
> + ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.100
> + ip link del ${eth1}.100
> + ip link del br0
> +
> + log_test "Add VLAN upper to port in bridge which has another port with
> same upper VLAN ID"
> +}
> +
> +test_vlan_upper_on_bridge_port_when_another_port_has_upper_with_same_vid_initially_unaware()
> +{
> + ip link add br0 type bridge vlan_filtering 0
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link set ${eth0} master br0
> + ip link set ${eth1} master br0
> + ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> + ip link set br0 type bridge vlan_filtering 1
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.100
> + ip link del ${eth1}.100
> + ip link del br0
> +
> + log_test "Add VLAN upper to port in bridge which has another port with
> same upper VLAN ID, and bridge is initially unaware"
> +}
> +
> +test_vlan_upper_join_vlan_aware_bridge_which_contains_the_physical_port()
> +{
> + ip link add br0 type bridge vlan_filtering 1
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link set ${eth0} master br0
> + ip link set ${eth0}.100 master br0
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.100 > /dev/null 2>&1 || :
> + ip link del br0
> +
> + log_test "VLAN upper joins VLAN-aware bridge"
> +}
> +
> +test_vlan_upper_join_vlan_aware_bridge_which_contains_the_physical_port_initially_unaware()
> +{
> + ip link add br0 type bridge vlan_filtering 0
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link set ${eth0} master br0
> + ip link set ${eth0}.100 master br0
> + ip link set br0 type bridge vlan_filtering 1
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.100 > /dev/null 2>&1 || :
> + ip link del br0
> +
> + log_test "VLAN upper joins VLAN-aware bridge, but bridge is initially
> unaware"
> +}
> +
> +test_bridge_join_when_vlan_upper_is_already_in_bridge()
> +{
> + ip link add br0 type bridge vlan_filtering 1
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link set ${eth0}.100 master br0
> + ip link set ${eth0} master br0
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.100 > /dev/null 2>&1 || :
> + ip link del br0
> +
> + log_test "Bridge join when VLAN upper is already in VLAN-aware bridge"
> +}
> +
> +test_bridge_join_when_vlan_upper_is_already_in_bridge_initially_unaware()
> +{
> + ip link add br0 type bridge vlan_filtering 0
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link set ${eth0}.100 master br0
> + ip link set ${eth0} master br0
> + ip link set br0 type bridge vlan_filtering 1
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.100 > /dev/null 2>&1 || :
> + ip link del br0
> +
> + log_test "Bridge join when VLAN upper is already in VLAN-aware bridge,
> which was initially VLAN-unaware"
> +}
> +
> +test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port()
> +{
> + ip link add br0 type bridge vlan_filtering 1
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link set ${eth1} master br0
> + ip link set ${eth0}.100 master br0
> + check_fail $? "Expected to fail but didn't"
Should it though?
br0
/ \
.100 \
| \
eth0 eth1
eth0 is in standalone mode here. So if the kernel allows it, who are we
to argue?
> + ip link del ${eth0}.100 > /dev/null 2>&1 || :
> + ip link del br0
> +
> + log_test "VLAN upper joins VLAN-aware bridge which contains another
> physical port"
> +}
> +
> +test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port_initially_unaware()
> +{
> + ip link add br0 type bridge vlan_filtering 0
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link set ${eth1} master br0
> + ip link set ${eth0}.100 master br0
> + ip link set br0 type bridge vlan_filtering 1
> + check_fail $? "Expected to fail but didn't"
Same thing here.
> + ip link del ${eth0}.100 > /dev/null 2>&1 || :
> + ip link del br0
> +
> + log_test "VLAN upper joins VLAN-aware bridge which contains another
> physical port, but bridge is initially unaware"
> +}
> +
> +test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge()
> +{
> + ip link add br0 type bridge vlan_filtering 1
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link set ${eth0}.100 master br0
> + ip link set ${eth1} master br0
> + check_fail $? "Expected to fail but didn't"
And here.
> + ip link del ${eth0}.100 > /dev/null 2>&1 || :
> + ip link del br0
> +
> + log_test "Bridge join when VLAN upper of another port is already in
> VLAN-aware bridge"
> +}
> +
> +test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge_initially_unaware()
> +{
> + ip link add br0 type bridge vlan_filtering 0
> + ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> + ip link set ${eth0}.100 master br0
> + ip link set ${eth0} master br0
I think you meant for this to be eth1, correct?
In that case, same comment as before applies to this as well.
> + ip link set br0 type bridge vlan_filtering 1
> + check_fail $? "Expected to fail but didn't"
> + ip link del ${eth0}.100 > /dev/null 2>&1 || :
> + ip link del br0
> +
> + log_test "Bridge join when VLAN upper of another port is already in
> VLAN-aware bridge, which was initially VLAN-unaware"
> +}
> +
> +ALL_TESTS="
> + test_bridge_vlan_when_port_has_that_vlan_as_upper
> +
> test_bridge_vlan_when_port_has_that_vlan_as_upper_but_is_initially_unaware
> + test_bridge_vlan_when_other_bridge_port_has_that_vlan_as_upper
> +
> test_bridge_vlan_when_other_bridge_port_has_that_vlan_as_upper_but_is_initially_unaware
> +
> test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_vlan_upper
> +
> test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_vlan_upper_initially_unaware
> + test_bridge_join_when_new_port_has_vlan_upper_equal_to_pvid
> +
> test_bridge_join_when_new_port_has_vlan_upper_equal_to_pvid_but_initially_unaware
> +
> test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_bridge_vlan
> +
> test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_bridge_vlan_initially_unaware
> + test_vlan_upper_on_bridge_port_when_another_port_has_upper_with_same_vid
> +
> test_vlan_upper_on_bridge_port_when_another_port_has_upper_with_same_vid_initially_unaware
> + test_vlan_upper_join_vlan_aware_bridge_which_contains_the_physical_port
> +
> test_vlan_upper_join_vlan_aware_bridge_which_contains_the_physical_port_initially_unaware
> + test_bridge_join_when_vlan_upper_is_already_in_bridge
> + test_bridge_join_when_vlan_upper_is_already_in_bridge_initially_unaware
> +
> test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port
> +
> test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port_initially_unaware
> + test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge
> +
> test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge_initially_unaware
> +"
> +
> +tests_run
> +
> +exit $EXIT_STATUS
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh
> b/tools/testing/selftests/net/forwarding/lib.sh
> index be71012b8fc5..8d7348a1834f 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -139,7 +139,6 @@ require_command()
> }
>
> require_command jq
> -require_command $MZ
>
> if [[ ! -v NUM_NETIFS ]]; then
> echo "SKIP: importer does not define \"NUM_NETIFS\""
> @@ -1113,6 +1112,8 @@ learning_test()
> local mac=de:ad:be:ef:13:37
> local ageing_time
>
> + require_command $MZ
> +
> RET=0
>
> bridge -j fdb show br $bridge brport $br_port1 \
> @@ -1188,6 +1189,8 @@ flood_test_do()
> local host2_if=$5
> local err=0
>
> + require_command $MZ
> +
> # Add an ACL on `host2_if` which will tell us whether the packet
> # was flooded to it or not.
> tc qdisc add dev $host2_if ingress
> @@ -1276,6 +1279,8 @@ __start_traffic()
> local dip=$1; shift
> local dmac=$1; shift
>
> + require_command $MZ
> +
> $MZ $h_in -p 8000 -A $sip -B $dip -c 0 \
> -a own -b $dmac -t "$proto" -q "$@" &
> sleep 1
> @@ -1352,6 +1357,8 @@ mcast_packet_test()
> local tc_proto="ip"
> local mz_v6arg=""
>
> + require_command $MZ
> +
> # basic check to see if we were passed an IPv4 address, if not assume
> IPv6
> if [[ ! $ip =~ ^[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}$ ]]; then
> tc_proto="ipv6"
> -----------------------------[ cut here ]-----------------------------
