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

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

Message-ID: <7cde982c-a9c1-4b9e-8d73-458ebede9bcc@blackwall.org>
Date: Tue, 30 Dec 2025 11:38:20 +0200
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Alexandre Knecht <knecht.alexandre@...il.com>, netdev@...r.kernel.org
Cc: roopa@...dia.com
Subject: Re: [PATCH net] bridge: fix C-VLAN preservation in 802.1ad
 vlan_tunnel egress

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>