Re: [PATCH net-next 03/12] net: netfilter: add ether_type to net_device_path_ctx and use it

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

Hi Jakub,

On Tue, Aug 11, 2026 at 06:34:43PM -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.
> ---
> net: netfilter: add ether_type to net_device_path_ctx and use it
> 
> This adds an ether_type field to struct net_device_path_ctx and makes the
> ipip and ip6tnl ndo_fill_forward_path callbacks reject a context whose
> ether_type does not match their own address family.  The netfilter
> flowtable path builder fills the new field from pkt->ethertype.
> 
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 17d28adb029bb..2327a2703b835 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -941,6 +941,7 @@ struct net_device_path_stack {
> >  struct net_device_path_ctx {
> >  	const struct net_device *dev;
> >  	u8			daddr[ETH_ALEN];
> > +	__be16			ether_type;
> >  
> >  	int			num_vlans;
> >  	struct {
> 
> [Severity: Medium]
> This new field becomes a mandatory input for the tunnel
> ndo_fill_forward_path callbacks, but only the netfilter context builder is
> updated here.  The two other in-tree constructors of the context still use
> designated initializers that set only .dev, so ether_type stays zero:
> 
> drivers/net/ethernet/mediatek/mtk_ppe_offload.c:mtk_flow_get_wdma_info() {
> 	struct net_device_path_ctx ctx = {
> 		.dev = dev,
> 	};
> 	...
> }
> 
> drivers/net/ethernet/airoha/airoha_ppe.c:airoha_ppe_get_wdma_info() {
> 	struct net_device_path_ctx ctx = {
> 		.dev = dev,
> 	};
> 	...
> }
> 
> Zero matches neither ETH_P_IP nor ETH_P_IPV6, and dev_fill_forward_path()
> turns any negative return from the callback into a full abort:
> 
> net/core/dev.c:dev_fill_forward_path() {
> 	...
> 		ret = ctx->dev->netdev_ops->ndo_fill_forward_path(ctx, path);
> 		if (ret < 0)
> 			goto err_out;
> 	...
> }
> 
> So does an ipip or ip6tnl device anywhere in the walk now always fail path
> resolution for those two drivers, even for the same-family case that used
> to work, leaving the PPE/WED entry unprogrammed?  Both drivers know the
> flow's L3 protocol at that point, so could they populate ether_type as
> well?
> 
> Would it also help to document in the struct that 0 means "unknown" and is
> treated as unsupported by the tunnel callbacks?

This is fine, Lorenzo Bianconi will follow up with ip6ip and ipip6
support for this soon.

> > diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
> > index fb7d96f99b06b..62a374079bfcb 100644
> > --- a/net/ipv4/ipip.c
> > +++ b/net/ipv4/ipip.c
> > @@ -360,6 +360,9 @@ static int ipip_fill_forward_path(struct net_device_path_ctx *ctx,
> >  	const struct iphdr *tiph = &tunnel->parms.iph;
> >  	struct rtable *rt;
> >  
> > +	if (ctx->ether_type != cpu_to_be16(ETH_P_IP))
> > +		return -EOPNOTSUPP;
> > +
> >  	if (tunnel->collect_md)
> >  		return -EOPNOTSUPP;
> >  
> 
> [Severity: Low]
> The commit message describes a datapath defect ("incorrect dst_entry family
> can be reached from datapath"), but there is no Fixes: tag.  Without this
> gate, an IPv6 flow routed out of an ipip device makes
> ipip_fill_forward_path() succeed and return an IPv4 rtable in
> path->tun.dst, after which nft_dev_path_info() reads the v6 members of the
> address union and nft_dev_forward_path() installs that dst:
> 
> net/netfilter/nf_flow_table_path.c:nft_dev_forward_path() {
> 	...
> 		dst_release(route->tuple[dir].dst);
> 		route->tuple[dir].dst = info.tun_dst;
> 	...
> }
> 
> The two handlers were added by ab427db17885 ("netfilter: flowtable: Add
> IPIP rx sw acceleration") and d98103575dcd, and the tunnel dst hand-off was
> last touched by 806273fcaffb82 ("netfilter: flowtable: release tunnel route
> on error when building forward path").  Could a Fixes: tag against one of
> these be added so a backport can be keyed off it?

I can request this patch to go to -stable, not a big deal.

> It would also be worth noting in the message that this is not
> standalone-backportable: it depends on the preceding commit 635bf8763d7e
> ("net: pass net_device_path_ctx to dev_fill_forward_path()"), since before
> that the context was built inside net/core/dev.c and the netfilter caller
> had no way to pass an ethertype.
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the new gate only validates
> the packet's L3 protocol and still leaves the tunnel device's configured
> payload protocol unchecked.  An ipip link can be configured with
> iph.protocol == IPPROTO_MPLS (mplsip), and such a device shares
> ipip_netdev_ops and therefore this callback, yet the callback never consults
> tunnel->parms.iph.protocol and unconditionally sets
> path->tun.inner_proto = IPPROTO_IPIP.
> 
> The device's own paths do enforce it:
> 
> net/ipv4/ipip.c:ipip_tunnel_xmit() {
> 	...
> 	if (tiph->protocol != ipproto && tiph->protocol != 0)
> 		goto tx_error;
> 	...
> }
> 
> net/ipv4/ipip.c:ipip_rcv() {
> 	...
> 		if (tunnel->parms.iph.protocol != ipproto &&
> 		    tunnel->parms.iph.protocol != 0)
> 			goto drop;
> 	...
> }
> 
> The offload path then writes the hardcoded value itself:
> 
> net/netfilter/nf_flow_table_ip.c:nf_flow_tunnel_ipip_push() {
> 	...
> 	iph->protocol	= tuple->tun.l3_proto;
> 	...
> }
> 
> So for an mplsip ipip device, does the slow path drop the first packet
> while every subsequent packet of the offloaded flow is encapsulated (and
> matching ingress packets decapsulated) with a protocol number that
> contradicts the device configuration?  Should the gate also compare against
> tunnel->parms.iph.protocol?
> 
> > diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> > index 042d743edb6c0..d063add01f523 100644
> > --- a/net/ipv6/ip6_tunnel.c
> > +++ b/net/ipv6/ip6_tunnel.c
> > @@ -1852,6 +1852,9 @@ static int ip6_tnl_fill_forward_path(struct net_device_path_ctx *ctx,
> >  	struct flowi6 fl6;
> >  	int err;
> >  
> > +	if (ctx->ether_type != cpu_to_be16(ETH_P_IPV6))
> > +		return -EOPNOTSUPP;
> > +
> >  	if (t->parms.flags & (IP6_TNL_F_USE_ORIG_TCLASS |
> >  			      IP6_TNL_F_USE_ORIG_FLOWLABEL |
> >  			      IP6_TNL_F_USE_ORIG_FWMARK))
> 
> [Severity: Medium]
> Same pre-existing observation on the IPv6 side, not something this patch
> introduces: t->parms.proto is ignored while path->tun.inner_proto is always
> set to IPPROTO_IPV6, whereas the software paths reject a mismatch:
> 
> net/ipv6/ip6_tunnel.c:ipxip6_tnl_xmit() {
> 	...
> 	tproto = READ_ONCE(t->parms.proto);
> 	if (tproto != protocol && tproto != 0)
> 		return -1;
> 	...
> }
> 
> ip6_tnl_rcv() applies the same check on receive.  For an ip6tnl in ip4ip6
> mode, does the offload path end up forwarding traffic that the tunnel
> device itself would drop?

As said I plan to request this patch to get into -stable to address
this too.

> > diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
> > index 0cbde535b8bae..5f166da3b09be 100644
> > --- a/net/netfilter/nf_flow_table_path.c
> > +++ b/net/netfilter/nf_flow_table_path.c
> > @@ -44,13 +44,15 @@ static bool nft_is_valid_ether_device(const struct net_device *dev)
> >  
> >  static int nft_dev_fill_forward_path(const struct dst_entry *dst_cache,
> >  				     const struct nf_conn *ct,
> > -				     enum ip_conntrack_dir dir, u8 *ha,
> > +				     enum ip_conntrack_dir dir,
> > +				     u8 *ha, __be16 ether_type,
> >  				     struct net_device_path_stack *stack)
> >  {
> >  	const void *daddr = &ct->tuplehash[!dir].tuple.src.u3;
> >  	struct net_device *dev = dst_cache->dev;
> >  	struct net_device_path_ctx ctx = {
> >  		.dev = dev,
> > +		.ether_type = ether_type,
> >  	};
> >  	struct neighbour *n;
> >  	u8 nud_state;
> 
> [ ... ]
> 
> > @@ -228,7 +230,7 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
> >  	unsigned char ha[ETH_ALEN];
> >  	int i;
> >  
> > -	if (nft_dev_fill_forward_path(dst, ct, dir, ha, &stack) < 0 ||
> > +	if (nft_dev_fill_forward_path(dst, ct, dir, ha, pkt->ethertype, &stack) < 0 ||
> >  	    nft_dev_path_info(&stack, &info, ha, ft) < 0)
> >  		return -ENOENT;
> >