[PATCH v2 nf] netfilter: nf_flow_table_ip: Introduce nf_flow_vlan_push()

[PATCH v2 nf] netfilter: nf_flow_table_ip: Introduce nf_flow_vlan_push()

[Eric Woudstra]

With double vlan tagged packets in the fastpath, getting the error:

skb_vlan_push got skb with skb->data not at mac header (offset 18)

Introduce nf_flow_vlan_push(), that can correctly push the inner vlan
in the fastpath. It is closedly modelled on existing nf_flow_pppoe_push()

Fixes: c653d5a78f34 ("netfilter: flowtable: inline vlan encapsulation in xmit path")
Signed-off-by: Eric Woudstra <ericwouds@gmail.com>

---

Changes in v2:

- Targetting nf.git
- Amended commit message

 net/netfilter/nf_flow_table_ip.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index 3fdb10d9bf7f..e65c8148688e 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -544,6 +544,27 @@ static int nf_flow_offload_forward(struct nf_flowtable_ctx *ctx,
 	return 1;
 }
 
+static int nf_flow_vlan_push(struct sk_buff *skb, __be16 proto, u16 id)
+{
+	if (skb_vlan_tag_present(skb)) {
+		struct vlan_hdr *vhdr;
+
+		if (skb_cow_head(skb, VLAN_HLEN))
+			return -1;
+
+		__skb_push(skb, VLAN_HLEN);
+		skb_reset_network_header(skb);
+
+		vhdr = (struct vlan_hdr *)(skb->data);
+		vhdr->h_vlan_TCI = htons(id);
+		vhdr->h_vlan_encapsulated_proto = skb->protocol;
+		skb->protocol = proto;
+	} else {
+		__vlan_hwaccel_put_tag(skb, proto, id);
+	}
+	return 0;
+}
+
 static int nf_flow_pppoe_push(struct sk_buff *skb, u16 id)
 {
 	int data_len = skb->len + sizeof(__be16);
@@ -738,8 +759,8 @@ static int nf_flow_encap_push(struct sk_buff *skb,
 		switch (tuple->encap[i].proto) {
 		case htons(ETH_P_8021Q):
 		case htons(ETH_P_8021AD):
-			if (skb_vlan_push(skb, tuple->encap[i].proto,
-					  tuple->encap[i].id) < 0)
+			if (nf_flow_vlan_push(skb, tuple->encap[i].proto,
+					      tuple->encap[i].id) < 0)
 				return -1;
 			break;
 		case htons(ETH_P_PPP_SES):
-- 
2.53.0

Re: [PATCH v2 nf] netfilter: nf_flow_table_ip: Introduce nf_flow_vlan_push()

[Florian Westphal]

Eric Woudstra <ericwouds@gmail.com> wrote:

Hi Eric

> With double vlan tagged packets in the fastpath, getting the error:
> 
> skb_vlan_push got skb with skb->data not at mac header (offset 18)
> 
> Introduce nf_flow_vlan_push(), that can correctly push the inner vlan
> in the fastpath. It is closedly modelled on existing nf_flow_pppoe_push()
> 
> Fixes: c653d5a78f34 ("netfilter: flowtable: inline vlan encapsulation in xmit path")
> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
  
> +static int nf_flow_vlan_push(struct sk_buff *skb, __be16 proto, u16 id)
> +{
> +	if (skb_vlan_tag_present(skb)) {
> +		struct vlan_hdr *vhdr;
> +
> +		if (skb_cow_head(skb, VLAN_HLEN))
> +			return -1;
> +
> +		__skb_push(skb, VLAN_HLEN);
> +		skb_reset_network_header(skb);
> +
> +		vhdr = (struct vlan_hdr *)(skb->data);
> +		vhdr->h_vlan_TCI = htons(id);
> +		vhdr->h_vlan_encapsulated_proto = skb->protocol;
> +		skb->protocol = proto;
> +	} else {
> +		__vlan_hwaccel_put_tag(skb, proto, id);
> +	}

I did not apply this because I'm not sure if this preserves correct tag
order.  Can you clarify?

Lets consider vlan-offload-doesn't-exist case.

First loop pushes vlan tag 1, we get:

  [vlan1][inet]

2nd items pushes vlan tag 2, we get:
  [vlan2][vlan1][inet]

Now lets consider with-offload.  We have one tag only, so we get 1 skb with hwaccel
tag in the sk_buff.  This is fine, HW will insert it for us.

But now lets consider two tags:

First loop pushes vlan1, we get the vlan1 tag in sk_buff vlan info.
Packet is: [inet].

2nd loop pushes vlan2, we get:
	[vlan2][inet].

Now, when packet is transmitted, where will the HW insert the tag?

[vlan1][vlan2][inet]?
Or will this be [vlan2][vlan1][inet]?

reading though the SW-side (no hw support), it seems it will do the right
thing (i.e.. the hw tag gets added as the inner tag, right before inet
and not added as outermost tag.

Can you confirm thats the case?

Sorry for not responding sooner, there are lots of patches atm and
I forgot about this one when I yanked it off the previous pull request
at the last second.

Re: [PATCH v2 nf] netfilter: nf_flow_table_ip: Introduce nf_flow_vlan_push()

[Eric Woudstra]

On 3/9/26 10:22 PM, Florian Westphal wrote:
> Eric Woudstra <ericwouds@gmail.com> wrote:
> 
> Hi Eric
> 
>> With double vlan tagged packets in the fastpath, getting the error:
>>
>> skb_vlan_push got skb with skb->data not at mac header (offset 18)
>>
>> Introduce nf_flow_vlan_push(), that can correctly push the inner vlan
>> in the fastpath. It is closedly modelled on existing nf_flow_pppoe_push()
>>
>> Fixes: c653d5a78f34 ("netfilter: flowtable: inline vlan encapsulation in xmit path")
>> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
>   
>> +static int nf_flow_vlan_push(struct sk_buff *skb, __be16 proto, u16 id)
>> +{
>> +	if (skb_vlan_tag_present(skb)) {
>> +		struct vlan_hdr *vhdr;
>> +
>> +		if (skb_cow_head(skb, VLAN_HLEN))
>> +			return -1;
>> +
>> +		__skb_push(skb, VLAN_HLEN);
>> +		skb_reset_network_header(skb);
>> +
>> +		vhdr = (struct vlan_hdr *)(skb->data);
>> +		vhdr->h_vlan_TCI = htons(id);
>> +		vhdr->h_vlan_encapsulated_proto = skb->protocol;
>> +		skb->protocol = proto;
>> +	} else {
>> +		__vlan_hwaccel_put_tag(skb, proto, id);
>> +	}
> 
> I did not apply this because I'm not sure if this preserves correct tag
> order.  Can you clarify?
> 
> Lets consider vlan-offload-doesn't-exist case.
> 
> First loop pushes vlan tag 1, we get:
> 
>   [vlan1][inet]

Always !skb_vlan_tag_present (all vlan tags are pulled before the push
in the fastpath), so vlan1 is always stored in skb->vlan_all.

> 
> 2nd items pushes vlan tag 2, we get:
>   [vlan2][vlan1][inet]
> 

The second is pushed directly [vlan2][inet]. At a later moment the
contents of skb->vlan_all is pushed by SW. Ending up with
[vlan1][vlan2][inet].

> Now lets consider with-offload.  We have one tag only, so we get 1 skb with hwaccel
> tag in the sk_buff.  This is fine, HW will insert it for us.
> 
> But now lets consider two tags:
> 
> First loop pushes vlan1, we get the vlan1 tag in sk_buff vlan info.
> Packet is: [inet].

Correct, so same as SW, vlan1 in skb->vlan_all.

> 
> 2nd loop pushes vlan2, we get:
> 	[vlan2][inet].
> 

Correct, [vlan2][inet].

> Now, when packet is transmitted, where will the HW insert the tag?
> 
> [vlan1][vlan2][inet]?
> Or will this be [vlan2][vlan1][inet]?

The contents of skb->vlan_all is pushed by HW. Ending up with
[vlan1][vlan2][inet], same as SW.

Hope this is enough to clarify, but you can double-check it looking at
nf_flow_tuple_encap(). With 2 vlan tags, the contents of skb->vlan_all,
is always vlan1, the first in the encap array, which is the outer tag.

> reading though the SW-side (no hw support), it seems it will do the right
> thing (i.e.. the hw tag gets added as the inner tag, right before inet
> and not added as outermost tag.
> 
> Can you confirm thats the case?
> 
> Sorry for not responding sooner, there are lots of patches atm and
> I forgot about this one when I yanked it off the previous pull request
> at the last second.

Re: [PATCH v2 nf] netfilter: nf_flow_table_ip: Introduce nf_flow_vlan_push()

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 3099 bytes --]

On Tue, Mar 10, 2026 at 09:25:46AM +0100, Eric Woudstra wrote:
> 
> 
> On 3/9/26 10:22 PM, Florian Westphal wrote:
> > Eric Woudstra <ericwouds@gmail.com> wrote:
> > 
> > Hi Eric
> > 
> >> With double vlan tagged packets in the fastpath, getting the error:
> >>
> >> skb_vlan_push got skb with skb->data not at mac header (offset 18)
> >>
> >> Introduce nf_flow_vlan_push(), that can correctly push the inner vlan
> >> in the fastpath. It is closedly modelled on existing nf_flow_pppoe_push()
> >>
> >> Fixes: c653d5a78f34 ("netfilter: flowtable: inline vlan encapsulation in xmit path")
> >> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
> >   
> >> +static int nf_flow_vlan_push(struct sk_buff *skb, __be16 proto, u16 id)
> >> +{
> >> +	if (skb_vlan_tag_present(skb)) {
> >> +		struct vlan_hdr *vhdr;
> >> +
> >> +		if (skb_cow_head(skb, VLAN_HLEN))
> >> +			return -1;
> >> +
> >> +		__skb_push(skb, VLAN_HLEN);
> >> +		skb_reset_network_header(skb);
> >> +
> >> +		vhdr = (struct vlan_hdr *)(skb->data);
> >> +		vhdr->h_vlan_TCI = htons(id);
> >> +		vhdr->h_vlan_encapsulated_proto = skb->protocol;
> >> +		skb->protocol = proto;
> >> +	} else {
> >> +		__vlan_hwaccel_put_tag(skb, proto, id);
> >> +	}
> > 
> > I did not apply this because I'm not sure if this preserves correct tag
> > order.  Can you clarify?
> > 
> > Lets consider vlan-offload-doesn't-exist case.
> > 
> > First loop pushes vlan tag 1, we get:
> > 
> >   [vlan1][inet]
> 
> Always !skb_vlan_tag_present (all vlan tags are pulled before the push
> in the fastpath), so vlan1 is always stored in skb->vlan_all.
> 
> > 
> > 2nd items pushes vlan tag 2, we get:
> >   [vlan2][vlan1][inet]
> > 
> 
> The second is pushed directly [vlan2][inet]. At a later moment the
> contents of skb->vlan_all is pushed by SW. Ending up with
> [vlan1][vlan2][inet].
> 
> > Now lets consider with-offload.  We have one tag only, so we get 1 skb with hwaccel
> > tag in the sk_buff.  This is fine, HW will insert it for us.
> > 
> > But now lets consider two tags:
> > 
> > First loop pushes vlan1, we get the vlan1 tag in sk_buff vlan info.
> > Packet is: [inet].
> 
> Correct, so same as SW, vlan1 in skb->vlan_all.
> 
> > 
> > 2nd loop pushes vlan2, we get:
> > 	[vlan2][inet].
> > 
> 
> Correct, [vlan2][inet].
> 
> > Now, when packet is transmitted, where will the HW insert the tag?
> > 
> > [vlan1][vlan2][inet]?
> > Or will this be [vlan2][vlan1][inet]?
> 
> The contents of skb->vlan_all is pushed by HW. Ending up with
> [vlan1][vlan2][inet], same as SW.
> 
> Hope this is enough to clarify, but you can double-check it looking at
> nf_flow_tuple_encap(). With 2 vlan tags, the contents of skb->vlan_all,
> is always vlan1, the first in the encap array, which is the outer tag.

In your patch, if vlan offload is present, then this outer (second)
VLAN header is pushed with the outer vlan ID.

This goes against the assumption that the outer VLAN header is always
offloaded.

Probably it is easier to fix this by resetting the mac header (see
your patch).

But if you prefer, just fix your patch to be similar to
skb_vlan_push().

[-- Attachment #2: fix.patch --]
[-- Type: text/x-diff, Size: 523 bytes --]

diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index 3fdb10d9bf7f..ab58ba0c5ff6 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -738,6 +738,9 @@ static int nf_flow_encap_push(struct sk_buff *skb,
 		switch (tuple->encap[i].proto) {
 		case htons(ETH_P_8021Q):
 		case htons(ETH_P_8021AD):
+			skb_reset_mac_header(skb);
+			skb_reset_mac_len(skb);
+
 			if (skb_vlan_push(skb, tuple->encap[i].proto,
 					  tuple->encap[i].id) < 0)
 				return -1;

[PATCH v2 nf] netfilter: nf_flow_table_ip: Introduce nf_flow_vlan_push()

[Eric Woudstra]

Calling skb_reset_mac_header() before calling skb_vlan_push() does
remove the error:

"skb_vlan_push got skb with skb->data not at mac header (offset 18)"

But the inner vlan tag is still not inserted correctly.

skb_vlan_push() uses __vlan_insert_inner_tag() to insert the tag
at offset ETH_HLEN. But the inner tag should only be pushed, without
offset, similar to nf_flow_pppoe_push().

Fixes: c653d5a78f34 ("netfilter: flowtable: inline vlan encapsulation in xmit path")
Fixes: a3aca98aec9a ("netfilter: nf_flow_table_ip: reset mac header before vlan push")
Signed-off-by: Eric Woudstra <ericwouds@gmail.com>

---

 net/netfilter/nf_flow_table_ip.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index fd56d663cb5b..0086f8a1a0d6 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -544,6 +544,26 @@ static int nf_flow_offload_forward(struct nf_flowtable_ctx *ctx,
 	return 1;
 }
 
+static int nf_flow_vlan_push(struct sk_buff *skb, __be16 proto, u16 id)
+{
+	if (skb_vlan_tag_present(skb)) {
+		struct vlan_hdr *vhdr;
+
+		if (skb_cow_head(skb, VLAN_HLEN))
+			return -1;
+
+		__skb_push(skb, VLAN_HLEN);
+		skb_reset_network_header(skb);
+		vhdr = (struct vlan_hdr *)(skb->data);
+		vhdr->h_vlan_TCI = htons(id);
+		vhdr->h_vlan_encapsulated_proto = skb->protocol;
+		skb->protocol = proto;
+	} else {
+		__vlan_hwaccel_put_tag(skb, proto, id);
+	}
+	return 0;
+}
+
 static int nf_flow_pppoe_push(struct sk_buff *skb, u16 id)
 {
 	int data_len = skb->len + sizeof(__be16);
@@ -738,9 +758,8 @@ static int nf_flow_encap_push(struct sk_buff *skb,
 		switch (tuple->encap[i].proto) {
 		case htons(ETH_P_8021Q):
 		case htons(ETH_P_8021AD):
-			skb_reset_mac_header(skb);
-			if (skb_vlan_push(skb, tuple->encap[i].proto,
-					  tuple->encap[i].id) < 0)
+			if (nf_flow_vlan_push(skb, tuple->encap[i].proto,
+					      tuple->encap[i].id) < 0)
 				return -1;
 			break;
 		case htons(ETH_P_PPP_SES):
-- 
2.53.0

Re: [PATCH v2 nf] netfilter: nf_flow_table_ip: Introduce nf_flow_vlan_push()

[Pablo Neira Ayuso]

On Tue, Apr 14, 2026 at 01:21:20PM +0200, Eric Woudstra wrote:
> Calling skb_reset_mac_header() before calling skb_vlan_push() does
> remove the error:
> 
> "skb_vlan_push got skb with skb->data not at mac header (offset 18)"
> 
> But the inner vlan tag is still not inserted correctly.
> 
> skb_vlan_push() uses __vlan_insert_inner_tag() to insert the tag
> at offset ETH_HLEN. But the inner tag should only be pushed, without
> offset, similar to nf_flow_pppoe_push().

It is doubled-tagged-vlan that is broken, right? I observed this once
but I have been burdened into a few things.

> Fixes: c653d5a78f34 ("netfilter: flowtable: inline vlan encapsulation in xmit path")
> Fixes: a3aca98aec9a ("netfilter: nf_flow_table_ip: reset mac header before vlan push")
> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
> 
> ---
> 
>  net/netfilter/nf_flow_table_ip.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index fd56d663cb5b..0086f8a1a0d6 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -544,6 +544,26 @@ static int nf_flow_offload_forward(struct nf_flowtable_ctx *ctx,
>  	return 1;
>  }
>  
> +static int nf_flow_vlan_push(struct sk_buff *skb, __be16 proto, u16 id)
> +{
> +	if (skb_vlan_tag_present(skb)) {
> +		struct vlan_hdr *vhdr;
> +
> +		if (skb_cow_head(skb, VLAN_HLEN))
> +			return -1;
> +
> +		__skb_push(skb, VLAN_HLEN);
> +		skb_reset_network_header(skb);
> +		vhdr = (struct vlan_hdr *)(skb->data);
> +		vhdr->h_vlan_TCI = htons(id);
> +		vhdr->h_vlan_encapsulated_proto = skb->protocol;
> +		skb->protocol = proto;
> +	} else {
> +		__vlan_hwaccel_put_tag(skb, proto, id);
> +	}
> +	return 0;
> +}
> +
>  static int nf_flow_pppoe_push(struct sk_buff *skb, u16 id)
>  {
>  	int data_len = skb->len + sizeof(__be16);
> @@ -738,9 +758,8 @@ static int nf_flow_encap_push(struct sk_buff *skb,
>  		switch (tuple->encap[i].proto) {
>  		case htons(ETH_P_8021Q):
>  		case htons(ETH_P_8021AD):
> -			skb_reset_mac_header(skb);
> -			if (skb_vlan_push(skb, tuple->encap[i].proto,
> -					  tuple->encap[i].id) < 0)
> +			if (nf_flow_vlan_push(skb, tuple->encap[i].proto,
> +					      tuple->encap[i].id) < 0)
>  				return -1;
>  			break;
>  		case htons(ETH_P_PPP_SES):
> -- 
> 2.53.0
> 

Re: [PATCH v2 nf] netfilter: nf_flow_table_ip: Introduce nf_flow_vlan_push()

[Eric Woudstra]

On 4/14/26 1:38 PM, Pablo Neira Ayuso wrote:
> On Tue, Apr 14, 2026 at 01:21:20PM +0200, Eric Woudstra wrote:
>> Calling skb_reset_mac_header() before calling skb_vlan_push() does
>> remove the error:
>>
>> "skb_vlan_push got skb with skb->data not at mac header (offset 18)"
>>
>> But the inner vlan tag is still not inserted correctly.
>>
>> skb_vlan_push() uses __vlan_insert_inner_tag() to insert the tag
>> at offset ETH_HLEN. But the inner tag should only be pushed, without
>> offset, similar to nf_flow_pppoe_push().
> 
> It is doubled-tagged-vlan that is broken, right? I observed this once
> but I have been burdened into a few things.

That is correct, both q-in-q and q-in-ad (that may not be the correct
terms, but I think it is clear).

>> Fixes: c653d5a78f34 ("netfilter: flowtable: inline vlan encapsulation in xmit path")
>> Fixes: a3aca98aec9a ("netfilter: nf_flow_table_ip: reset mac header before vlan push")
>> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
>>
>> ---
>>
>>  net/netfilter/nf_flow_table_ip.c | 25 ++++++++++++++++++++++---
>>  1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
>> index fd56d663cb5b..0086f8a1a0d6 100644
>> --- a/net/netfilter/nf_flow_table_ip.c
>> +++ b/net/netfilter/nf_flow_table_ip.c
>> @@ -544,6 +544,26 @@ static int nf_flow_offload_forward(struct nf_flowtable_ctx *ctx,
>>  	return 1;
>>  }
>>  
>> +static int nf_flow_vlan_push(struct sk_buff *skb, __be16 proto, u16 id)
>> +{
>> +	if (skb_vlan_tag_present(skb)) {
>> +		struct vlan_hdr *vhdr;
>> +
>> +		if (skb_cow_head(skb, VLAN_HLEN))
>> +			return -1;
>> +
>> +		__skb_push(skb, VLAN_HLEN);
>> +		skb_reset_network_header(skb);
>> +		vhdr = (struct vlan_hdr *)(skb->data);
>> +		vhdr->h_vlan_TCI = htons(id);
>> +		vhdr->h_vlan_encapsulated_proto = skb->protocol;
>> +		skb->protocol = proto;
>> +	} else {
>> +		__vlan_hwaccel_put_tag(skb, proto, id);
>> +	}
>> +	return 0;
>> +}
>> +
>>  static int nf_flow_pppoe_push(struct sk_buff *skb, u16 id)
>>  {
>>  	int data_len = skb->len + sizeof(__be16);
>> @@ -738,9 +758,8 @@ static int nf_flow_encap_push(struct sk_buff *skb,
>>  		switch (tuple->encap[i].proto) {
>>  		case htons(ETH_P_8021Q):
>>  		case htons(ETH_P_8021AD):
>> -			skb_reset_mac_header(skb);
>> -			if (skb_vlan_push(skb, tuple->encap[i].proto,
>> -					  tuple->encap[i].id) < 0)
>> +			if (nf_flow_vlan_push(skb, tuple->encap[i].proto,
>> +					      tuple->encap[i].id) < 0)
>>  				return -1;
>>  			break;
>>  		case htons(ETH_P_PPP_SES):
>> -- 
>> 2.53.0
>>