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

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

[Alexandre Knecht]

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

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

[Ido Schimmel]

+ 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

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

[Nikolay Aleksandrov]

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@gmail.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@blackwall.org>

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

[Nikolay Aleksandrov]

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@gmail.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@blackwall.org>
> 
> 
> 

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

+ Ido

Cheers,
  Nik

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

[Alexandre Knecht]

Hi Ido, Nik, thanks for the review. I'm sorry for the missing
maintainers, I felt so bad when I saw the bot result after posting,
will not happen ever again.

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

You're right that I should clarify, but I think we may be talking
about different scenarios.

The 802.1Q bridge I mentioned is specifically the case with
vlan_tunnel enabled (for VXLAN bridging). In this setup:

1. The bridge uses per-vlan br_tunnel_info to map C-VLAN (0x8100) to VNI
2. On egress via br_handle_egress_vlan_tunnel(), the C-VLAN is cleared
from hwaccel and used to set the tunnel_id in dst_metadata
3. After skb_vlan_pop() clears hwaccel, skb->protocol is the actual
payload type (e.g., IPv4), not a VLAN ethertype, so the second phase
of skb_vlan_pop() that pops nested VLANs from payload never triggers

So in this 802.1Q + vlan_tunnel path, there's no C-VLAN in hwaccel at
the VXLAN driver entry point.

For 802.1ad bridges, here is what I understood of the flow at egress
in br_handle_egress_vlan_tunnel():

  // 1. S-VLAN is in hwaccel at this point (bridge put it there)
  tunnel_id = READ_ONCE(vlan->tinfo.tunnel_id);
  if (!tunnel_id || unlikely(!skb_vlan_tag_present(skb))) // ← checks hwaccel!
  return 0;

  // 2. Set tunnel metadata (VNI) : this goes in skb_dst, NOT hwaccel
  skb_dst_drop(skb);
  // ... creates metadata_dst with tunnel_id ...
  skb_dst_set(skb, &tunnel_dst->dst);

  // 3. Now remove the S-VLAN from hwaccel (it's been "consumed" for VNI lookup)
  err = skb_vlan_pop(skb); // ← BUG: also pops C-VLAN from payload

So at the moment skb_vlan_pop() is called:
- S-VLAN: in hwaccel (skb->vlan_tci) : needs to be cleared
- C-VLAN: in packet payload : should NOT be touched
- VNI: already set in skb_dst() metadata : separate from VLAN

The S-VLAN was used to lookup the VNI, but it's still sitting in
hwaccel and needs to be removed before the packet goes into the VXLAN
tunnel. The problem is skb_vlan_pop() does more than just clear
hwaccel, it also "normalizes" any nested VLAN it finds in the payload.
The fix uses __vlan_hwaccel_clear_tag() instead, which only clears
hwaccel without touching the payload.

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

[Ido Schimmel]

On Tue, Dec 30, 2025 at 12:25:39PM +0100, Alexandre Knecht wrote:
> Hi Ido, Nik, thanks for the review. I'm sorry for the missing
> maintainers, I felt so bad when I saw the bot result after posting,
> will not happen ever again.
> 
> > 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).
> 
> You're right that I should clarify, but I think we may be talking
> about different scenarios.
> 
> The 802.1Q bridge I mentioned is specifically the case with
> vlan_tunnel enabled (for VXLAN bridging). In this setup:
> 
> 1. The bridge uses per-vlan br_tunnel_info to map C-VLAN (0x8100) to VNI
> 2. On egress via br_handle_egress_vlan_tunnel(), the C-VLAN is cleared
> from hwaccel and used to set the tunnel_id in dst_metadata
> 3. After skb_vlan_pop() clears hwaccel, skb->protocol is the actual
> payload type (e.g., IPv4), not a VLAN ethertype, so the second phase
> of skb_vlan_pop() that pops nested VLANs from payload never triggers
> 
> So in this 802.1Q + vlan_tunnel path, there's no C-VLAN in hwaccel at
> the VXLAN driver entry point.
> 
> For 802.1ad bridges, here is what I understood of the flow at egress
> in br_handle_egress_vlan_tunnel():
> 
>   // 1. S-VLAN is in hwaccel at this point (bridge put it there)
>   tunnel_id = READ_ONCE(vlan->tinfo.tunnel_id);
>   if (!tunnel_id || unlikely(!skb_vlan_tag_present(skb))) // ← checks hwaccel!
>   return 0;
> 
>   // 2. Set tunnel metadata (VNI) : this goes in skb_dst, NOT hwaccel
>   skb_dst_drop(skb);
>   // ... creates metadata_dst with tunnel_id ...
>   skb_dst_set(skb, &tunnel_dst->dst);
> 
>   // 3. Now remove the S-VLAN from hwaccel (it's been "consumed" for VNI lookup)
>   err = skb_vlan_pop(skb); // ← BUG: also pops C-VLAN from payload
> 
> So at the moment skb_vlan_pop() is called:
> - S-VLAN: in hwaccel (skb->vlan_tci) : needs to be cleared
> - C-VLAN: in packet payload : should NOT be touched
> - VNI: already set in skb_dst() metadata : separate from VLAN
> 
> The S-VLAN was used to lookup the VNI, but it's still sitting in
> hwaccel and needs to be removed before the packet goes into the VXLAN
> tunnel. The problem is skb_vlan_pop() does more than just clear
> hwaccel, it also "normalizes" any nested VLAN it finds in the payload.
> The fix uses __vlan_hwaccel_clear_tag() instead, which only clears
> hwaccel without touching the payload.

I agree with the analysis and the fix looks correct to me. Also tested
with test_vxlan_vnifiltering.sh which tests the 802.1Q scenario.