Re: [PATCH net-next 06/12] netfilter: flowtable: store ethertype in flowtable context

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

On Tue, Aug 11, 2026 at 06:34:44PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> netfilter: flowtable: store ethertype in flowtable context
> 
> This adds an ether_type field to struct nf_flowtable_ctx, sets it while
> parsing the layer 2 header in nf_flow_skb_encap_protocol(), and replaces
> the local inner_proto computations in nf_flow_tuple_encap() and
> nf_flow_skb_encap_protocol() with reads of ctx->ether_type.  The IPv4 and
> IPv6 lookup helpers now compare ctx->ether_type themselves instead of
> passing an expected protocol down.
> 
> No Link: tags in the commit header.
> 
> > diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> > index 7692ae7aa8539..3f417a43bd122 100644
> > --- a/net/netfilter/nf_flow_table_ip.c
> > +++ b/net/netfilter/nf_flow_table_ip.c
> > @@ -161,7 +162,6 @@ static void nf_flow_tuple_encap(struct nf_flowtable_ctx *ctx,
> >  				struct sk_buff *skb,
> >  				struct flow_offload_tuple *tuple)
> >  {
> > -	__be16 inner_proto = skb->protocol;
> >  	struct vlan_ethhdr *veth;
> >  	struct pppoe_hdr *phdr;
> >  	struct ipv6hdr *ip6h;
> > @@ -179,19 +179,17 @@ static void nf_flow_tuple_encap(struct nf_flowtable_ctx *ctx,
> >  		veth = (struct vlan_ethhdr *)skb_mac_header(skb);
> >  		tuple->encap[i].id = ntohs(veth->h_vlan_TCI);
> >  		tuple->encap[i].proto = skb->protocol;
> > -		inner_proto = veth->h_vlan_encapsulated_proto;
> >  		offset += VLAN_HLEN;
> >  		break;
> >  	case htons(ETH_P_PPP_SES):
> >  		phdr = (struct pppoe_hdr *)skb_network_header(skb);
> >  		tuple->encap[i].id = ntohs(phdr->sid);
> >  		tuple->encap[i].proto = skb->protocol;
> > -		inner_proto = *((__be16 *)(phdr + 1));
> >  		offset += PPPOE_SES_HLEN;
> >  		break;
> >  	}
> >  
> > -	switch (inner_proto) {
> > +	switch (ctx->ether_type) {
> >  	case htons(ETH_P_IP):
> >  		iph = (struct iphdr *)(skb_network_header(skb) + offset);
> >  		if (ctx->tun.inner_proto == IPPROTO_IPIP) {
> 
> [Severity: Medium]
> 
> Does this hunk change behaviour for PPPoE frames rather than just moving
> the ethertype into the context?
> 
> Before this patch, the PPPoE arm fed the raw PPP protocol id into the
> switch:
> 
> net/netfilter/nf_flow_table_ip.c:nf_flow_tuple_encap() {
> 	case htons(ETH_P_PPP_SES):
> 		...
> 		inner_proto = *((__be16 *)(phdr + 1));
> 		offset += PPPOE_SES_HLEN;
> 		break;
> 	}
> 
> 	switch (inner_proto) {
> 	case htons(ETH_P_IP):
> 	...
> }
> 
> PPP_IP is 0x0021 and PPP_IPV6 is 0x0057, so inner_proto could never equal
> htons(ETH_P_IP) (0x0800) or htons(ETH_P_IPV6) (0x86dd), and PPPoE frames
> always landed in the default: arm.  That left
> tuple->tun.{src_v4,dst_v4,src_v6,dst_v6} and tuple->tun.inner_proto zero
> even when nf_flow_skb_encap_protocol() -> nf_flow_ip4_tunnel_proto() had
> already set ctx->tun.inner_proto = IPPROTO_IPIP.
> 
> After the patch, ctx->ether_type for PPPoE is the value normalised by
> __nf_flow_pppoe_proto() in include/net/netfilter/nf_flow_table.h:
> 
> 	switch (proto) {
> 	case htons(PPP_IP):
> 		return htons(ETH_P_IP);
> 	case htons(PPP_IPV6):
> 		return htons(ETH_P_IPV6);
> 	}
> 
> so these tunnel arms now execute for PPPoE traffic and populate
> tuple->tun.
> 
> struct flow_offload_tunnel tun sits before the __hash marker in struct
> flow_offload_tuple, so it is part of the rhashtable lookup key, and the
> control path always installs non-zero endpoints for tunnel flows:
> 
> net/netfilter/nf_flow_table_core.c:flow_offload_fill_tuple() {
> 	flow_tuple->tun = route->tuple[dir].in.tun;
> 	flow_tuple->encap_num = route->tuple[dir].in.num_encaps;
> }
> 
> filled from DEV_PATH_TUN in nft_dev_path_info(), which can be walked
> together with DEV_PATH_PPPOE in the same path stack (ipip or ip6tnl over a
> PPPoE underlay).  That means PPPoE encapsulated IPIP/IP6IP6 rx flows
> previously missed their flowtable entry on every packet and now start
> matching.
> 
> Should the commit message mention this, and should a Fixes: tag be added
> pointing at ab427db17885 ("netfilter: flowtable: Add IPIP rx sw
> acceleration") and d98103575dcd ("netfilter: flowtable: Add IP6IP6 rx sw
> acceleration"), which introduced the raw PPP id versus ETH_P_* comparison?
> As written, the message only says "Store the ether_type in the context
> after parsing the layer 2 header for the first time and then use it later
> on", which reads as a pure cleanup.

I will make sure this reaches -stable at some point as this fixes ipip
and ip6ip6 over pppoe.

> The behaviour persists to the end of the series (nf_flow_tuple_encap() at
> 839e6caf13c7 still switches on ctx->ether_type), so no later patch
> documents or reverts it.
> 
> > @@ -389,22 +387,27 @@ static bool nf_flow_skb_encap_protocol(struct nf_flowtable_ctx *ctx,
> >  			return false;
> >  
> >  		veth = (struct vlan_ethhdr *)skb_mac_header(skb);
> > -		if (veth->h_vlan_encapsulated_proto == proto) {
> > -			ctx->offset += VLAN_HLEN;
> > -			inner_proto = proto;
> > -			ret = true;
> > -		}
> > +		ctx->ether_type = veth->h_vlan_encapsulated_proto;
> > +		ctx->offset += VLAN_HLEN;
> > +		ret = true;
> >  		break;
> >  	case htons(ETH_P_PPP_SES):
> > -		if (nf_flow_pppoe_proto(skb, &inner_proto) &&
> > -		    inner_proto == proto) {
> > -			ctx->offset += PPPOE_SES_HLEN;
> > -			ret = true;
> > -		}
> > +		if (!nf_flow_pppoe_proto(skb, &ether_type))
> > +			return false;
> > +
> > +		ctx->ether_type = ether_type;
> > +		ctx->offset += PPPOE_SES_HLEN;
> > +		ret = true;
> > +		break;
> 
> This is the store of the normalised ETH_P_* value that nf_flow_tuple_encap()
> later consumes for PPPoE frames.
> 
> [ ... ]