[PATCH net v2] net: dsa: tag_brcm: legacy: fix untagged rx on unbridged ports for bcm63xx

[PATCH net v2] net: dsa: tag_brcm: legacy: fix untagged rx on unbridged ports for bcm63xx

[Jonas Gorski]

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@gmail.com>
---
v1 -> v2:
 * rewrote the comment to make it less wordy (hopefully not too terse)

 net/dsa/tag_brcm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 26bb657ceac3..d9c77fa553b5 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,12 @@ 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 always tags on egress on the CPU
+	 * port. We use VID 0 internally for untagged traffic, so strip the tag
+	 * if the TCI field is all 0, and keep it otherwise to also retain
+	 * e.g. 802.1p tagged packets.
+	 */
+	if (proto[0] == htons(ETH_P_8021Q) && proto[1] == 0)
 		len += VLAN_HLEN;
 
 	/* Remove Broadcom tag and update checksum */

base-commit: 84a905290cb4c3d9a71a9e3b2f2e02e031e7512f
-- 
2.43.0

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

[Vladimir Oltean]

On Mon, Oct 27, 2025 at 08:46:21PM +0100, 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@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Sorry for dropping the ball on v1. To reply to your reply there,
https://lore.kernel.org/netdev/CAOiHx=mNnMJTnAN35D6=LPYVTQB+oEmedwqrkA6VRLRVi13Kjw@mail.gmail.com/
I hadn't realized that b53 sets ds->untag_bridge_pvid conditionally,
which makes any consolidation work in stable trees very complicated
(although still desirable in net-next).

| And to sidetrack the discussion a bit, I wonder if calling
| __vlan_hwaccel_clear_tag() in
| dsa_software_untag_vlan_(un)aware_bridge() without checking the
| vlan_tci field strips 802.1p information from packets that have it. I
| fail to find if this is already parsed and stored somewhere at a first
| glance.

802.1p information should be parsed in vlan_do_receive() if vlan_find_dev()
found something:

	skb->priority = vlan_get_ingress_priority(vlan_dev, skb->vlan_tci);

This logic is invoked straight from __netif_receive_skb_core(), and
relative to dsa_switch_rcv(), it hasn't run yet.

