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

On 2/22/26 6:27 PM, Florian Westphal wrote:

Eric Woudstra [off-list ref] wrote:

quoted

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.

quoted

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