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

On Mon, Mar 08, 2021 at 22:50, Vladimir Oltean [off-list ref] wrote:

On Mon, Mar 08, 2021 at 09:00:49PM +0100, Tobias Waldekranz wrote:

quoted

Alright, we do not want to lie to the stack, got it...

[...]

quoted

...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.

quoted

quoted

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.

quoted

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.

quoted

- 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!

quoted

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'?

I get the sense that one reason that the mentioned cases are not caught
by the existing validation logic, is that checks are scattered in
multiple places (primarily dsa_slave_check_8021q_upper and
dsa_port_can_apply_vlan_filtering).

Ideally we should have a single function that answers the question
"given the current VLAN config, is it OK to make this one modification?"

This is all still very hand-waivy though, it might not be possible.

quoted

- 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?

Because we want the core to add the `struct vlan_vid_info` to our port's
vlan list so that we can lazy load it if/when needed. But maybe your
dynamic rx-vlan-filter patch will render that unnecessary?

quoted

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

I was just going to hook in to mv88e6xxx_port_vlan_filtering, call
vlan_for_each and generate an mv88e6xxx-internal call to add the
VIDs to the port and the CPU port. This does rely on rx-vlan-filter
always being enabled as the VLAN will be setup on all DSA ports at that
point, just not on any user ports.

Basically when VLAN filtering is disabled, the VTU will only contain the
bridge VLANs and none of the 8021q VLANs?

Yes, the switch won't be able to use the 1Q VLANs for anything useful
anyway.

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

I am fine with that, but I think that means that we need to solve the
replay at the DSA layer in order to setup DSA ports correctly.

quoted

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'.