Message-ID: <CAOiHx==VEbdn3ULHXf5FEBaNAxzyoHTqJEMYYtcQzjkj__RoLg@mail.gmail.com>
Date: Mon, 14 Apr 2025 15:49:27 +0200
From: Jonas Gorski <jonas.gorski@...il.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: Nikolay Aleksandrov <razor@...ckwall.org>, Ido Schimmel <idosch@...dia.com>, 
	"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>, 
	Andrew Lunn <andrew@...n.ch>, bridge@...ts.linux.dev, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net 2/2] net: dsa: propagate brentry flag changes

On Mon, Apr 14, 2025 at 2:52 PM Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> +Jonas, whom I've mistakenly removed from To: :-/
>
> On Mon, Apr 14, 2025 at 03:49:30PM +0300, Vladimir Oltean wrote:
> > On Sat, Apr 12, 2025 at 02:24:28PM +0200, Jonas Gorski wrote:
> > > Currently any flag changes for brentry vlans are ignored, so the
> > > configured cpu port vlan will get stuck at whatever the original flags
> > > were.
> > >
> > > E.g.
> > >
> > > $ bridge vlan add dev swbridge vid 10 self pvid untagged
> > > $ bridge vlan add dev swbridge vid 10 self
> > >
> > > Would cause the vlan to get "stuck" at pvid untagged in the hardware,
> > > despite now being configured as tagged on the bridge.
> > >
> > > Fix this by passing on changed vlans to drivers, but do not increase the
> > > refcount for updates.
> > >
> > > Since we should never get an update for a non-existing VLAN, add a
> > > WARN_ON() in case it happens.
> > >
> > > Fixes: 134ef2388e7f ("net: dsa: add explicit support for host bridge VLANs")
> > > Signed-off-by: Jonas Gorski <jonas.gorski@...il.com>
> > > ---
> >
> > I think it's important to realize that the meaning of the "flags" of
> > VLANs offloaded to the CPU port is not completely defined.
> > "egress-untagged" from the perspective of the hardware CPU port is the
> > opposite direction compared to "egress-untagged" from the perspective of
> > the bridge device (one is Linux RX, the other is Linux TX).
> >
> > Additionally, we install in DSA as host VLANs also those bridge port VLANs
> > which were configured by the user on foreign interfaces. It's not exactly
> > clear how to reconcile the "flags" of a VLAN installed on the bridge
> > itself with the "flags" of a VLAN installed on a foreign bridge port.
> >
> > Example:
> > ip link add br0 type bridge vlan_filtering 1 vlan_default_pvid 0
> > ip link set veth0 master br0 # foreign interface, unrelated to DSA
> > ip link set swp0 master br0 # DSA interface
> > bridge vlan add dev br0 vid 1 self pvid untagged # leads to an "dsa_vlan_add_hw: cpu port N vid 1 untagged" trace event
> > bridge vlan add dev veth0 vid 1 # still leads to an "dsa_vlan_add_bump: cpu port N vid 1 refcount 2" trace event after your change
> >
> > Depending on your expectations, you might think that host VID 1 would
> > also need to become egress-tagged in this case, although from the
> > bridge's perspective, it hasn't "changed", because it is a VLAN from a
> > different VLAN group (port veth0 vs bridge br0).
> >
> > The reverse is true as well. Because the user can toggle the "pvid" flag
> > of the bridge VLAN, that will make the switchdev object be notified with
> > changed=true. But since DSA clears BRIDGE_VLAN_INFO_PVID, the host VLAN,
> > as programmed to hardware, would be identical, yet we reprogram it anyway.
> >
> > Both would seem to indicate that "changed" from the bridge perspective
> > is not what matters for calling the driver, but a different "changed"
> > flag, calculated by DSA from its own perspective.
> >
> > I was a bit reluctant to add such complexity in dsa_port_do_vlan_add(),
> > considering that many drivers treat the VLANs on the CPU port as
> > always-tagged towards software (not b53 though, except for
> > b53_vlan_port_needs_forced_tagged() which is only for DSA_TAG_PROTO_NONE).
> > In fact, what is not entirely clear to me is what happens if they _don't_
> > treat the CPU port in a special way. Because software needs to know in
> > which VLAN did the hardware begin to process a packet: if the software
> > bridge needs to continue the processing of that packet, it needs to do
> > so _in the same VLAN_. If the accelerator sends packets as VLAN-untagged
> > to software, that information is lost and VLAN hopping might take place.
> > So I was hoping that nobody would notice that the change of flags on
> > host VLANs isn't propagated to drivers, because none of the flags should
> > be of particular relevance in the first place.
> >
> > I would like to understand better, in terms of user-visible impact, what
> > is the problem that you see?

I was just in the progress of writing down some thoughts myself I had
while thinking about this.

So to me passing on the original flags but then no updates of these
flags feels wrong. I would expect them to be either never passed on,
or always sync it (and let the driver choose to handle or ignore
them).

Having the cpu port as egress untagged I can easily find one case
where this breaks stuff (at least with b53):

With lan1, lan2 being dsa ports of the same switch:

# bridge with lan1
$ ip link add swbridge type bridge vlan_filtering 1
$ ip link set lan1 master swbridge
$ bridge vlan add dev swbridge vid 10 self pvid untagged

# lan2 stand alone
$ ip link add lan2.10 link lan2 type vlan id 10

as then lan2.10 would never receive any packets, as the VLAN 10
packets received by the CPU ports never carry any vlan tags.

The core issue here is that b53 switches do not provide any way of
knowing the original tagged state of received packets, as the dsa
header has no field for that (bcm56* switches do, but these are a
different beast).

I guess the proper fix for b53 is probably to always have a vlan tag
on the cpu port (except for the special vlan 0 for untagged traffic on
ports with no PVID), and enable untag_vlan_aware_bridge_pvid.

To continue the stream of consciousness, it probably does not make
sense to pass on the untagged flag for the bridge/cpu port, because it
will affect all ports of the switch, regardless of them being member
of the bridge. Looking through drivers in net/drivers/dsa, I don't see
anyone checking if egress untag is applied to the cpu port, so I
wonder if not most, maybe even all (dsa) switch drivers have the same
issue and would actually need to keep the cpu port always tagged. And
looking through the tag_* handlers, only ocelot looks like it may have
the information available whether a packet was originally tagged or
not, so would also need to have untag_vlan_aware_bridge_pvid enabled.

Makes the think the cpu port always tagged should be the default
behavior. It's easy to strip a vlan tag that shouldn't be there, but
hard to add one that's missing, especially since in contrast to PVID
you can have more than one vlan as egress untagged.

Jonas

Powered by blists - more mailing lists