Message-ID: <CAOiHx==qdf+NVk5wG2J48gVBovp-UQuO1iS+6cCFbKyGyR0uqw@mail.gmail.com>
Date: Sat, 26 Apr 2025 17:44:33 +0200
From: Jonas Gorski <jonas.gorski@...il.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: Vladimir Oltean <olteanv@...il.com>, Florian Fainelli <f.fainelli@...il.com>, 
	Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: fix VLAN 0 filter imbalance when toggling filtering

On Fri, Apr 25, 2025 at 1:42 PM Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> On Fri, Apr 25, 2025 at 09:52:13AM +0200, Jonas Gorski wrote:
> > I gave it a test with a vlan_filtering bridge with no PVID / egress
> > untagged vlan defined on a pure software bridge, and STP continued to
> > work fine.
>
> STP is not part of the bridge data path, it is control path. The PVID
> rules don't apply to it.
> In software terms, br_handle_frame() returns RX_HANDLER_PASS for it, it
> doesn't go through br_handle_vlan().
>
> > So in a sense, VLAN 0 is needed, as we still need to allow
> > untagged traffic to be received regardless of a PVID egress untagged
> > VLAN being defined.
>
> When we are talking about the hardware data path of a switchdev port,
> that is debatable as well, since many switchdevs have built-in packet
> traps which again bypass the VLAN table (a function specific to the
> switching layer, like learning, STP state etc). I would argue that the
> presence of VID 0 in the RX filtering table is irrelevant for STP as far
> as switchdevs are concerned.

For b53, it is not, at least in the current state. LLC packets (and
other reserved multicast) are only redirected to CPU if there is a
PVID VLAN defined. But this isn't unfixable, I just noticed we
explicitly enable that reserved multicast follows VLAN configuration.

> > But we shouldn't forward it (except to the cpu port) unless it is part
> > of a PVID egress untagged VLAN. This is the tricky part. If (dsa)
> > switch drivers ensure that untagged traffic always reaches the cpu
> > port, then we can ignore VLAN 0.
> >
> > So I think this boils down to that dsa needs a way to pass on to
> > drivers whether a VLAN should be forwarded to other members or not
> > when adding it to a port.
>
> That can be done (add a struct dsa_db argument to port_vlan_add(),
> signifying whether it is a port VLAN or a bridge VLAN), but I haven't
> come across switches which can make the distinction. It would require
> mapping the same VID, coming from different ports, to different hardware
> FIDs.

At least for b53, the workaround I see that could work is (for the
filtering case):

1. enable trap of vlan membership violations to cpu (this also
disables learning for those packets)
2. when the first port installs a vlan filter, have a vlan entry with
just the cpu port
3. when the last port removes the filter, remove the cpu port again

The theoretical result should be:

1. if the vlan does not exist on the bridge, and no standalone port
has a vlan upper with it, it will be dropped
2. if the vlan does exist on the bridge, it will be forwarded between
ports that are members of it
3. if there is a vlan upper, that ports traffic will be redirected to
the cpu port

Since all packets will be redirected to the cpu port, and learning is
disabled for these packets, there is no need for a separate FID for
that port/vlan, and it would ensure that vlan uppers are separated
between each other.

This would even work for VLANs that are already configured on the
bridge (though switchdev.rst says that isn't allowed).

The down side would be that any VLAN membership violation for
configured VLANs would be redirected to the cpu port, though it's
probably still less than e.g. switching ports to standalone and do
forwarding in software (which would be an alternative for allowing
multiple VLAN uppers with a vlan aware bridge).

> > Currently, from a dsa driver perspective, the following two scenarios
> > would be indistinguishable:
> >
> > $ ip link add br0 type bridge vlan_filtering 1
> > $ ip link set sw1p1 master br0
> > $ ip link set sw1p2 master br0
> > $ bridge vlan add dev sw1p1 vid 10
> > $ bridge vlan add dev sw2p1 vid 10
> >
> > and
> >
> > $ ip link add br0 type bridge vlan_filtering 1
> > $ ip link set sw1p1 master br0
> > $ ip link set sw1p2 master br0
> > $ ip link add sw1p1.10 link sw1p1 type vlan id 10
> > $ ip link add sw1p2.10 link sw1p2 type vlan id 10
> >
> > But in the second case, swp1p1 and sw1p2 should be isolated.
> >
> > This is because vlan filters and bridge vlans result in the same
> > port_vlan_add() call, with no way of the driver to tell from where the
> > call comes from.
> >
> > And yes, this is something that is probably hard to configure for many
> > smaller embedded switch chips. E.g. b53 supported switches do not have
> > forward/flood/etc masks per VLAN, so some cheating/workaround is
> > needed here. switchdev.rst says to fall back to software forwarding if
> > there is no other way. I have some ideas, but I will first need to
> > verify that they work ... .
>
> We have insufficient coverage in dsa_user_prechangeupper_sanity_check()
> and dsa_port_can_apply_vlan_filtering(), but we should add another
> restriction for this: 8021q uppers with the same VID should not be
> installed to ports spanning the same VLAN-aware bridge. And there should
> be a new test for it in tools/testing/selftests/net/forwarding/no_forwarding.sh.
>
> The restriction can be selectively lifted if there ever appear drivers
> which can make the distinction you are talking about, but I don't think
> that any of them can, at the moment.

AFAICT, we probably then also need to deny any vlan uppers on ports
bridged via vlan unaware bridges for now, as we currently don't
actually configure them at all.

switchdev.rst says "Frames ingressing the device with a VID that is
not programmed into the bridge/switch's VLAN table must be forwarded
and may be processed using a VLAN device (see below)" (I thought with
a vlan unaware bridge we do not program the VLAN table? anway ...)

"- with VLAN filtering turned off, the bridge will process all ingress traffic
  for the port, except for the traffic tagged with a VLAN ID destined for a
  VLAN upper."

This is currently not happening when creating a vlan upper, at least I
don't see any port_vlan_add() (?) calls for that. And even then, it
would be indistinguishable from the port_vlan_add() calls that happen
if you add a vlan to the bridge's vlan table, which you can, and DSA
passes on, but should not be acted upon by the switch until vlan
filtering is turned on.

Also in general vlan unaware bridges feel like they have a lot of
complicated and probably unsupportable setups.

Like for example:

br0 { sw1p1, sw1p2, sw1p3, sw1p4 }
br1 { sw1p1.10, sw1p4.10 }

would AFAIU mean:

- forward all traffic except vlan 10 tagged between all 4 ports
- vlan 10 tagged traffic from sw1p1 can only go to sw1p4
- vlan 10 tagged traffic from sw1p4 can only go to sw1p1
- vlan 10 tagged traffic from sw1p2 and sw1p3 can go to all other ports

I'm not confident that I could program that correctly on a Broadcom
XGS switch, and these support a *lot* of VLAN shenanigans.

This probably leaves:

"    * Treat bridge ports with VLAN upper interfaces as standalone, and let
      forwarding be handled in the software data path."

as the only viable option here (I have to admit I don't really
understand what the first option is suggesting to do).

Best regards,
Jonas

Powered by blists - more mailing lists