Date:   Tue, 09 Mar 2021 22:28:11 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Florian Fainelli <f.fainelli@...il.com>, davem@...emloft.net,
        kuba@...nel.org
Cc:     andrew@...n.ch, vivien.didelot@...il.com, olteanv@...il.com,
        netdev@...r.kernel.org
Subject: Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

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 :)

Powered by blists - more mailing lists