netdev - Re: [PATCH net v2] 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: <20251027211540.dnjanhdbolt5asxi@skbuf>
Date: Mon, 27 Oct 2025 23:15:40 +0200
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 v2] net: dsa: tag_brcm: legacy: fix untagged rx on
 unbridged ports for bcm63xx

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@...il.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@...il.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.