Message-ID: <87czw1pg60.fsf@waldekranz.com>
Date:   Sun, 14 Mar 2021 22:40:55 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>, davem@...emloft.net,
        kuba@...nel.org, andrew@...n.ch, vivien.didelot@...il.com,
        netdev@...r.kernel.org
Subject: Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

On Wed, Mar 10, 2021 at 00:01, Vladimir Oltean <olteanv@...il.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.fainelli@...il.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 <tobias@...dekranz.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.fsf@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-olteanv@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.oltean@....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.oltean@....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 ]-----------------------------

Powered by blists - more mailing lists