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

[Jakub Kicinski <kuba@kernel.org>]

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?

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

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?

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