[PATCH RFC v1 nf-next] netfilter: nf_flow_table_ip: Introduce nf_flow_vlan_push()

[PATCH RFC v1 nf-next] 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 push the inner vlan in the
fastpath.

Fixes: c653d5a78f34 ("netfilter: flowtable: inline vlan encapsulation in xmit path")
Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
---
 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 RFC v1 nf-next] netfilter: nf_flow_table_ip: Introduce nf_flow_vlan_push()

[Florian Westphal]

Eric Woudstra <ericwouds@gmail.com> wrote:
> 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 push the inner vlan in the
> fastpath.
> 
> Fixes: c653d5a78f34 ("netfilter: flowtable: inline vlan encapsulation in xmit path")

This change is in net/nf tree, so why are you targetting nf-next?
Are you proposing a revert for nf?  If so, please first send a revert
for nf.

Is there a test case for this that demonstrages the breakage?

And why is this tagged as RFC, what is the problem with this patch?

> +	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;

Ok, I see, this opencodes a variant of skb_vlan_push().
Would it be possible to correct skb->data so it points to the mac header
temporarily?  skb->data always points to network header so this cannot
have worked, ever.

Re: [PATCH RFC v1 nf-next] netfilter: nf_flow_table_ip: Introduce nf_flow_vlan_push()

[Eric Woudstra]

On 2/22/26 6:27 PM, Florian Westphal wrote:
> Eric Woudstra <ericwouds@gmail.com> wrote:
>> 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 push the inner vlan in the
>> fastpath.
>>
>> Fixes: c653d5a78f34 ("netfilter: flowtable: inline vlan encapsulation in xmit path")
> 
> This change is in net/nf tree, so why are you targetting nf-next?
> Are you proposing a revert for nf?  If so, please first send a revert
> for nf.
> 
> Is there a test case for this that demonstrages the breakage?
> 
> And why is this tagged as RFC, what is the problem with this patch?
> 

I have run into this, when testing my branch for implementing the
bridge-fastpath, not in the forward-fastpath. But anyway, no matter how
packets are handled in the original path (forwarding or bridged), once
going through the fastpath it would not matter, so it is broken in any
fastpath.

I have the complete testcase for bridge-fastpath here:

https://github.com/ericwoud/linux/commits/bpir-nftflow-nf-next

I do not have a ready to use testcase in the forward-fastpath, but is is
clearly broken. So this is why I started with an RFC first.

>> +	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;
> 
> Ok, I see, this opencodes a variant of skb_vlan_push().
> Would it be possible to correct skb->data so it points to the mac header
> temporarily?  skb->data always points to network header so this cannot
> have worked, ever.

The code here for the inner header is an almost exact copy of
nf_flow_pppoe_push(), which was also implemented at the same time.
So handling pppoe and inner-vlan header is implemented in the same
manner, which keeps it simple and uniform. If one functions
(in)correctly, then so would the other.

I've been implementing handling the inner vlan header like this for a
half year now. My version of nf_flow_encap_push() was a bit different,
but after this patch it is quite similar.

Re: [PATCH RFC v1 nf-next] netfilter: nf_flow_table_ip: Introduce nf_flow_vlan_push()

[Florian Westphal]

Eric Woudstra <ericwouds@gmail.com> wrote:
> I have run into this, when testing my branch for implementing the
> bridge-fastpath, not in the forward-fastpath. But anyway, no matter how
> packets are handled in the original path (forwarding or bridged), once
> going through the fastpath it would not matter, so it is broken in any
> fastpath.

Agree, it cannot work as-is.

> > Ok, I see, this opencodes a variant of skb_vlan_push().
> > Would it be possible to correct skb->data so it points to the mac header
> > temporarily?  skb->data always points to network header so this cannot
> > have worked, ever.
> 
> The code here for the inner header is an almost exact copy of
> nf_flow_pppoe_push(), which was also implemented at the same time.

Ah, I see.  Makes sense to me.

What aobut this:
Wait for a day or so to give others to provide feedback. If no more
comments, re-send this patch, targetting nf.git, and amend the commit
message to mention that the new function is closedly modelled on
existing nf_flow_pppoe_push().

Makes sense to you?