Re: [PATCH net 1/2] net: dsa: Accept software VLANs for stacked interfaces

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Mar 08, 2021 at 09:00:49PM +0100, Tobias Waldekranz wrote:
> Alright, we do not want to lie to the stack, got it...

[...]

> ...hang on, are we OK with lying or not? Yes, I guess?

I'm not too happy about it. The problem in my mind, really, is that if
we disable 'rx-vlan-filter' and we gain an 8021q upper in the meantime,
we'll lose the .ndo_vlan_rx_add_vid call for it. This is worse in my
opinion than saying you're going to drop unknown VLANs but not actually
doing it.

> > It's a lot easier that way, otherwise you will end up having to replay
> > them somehow.
> 
> I think vlan_for_each should be enough to perform the replay when
> toggling VLAN filtering on a port?

Yes, good point about vlan_for_each, I didn't notice that, since almost
nobody uses it, and absolutely nobody uses it for replaying VLANs in the
RX filter, but it looks like it might be able to do the trick.

> More importantly, there are other sequences that we do not guard against
> today:
> 
> - Adding VID to a bridge port that is used on an 1Q upper of another
>   bridged port.
> 
>     .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
> 
>     After this sequence, the switch will forward VID 100 tagged frames
>     between lan0 and lan1.

Yes, this is not caught today. Should be trivially fixed by iterating
over all dp->bridge_dev lowers in dsa_slave_vlan_add, when calling
dsa_slave_vlan_check_for_8021q_uppers, not just for the specified port.

> - Briding two ports that both have 1Q uppers using the same VID.
> 
>     .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
> 
>     This is also allowed by DSA today, and produces the same switch
>     config as the previous sequence.

Correct, this is also not caught.
In this case it looks like there isn't even an attempt to validate the
VLAN configuration of the ports already in the bridge. We would probably
have to hook into dsa_port_bridge_join, iterate through all the VLAN
uppers of the new port, then for each VLAN upper we should construct a
fake struct switchdev_obj_port_vlan and call dsa_slave_vlan_check_for_8021q_uppers
again for all lowers of the bridge which we're about to join that are
DSA ports. Patches welcome!

> So in summary:
> 
> - Try to design some generic VLAN validation that can be used when:
>   - Adding VLANs to standalone ports.
>   - Adding VLANs to bridged ports.
>   - Toggling VLAN filtering on ports.

What do you mean 'generic'?

> - Remove 1/2.
> - Rework 2/2 to:
>   - `return 0` when adding a VLAN to a non-bridged port, not -EOPNOTSUPP.

Still in mv88e6xxx you mean? Well, if mv88e6xxx is still not going to
install the VLAN to hardware, why would it lie to DSA and return 0?

>   - Lazy load/unload VIDs from VLAN uppers when toggling filtering on a
>     port using vlan_for_each or similar.

How do you plan to do it exactly? Hook into dsa_port_vlan_filtering and:
if vlan_filtering == false, then do vlan_for_each(..., dsa_slave_vlan_rx_kill_vid)
if vlan_filtering == true, then do vlan_for_each(..., dsa_slave_vlan_rx_add_vid)?
Basically when VLAN filtering is disabled, the VTU will only contain the
bridge VLANs and none of the 8021q VLANs?

If we make this happen, then my patches for runtime toggling
'rx-vlan-filter' should also be needed.

> Does that sound reasonable?
> 
> Are we still in net territory or is this more suited for net-next?

It'll be a lot of patches, but the base logic is there already, so I
think we could still target 'net'.