linux-kernel - Re: [PATCH net] net: dsa: tag_brcm: legacy: fix untagged rx on unbridged ports for bcm63xx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aO_PMWQlv0DhHukm@horms.kernel.org>
Date: Wed, 15 Oct 2025 17:43:29 +0100
From: Simon Horman <horms@...nel.org>
To: Jonas Gorski <jonas.gorski@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Álvaro Fernández Rojas <noltari@...il.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: tag_brcm: legacy: fix untagged rx on
 unbridged ports for bcm63xx

On Wed, Oct 15, 2025 at 06:24:33PM +0200, Jonas Gorski wrote:
> On Wed, Oct 15, 2025 at 6:12 PM Simon Horman <horms@...nel.org> wrote:
> >
> > On Wed, Oct 15, 2025 at 09:08:54AM +0200, Jonas Gorski wrote:
> > > The internal switch on BCM63XX SoCs will unconditionally add 802.1Q VLAN
> > > tags on egress to CPU when 802.1Q mode is enabled. We do this
> > > unconditionally since commit ed409f3bbaa5 ("net: dsa: b53: Configure
> > > VLANs while not filtering").
> > >
> > > This is fine for VLAN aware bridges, but for standalone ports and vlan
> > > unaware bridges this means all packets are tagged with the default VID,
> > > which is 0.
> > >
> > > While the kernel will treat that like untagged, this can break userspace
> > > applications processing raw packets, expecting untagged traffic, like
> > > STP daemons.
> > >
> > > This also breaks several bridge tests, where the tcpdump output then
> > > does not match the expected output anymore.
> > >
> > > Since 0 isn't a valid VID, just strip out the VLAN tag if we encounter
> > > it, unless the priority field is set, since that would be a valid tag
> > > again.
> > >
> > > Fixes: 964dbf186eaa ("net: dsa: tag_brcm: add support for legacy tags")
> > > Signed-off-by: Jonas Gorski <jonas.gorski@...il.com>
> >
> > ...
> >
> > > @@ -237,8 +239,14 @@ static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
> > >       if (!skb->dev)
> > >               return NULL;
> > >
> > > -     /* VLAN tag is added by BCM63xx internal switch */
> > > -     if (netdev_uses_dsa(skb->dev))
> > > +     /* The internal switch in BCM63XX SoCs will add a 802.1Q VLAN tag on
> > > +      * egress to the CPU port for all packets, regardless of the untag bit
> > > +      * in the VLAN table.  VID 0 is used for untagged traffic on unbridged
> > > +      * ports and vlan unaware bridges. If we encounter a VID 0 tagged
> > > +      * packet, we know it is supposed to be untagged, so strip the VLAN
> > > +      * tag as well in that case.
> >
> > Maybe it isn't important, but here it is a TCI 0 that is being checked:
> > VID 0, PCP 0, and DEI 0.
> 
> Right, that is intentional (I tried to convey it in the commit
> message, though should probably also extend it here).

Thanks, I see that more clearly now.

> If any of the fields is non-zero, then the tag is meaningful, and we
> don't want to strip it (e.g. 802.1p tagged packets).

I guess there are already a lot of words there. But, FWIIW, I would lean
to wards tightening up the comment a bit.