Apart from that and user-configured netfilter/tc rules, I don't think
there's anything else in the kernel that processes the vlan_tci on
ingress (of course that isn't to say anything about user space).

With regard to dsa_software_untag_vlan_unaware_bridge(), which I'd like
to see removed rather than reworked, it does force you to use a br0.0
VLAN upper to not strip the 802.1p info, which is OK.

With regard to dsa_software_untag_vlan_aware_bridge(), it only strips
VID values which are != 0 (because the bridge PVID iself is != 0 - if it
was zero that would be another bug, the port should have dropped the
packet with a VLAN-aware bridge). So it doesn't discard pure 802.1p
information, but it might strip the PCP of a PVID-tagged packet. This
appears to be an area of improvement. It seems reasonable to say that
if the PCP is non-zero, it looked like that on the wire, and wasn't
inserted by the switch.

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

[Jonas Gorski]

On Mon, Oct 27, 2025 at 10:15 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Oct 27, 2025 at 08:46:21PM +0100, 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@gmail.com>
> > ---
>
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
>
> Sorry for dropping the ball on v1. To reply to your reply there,
> https://lore.kernel.org/netdev/CAOiHx=mNnMJTnAN35D6=LPYVTQB+oEmedwqrkA6VRLRVi13Kjw@mail.gmail.com/
> I hadn't realized that b53 sets ds->untag_bridge_pvid conditionally,
> which makes any consolidation work in stable trees very complicated
> (although still desirable in net-next).

It's for some more obscure cases where we cannot use the Broadcom tag,
like a switch where the CPU port isn't a management port but a normal
port. I am not sure this really exists, but maybe Florian knows if
there are any (still used) boards where this applies.

If not, I am more than happy to reject this path as -EINVAL instead of
the current TAG_NONE with untag_bridge_pvid = true.

>
> | And to sidetrack the discussion a bit, I wonder if calling
> | __vlan_hwaccel_clear_tag() in
> | dsa_software_untag_vlan_(un)aware_bridge() without checking the
> | vlan_tci field strips 802.1p information from packets that have it. I
> | fail to find if this is already parsed and stored somewhere at a first
> | glance.
>
> 802.1p information should be parsed in vlan_do_receive() if vlan_find_dev()
> found something:
>
>         skb->priority = vlan_get_ingress_priority(vlan_dev, skb->vlan_tci);
>
> This logic is invoked straight from __netif_receive_skb_core(), and
> relative to dsa_switch_rcv(), it hasn't run yet.

I see. So I presume it runs again after dsa_switch_rcv() has run? Or
rather __netif_receive_skb_core() jumps to to another_round or so?

> Apart from that and user-configured netfilter/tc rules, I don't think
> there's anything else in the kernel that processes the vlan_tci on
> ingress (of course that isn't to say anything about user space).
>
> With regard to dsa_software_untag_vlan_unaware_bridge(), which I'd like
> to see removed rather than reworked, it does force you to use a br0.0
> VLAN upper to not strip the 802.1p info, which is OK.
>
> With regard to dsa_software_untag_vlan_aware_bridge(), it only strips
> VID values which are != 0 (because the bridge PVID iself is != 0 - if it
> was zero that would be another bug, the port should have dropped the
> packet with a VLAN-aware bridge).

AFAICT if a port on a vlan aware bridge has no PVID configured and
receives a VID 0 tagged packet, then
dsa_software_untag_vlan_aware_bridge() will strip it:

Since skb-proto is 802.1Q, we call skb_vlan_untag(), which will set
skb->vlan_proto to 802.1q and skb->vlan_tci to 0

skb->vlan_all is now != 0, so skb_vlan_tag_present() returns true.

skb_vlan_tag_get_id() returns 0, since skb->vlan_tci is 0.

So on a vlan_aware bridge with untag_vlan_aware_bridge_pvid enabled we
now call dsa_software_untag_vlan_aware_bridge() with vid = 0.

In there br_vlan_get_pvid_rcu() sets pvid to 0 if no PVID is
configured since br_get_pvid() returns 0 in that case, so the
condition (vid == pvid) is fulfilled, so we call
__vlan_hwaccel_clear_tag(), stripping the tag.

Obviously for any other configurations (e.g. PVID > 0 and VID = 0) we
don't do anything.

For BCM63xx, the issue with VID 0 tagged traffic on egress on the CPU
port is only if there is no PVID configured on a port - once there is
a PVID, the ingress untagged traffic will be tagged with the
appropriate VLAN ID on egress on the CPU port, and everything is fine.
So in a sense it happens to work by accident for ports without a PVID
and untagged traffic on them.

But vlan aware bridges are the only cases where PVIDs do get
configured/are active, in all other configurations PVID is programmed
as 0 on ports.

Best regards,
Jonas

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

[Jakub Kicinski]

On Tue, 28 Oct 2025 11:15:23 +0100 Jonas Gorski wrote:
> > Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> >
> > Sorry for dropping the ball on v1. To reply to your reply there,
> > https://lore.kernel.org/netdev/CAOiHx=mNnMJTnAN35D6=LPYVTQB+oEmedwqrkA6VRLRVi13Kjw@mail.gmail.com/
> > I hadn't realized that b53 sets ds->untag_bridge_pvid conditionally,
> > which makes any consolidation work in stable trees very complicated
> > (although still desirable in net-next).  
> 
> It's for some more obscure cases where we cannot use the Broadcom tag,
> like a switch where the CPU port isn't a management port but a normal
> port. I am not sure this really exists, but maybe Florian knows if
> there are any (still used) boards where this applies.
> 
> If not, I am more than happy to reject this path as -EINVAL instead of
> the current TAG_NONE with untag_bridge_pvid = true.

IIUC Vladimir is okay with the patch but I realized now that Florian 
is not even CCed here, and ack would be good. Adding him now. And we
should probably add a MAINTAINERS entry for tag_brcm to avoid this in
the future?

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 27 Oct 2025 20:46:21 +0100 you 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.
> 
> [...]

Here is the summary with links:
  - [net,v2] net: dsa: tag_brcm: legacy: fix untagged rx on unbridged ports for bcm63xx
    https://git.kernel.org/netdev/net/c/3d18a84eddde

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Jonas Gorski]

On Thu, Oct 30, 2025 at 2:12 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 28 Oct 2025 11:15:23 +0100 Jonas Gorski wrote:
> > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> > >
> > > Sorry for dropping the ball on v1. To reply to your reply there,
> > > https://lore.kernel.org/netdev/CAOiHx=mNnMJTnAN35D6=LPYVTQB+oEmedwqrkA6VRLRVi13Kjw@mail.gmail.com/
> > > I hadn't realized that b53 sets ds->untag_bridge_pvid conditionally,
> > > which makes any consolidation work in stable trees very complicated
> > > (although still desirable in net-next).
> >
> > It's for some more obscure cases where we cannot use the Broadcom tag,
> > like a switch where the CPU port isn't a management port but a normal
> > port. I am not sure this really exists, but maybe Florian knows if
> > there are any (still used) boards where this applies.
> >
> > If not, I am more than happy to reject this path as -EINVAL instead of
> > the current TAG_NONE with untag_bridge_pvid = true.
>
> IIUC Vladimir is okay with the patch but I realized now that Florian
> is not even CCed here, and ack would be good. Adding him now. And we
> should probably add a MAINTAINERS entry for tag_brcm to avoid this in
> the future?

Oh, I didn't notice, thanks for adding him. And yes, I'll send out a
patch for that shortly.

Best regards,
Jonas

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

[Florian Fainelli]

On 10/29/25 18:12, Jakub Kicinski wrote:
> On Tue, 28 Oct 2025 11:15:23 +0100 Jonas Gorski wrote:
>>> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
>>>
>>> Sorry for dropping the ball on v1. To reply to your reply there,
>>> https://lore.kernel.org/netdev/CAOiHx=mNnMJTnAN35D6=LPYVTQB+oEmedwqrkA6VRLRVi13Kjw@mail.gmail.com/
>>> I hadn't realized that b53 sets ds->untag_bridge_pvid conditionally,
>>> which makes any consolidation work in stable trees very complicated
>>> (although still desirable in net-next).
>>
>> It's for some more obscure cases where we cannot use the Broadcom tag,
>> like a switch where the CPU port isn't a management port but a normal
>> port. I am not sure this really exists, but maybe Florian knows if
>> there are any (still used) boards where this applies.

There are two devices that I encountered where we could not use Broadcom 
tags. One was indeed a case where the CPU port was for reasons unknown 
not the IMP port, and therefore it was not possible to use Broadcom 
tags. This system is not supported anymore and won't be. The second 
device was an external BCM53125 connected to an internal SF2 switch, in 
that case, we cannot enable Broadcom tags on the BCM53125 because there 
is no way to way to cascade both tags one after the other on ingress 
unfortunately...

>>
>> If not, I am more than happy to reject this path as -EINVAL instead of
>> the current TAG_NONE with untag_bridge_pvid = true.
> 
> IIUC Vladimir is okay with the patch but I realized now that Florian
> is not even CCed here, and ack would be good. Adding him now. And we
> should probably add a MAINTAINERS entry for tag_brcm to avoid this in
> the future?

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian