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 for Android: free password hash cracker in your pocket

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

Message-ID: <20251016102725.x5gqyehuyu44ejj3@skbuf>
Date: Thu, 16 Oct 2025 13:27:25 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Jonas Gorski <jonas.gorski@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, "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>,
	Á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

Hi Jonas,

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)?

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.