Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

On Tue, Mar 09, 2021 at 12:40, Florian Fainelli [off-list ref] wrote:

On 3/9/21 10:42 AM, Tobias Waldekranz wrote:

quoted

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.

quoted

- 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

quoted

- 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

quoted

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@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.fsf@waldekranz.com/ (local)

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/ (local)

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