Re: [PATCH net] bridge: fix C-VLAN preservation in 802.1ad vlan_tunnel egress

[Ido Schimmel <idosch@nvidia.com>]

+ Nik (Please use scripts/get_maintainer.pl next time)

On Sun, Dec 28, 2025 at 03:00:57AM +0100, 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.

It's not clear from the commit message why 802.1Q bridges are
unaffected. I think you mean that in 802.1Q bridges the C-VLAN is never
in the payload and always in hwaccel (even when Tx VLAN offload is
disabled, thanks to commit 12464bb8de021).

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

Looks correct to me:

Reviewed-by: Ido Schimmel <idosch@nvidia.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