Re: [PATCH net 0/4] netfilter: updates for net

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Florian,
> 
> On Wed, Mar 04, 2026 at 06:29:36PM +0100, Florian Westphal wrote:
> > 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.