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

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Thu, Mar 05, 2026 at 10:05:15AM +0100, Florian Westphal wrote:
> 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:

This used to work, I just accidentally broke it when using
skb_vlan_push() in net-next.

I will post fix.

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

Could you elaborate more precisely?

Thanks.