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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOiHx=kU_UnAMECgwW3RODkVvojkcPsiy66pS_afF4jSm4HsAg@mail.gmail.com>
Date: Tue, 21 Oct 2025 09:30:44 +0200
From: Jonas Gorski <jonas.gorski@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Vladimir Oltean <olteanv@...il.com>, Andrew Lunn <andrew@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Simon Horman <horms@...nel.org>, 
	Á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 Tue, Oct 21, 2025 at 9:08 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On 10/16/25 1:50 PM, Jonas Gorski wrote:
> > On Thu, Oct 16, 2025 at 12:27 PM Vladimir Oltean <olteanv@...il.com> 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>
> >>> ---
> >>>  net/dsa/tag_brcm.c | 12 ++++++++++--
> >>>  1 file changed, 10 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> >>> index 26bb657ceac3..32879d1b908b 100644
> >>> --- a/net/dsa/tag_brcm.c
> >>> +++ b/net/dsa/tag_brcm.c
> >>> @@ -224,12 +224,14 @@ static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
> >>>  {
> >>>       int len = BRCM_LEG_TAG_LEN;
> >>>       int source_port;
> >>> +     __be16 *proto;
> >>>       u8 *brcm_tag;
> >>>
> >>>       if (unlikely(!pskb_may_pull(skb, BRCM_LEG_TAG_LEN + VLAN_HLEN)))
> >>>               return NULL;
> >>>
> >>>       brcm_tag = dsa_etype_header_pos_rx(skb);
> >>> +     proto = (__be16 *)(brcm_tag + BRCM_LEG_TAG_LEN);
> >>>
> >>>       source_port = brcm_tag[5] & BRCM_LEG_PORT_ID;
> >>>
> >>> @@ -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.
> >>> +      */
> >>> +     if (proto[0] == htons(ETH_P_8021Q) && proto[1] == 0)
> >>>               len += VLAN_HLEN;
> >>>
> >>>       /* Remove Broadcom tag and update checksum */
> >>>
> >>> base-commit: 7f0fddd817ba6daebea1445ae9fab4b6d2294fa8
> >>> --
> >>> 2.43.0
> >>>
> >>
> >> Do I understand correctly the following:
> >>
> >> - b53_default_pvid() returns 0 for this switch
> >> - dsa_software_untag_vlan_unaware_bridge() does not remove it, because,
> >>   as the FIXME says, 0 is not the PVID of the VLAN-unaware bridge (and
> >>   even if it were, the same problem exists for standalone ports and is
> >>   not tackled by that logic)?
> >
> > In general yes. And it happens to work for vlan aware bridges because
> > br_get_pvid() returns 0 if a port has no PVID configured.
> >
> > Also b53 doesn't set untag_bridge_pvid except in very weird edge
> > cases, so dsa_software_untag_vlan_unaware_bridge() isn't even called
> > ;-)
> >
> >> I'm trying to gauge the responsibility split between taggers and
> >> dsa_software_vlan_untag(). We could consider implementing the missing
> >> bits in that function and letting the generic untagging logic do it.
> >
> > If there are more devices that need this, it might make sense. Not
> > sure if this has any negative performance impact compared to directly
> > stripping it along the proprietary tag.
>
> I think this patch makes sense for 'net' and reaching stable trees,
> where most b53 users sits (I think/guess).
>
> The DSA-core base solution could be a follow-up IMHO.
>
> @Jonas, please still clarify a bit the comment, as per Simon's request.

That was my plan until my internet connection at home went down
yesterday, and hasn't been fixed since. Once the ISP fixed it I will
send out a V2 (+/- me being back home from work)

Best regards,
Jonas