netdev - 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: <CAOiHx=mbd-JND4XR9FkARa0b6gGS0BXJwuAKSP_gVO48m00ZWg@mail.gmail.com>
Date: Tue, 21 Oct 2025 11:38:31 +0200
From: Jonas Gorski <jonas.gorski@...il.com>
To: Simon Horman <horms@...nel.org>
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 6:43 PM Simon Horman <horms@...nel.org> wrote:
>
> 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.

While I can't send patches, I can at least send emails at work:

How about:

"The internal switch in BCM63XX SoCs ignores the CPU port's egress
untag bit. We use VID 0 internally for untagged traffic, so it becomes
tagged with VID 0. Fix this up by removing the tag if the TCI field is
all 0, and keep it otherwise."

Best regards,
Jonas