Message-ID: <20210316224130.ehqkcfktxwvj4j4s@skbuf>
Date:   Wed, 17 Mar 2021 00:41:30 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        netdev@...r.kernel.org, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2 net] net: dsa: Centralize validation of VLAN
 configuration

On Tue, Mar 16, 2021 at 10:49:52PM +0100, Tobias Waldekranz wrote:
> There are four kinds of events that have an impact on VLAN
> configuration of DSA ports:
> 
> - Adding VLAN uppers
>   (ip link add dev swp0.1 link swp0 type vlan id 1)
> 
> - Bridging VLAN uppers
>   (ip link set dev swp0.1 master br0)
> 
> - Adding bridge 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)

Expanding on my idea from v1, thinking out loud a bit:
https://patchwork.kernel.org/project/netdevbpf/patch/20210315195413.2679929-1-tobias@waldekranz.com/

All of these configurations could be supported by unoffloading
_something_.

> For all of these events, we want to ensure that some invariants are
> upheld for offloaded ports belonging to our tree:
> 
> - For hardware where VLAN filtering is a global setting, either all
>   bridges must use VLAN filtering, or no bridge can.

Ok, for this one I don't know what I would unoffload. The bridge with
the smallest throughput, I guess? Anyway, it would seem pretty arbitrary
for DSA to take this decision by itself.

> - For all filtering bridges, no VID may be configured on more than one
>   VLAN upper. An example of a violation of this would be:
> 
>   .100  br0  .100
>      \  / \  /
>      swp0 swp1
> 
>   $ ip link add dev br0 type bridge vlan_filtering 1
>   $ ip link add dev swp0.100 link swp0 type vlan id 100
>   $ ip link set dev swp0 master br0
>   $ ip link add dev swp1.100 link swp0 type vlan id 100
>   $ ip link set dev swp1 master br0

In this case, all bridge ports having 8021q coinciding uppers, except
one, should be unoffloaded.

> - For all filtering bridges, no upper VLAN may share a bridge with
>   another offloaded port. An example of a violation of this would be:
> 
>        br0
>       /  |
>      /   |
>   .100   |
>     |    |
>    swp0 swp1
> 
>   $ ip link add dev br0 type bridge vlan_filtering 1
>   $ ip link add dev swp0.100 link swp0 type vlan id 100
>   $ ip link set dev swp0.100 master br0
>   $ ip link set dev swp1 master br0

In this case it's pretty simple, we simply do not offload an 8021q upper
as a bridge port. With the patches that I hope to send tomorrow for
SWITCHDEV_BRPORT_OFFLOADED, this should be just as supported as an
unoffloaded LAG added to a bridge.

Actually for this one, I think there is the same limitation for both the
vlan_filtering=0 and vlan_filtering=1 cases. When vlan_filtering=0,
traffic coming from swp1 might be VLAN-tagged, and the expectation is
that the swp0.100 bridge port adds yet another VLAN tag on egress.

> - For all filtering bridges, no VID that exists in the bridge may be
>   used by a VLAN upper. An example of a violation of this would be:
> 
>       br0
>      (100)
>        |
>   .100 |
>      \ |
>      swp0
> 
>   $ ip link add dev br0 type bridge vlan_filtering 1
>   $ ip link add dev swp0.100 link swp0 type vlan id 100
>   $ ip link set dev swp0 master br0
>   $ bridge vlan add dev swp0 vid 100

With software interfaces, this configuration would mean that the VLAN
100 traffic is stolen from the bridge's data path by swp0.100, which
makes it pointless to let the bridge use that VLAN in the first place.

It is clear in this case too what port should be unoffloaded.

> 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.
> 
> Reported-by: kernel test robot <lkp@...el.com>
> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
> ---

So without a clear use case for the corner cases above, I think I just
debunked by own idea about unoffloading bridge ports as being a useful
way to react.

I'll do some testing tomorrow, sorry for the rambling.

These are all really marginal conditions which nobody should be hitting
under normal usage, I'm starting to reconsider whether this is appropriate
as "net" material vs "net-next" (and the advantage of sending to "net-next"
would be that you could send the selftests too). I know that code in
general should only be as simple as necessary to be correct, but with
switchdev, even the bare minimum for correctness is a hell of a lot.
Vivien, Florian, Andrew, what do you think, net or net-next?

Powered by blists - more mailing lists