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

[Vladimir Oltean <olteanv@gmail.com>]

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.