Pablo Neira Ayuso [off-list ref] wrote:

Hi Florian,

On Wed, Mar 04, 2026 at 06:29:36PM +0100, Florian Westphal wrote:

quoted

Hi,

The following patchset contains Netfilter fixes for *net*:

1) Fix a bug with vlan headers in the flowtable infrastructure.
   Existing code uses skb_vlan_push() helper, but that helper
   requires skb->data to point to the MAC header, which isn't the
   case for flowtables.  Switch to a new helper, modeled on the
   existing PPPoE helper. From Eric Woudstra. This bug was added
   in v6.19-rc1.

In patch 1/4, why is this new function so different wrt. skb_vlan_push?
 

I asked that to Eric when I reviewed this, and that was his reply:
--------------------------------------------------------------------
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.
--------------------------------------------------------------------

                skb_postpush_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
        }
        __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
 
 
In case there are two VLANs, the existing in hwaccel gets pushed into
the VLAN header, and the outer VLAN becomes the one that is offloaded?
 
Is this reversed in this patch? The first VLAN tag is offloaded, then
the next one coming is pushed as a VLAN header?

Yes, it looks broken.  I wonder why we have no tests for this stuff.
First a vlan push function that cannot have worked, ever, now this
seemingly reversing-headers variant:

For PPPOE, its pushing the ppppe header to packet, so we get
strict ordering, later header coming in the stack gets placed on
top, before older one.

Here, first vlan push gets placed into hw tag in skb (which makes
sense, let HW take care of it).

But if 2nd comes along, then that gets placed in the packet
and the hwaccel tag remains?

What to do?  Should be nuke vlan offload support from flowtable?
It appears to be an unused feature.

I have low confidence in this code.