netdev - Re: [PATCH net] bridge: fix C-VLAN preservation in 802.1ad vlan

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: <d9558681-3113-4993-81a1-ff22873908cf@blackwall.org>
Date: Tue, 30 Dec 2025 11:43:15 +0200
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Alexandre Knecht <knecht.alexandre@...il.com>, netdev@...r.kernel.org
Cc: roopa@...dia.com, Ido Schimmel <idosch@...dia.com>
Subject: Re: [PATCH net] bridge: fix C-VLAN preservation in 802.1ad
 vlan_tunnel egress

On 30/12/2025 11:38, Nikolay Aleksandrov wrote:
> On 28/12/2025 04:00, Alexandre Knecht wrote:
>> When using an 802.1ad bridge with vlan_tunnel, the C-VLAN tag is
>> incorrectly stripped from frames during egress processing.
>>
>> br_handle_egress_vlan_tunnel() uses skb_vlan_pop() to remove the S-VLAN
>> from hwaccel before VXLAN encapsulation. However, skb_vlan_pop() also
>> moves any "next" VLAN from the payload into hwaccel:
>>
>>      /* move next vlan tag to hw accel tag */
>>      __skb_vlan_pop(skb, &vlan_tci);
>>      __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>>
>> For QinQ frames where the C-VLAN sits in the payload, this moves it to
>> hwaccel where it gets lost during VXLAN encapsulation.
>>
>> Fix by calling __vlan_hwaccel_clear_tag() directly, which clears only
>> the hwaccel S-VLAN and leaves the payload untouched.
>>
>> This path is only taken when vlan_tunnel is enabled and tunnel_info
>> is configured, so 802.1Q bridges are unaffected.
>>
>> Tested with 802.1ad bridge + VXLAN vlan_tunnel, verified C-VLAN
>> preserved in VXLAN payload via tcpdump.
>>
>> Fixes: 11538d039ac6 ("bridge: vlan dst_metadata hooks in ingress and 
>> egress paths")
>> Signed-off-by: Alexandre Knecht <knecht.alexandre@...il.com>
>> ---
>>   net/bridge/br_vlan_tunnel.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/bridge/br_vlan_tunnel.c b/net/bridge/br_vlan_tunnel.c
>> index 12de0d1df0bc..a1b62507e521 100644
>> --- a/net/bridge/br_vlan_tunnel.c
>> +++ b/net/bridge/br_vlan_tunnel.c
>> @@ -189,7 +189,6 @@ int br_handle_egress_vlan_tunnel(struct sk_buff *skb,
>>       IP_TUNNEL_DECLARE_FLAGS(flags) = { };
>>       struct metadata_dst *tunnel_dst;
>>       __be64 tunnel_id;
>> -    int err;
>>
>>       if (!vlan)
>>           return 0;
>> @@ -199,9 +198,13 @@ int br_handle_egress_vlan_tunnel(struct sk_buff 
>> *skb,
>>           return 0;
>>
>>       skb_dst_drop(skb);
>> -    err = skb_vlan_pop(skb);
>> -    if (err)
>> -        return err;
>> +    /* For 802.1ad (QinQ), skb_vlan_pop() incorrectly moves the C-VLAN
>> +     * from payload to hwaccel after clearing S-VLAN. We only need to
>> +     * clear the hwaccel S-VLAN; the C-VLAN must stay in payload for
>> +     * correct VXLAN encapsulation. This is also correct for 802.1Q
>> +     * where no C-VLAN exists in payload.
>> +     */
>> +    __vlan_hwaccel_clear_tag(skb);
>>
>>       if (BR_INPUT_SKB_CB(skb)->backup_nhid) {
>>           __set_bit(IP_TUNNEL_KEY_BIT, flags);
>> -- 
>> 2.43.0
>>
> 
> Nice catch. As Ido said, please use get_maintainer.pl next time.
> The change looks good to me as well. Thanks!
> 
> Acked-by: Nikolay Aleksandrov <razor@...ckwall.org>
> 
> 
> 

haha and I missed to add Ido to the reply..
need more coffee this morning :)
Sorry about that!

+ Ido

Cheers,
  Nik