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: <b0bc747b-82ee-4d7b-90f9-3ea299d1249c@redhat.com>
Date: Tue, 21 Oct 2025 09:08:14 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jonas Gorski <jonas.gorski@...il.com>, Vladimir Oltean <olteanv@...il.com>
Cc: 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 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.

Thanks,

Paolo

> 
> 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.