Re: [PATCH net-next 7/9] netfilter: flowtable: release tunnel route on error when building forward path

[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.
---
netfilter: flowtable: release tunnel route on error when building forward path

This drops the second route lookup in nft_flow_tunnel_update_route() and
instead passes the dst_entry obtained by dev_fill_forward_path() through
the tunnel forward path information, so ipip and ip6ip6 no longer release
it.  On failure while building the forward path the tunnel dst is released
via dev_fill_forward_path_release().

> diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
> index 0831f6b81717c..fb7d96f99b06b 100644
> --- a/net/ipv4/ipip.c
> +++ b/net/ipv4/ipip.c
> @@ -376,10 +376,10 @@ static int ipip_fill_forward_path(struct net_device_path_ctx *ctx,
>  	path->tun.src_v4.s_addr = tiph->saddr;
>  	path->tun.dst_v4.s_addr = tiph->daddr;
>  	path->tun.l3_proto = IPPROTO_IPIP;
> +	path->tun.dst = &rt->dst;
>  	path->dev = ctx->dev;
>  
>  	ctx->dev = rt->dst.dev;
> -	ip_rt_put(rt);
>  
>  	return 0;
>  }
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index 97c3f61d627bb..d80020bc2620e 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1870,12 +1870,14 @@ static int ip6_tnl_fill_forward_path(struct net_device_path_ctx *ctx,
>  		path->tun.src_v6 = fl6.saddr;
>  		path->tun.dst_v6 = fl6.daddr;
>  		path->tun.l3_proto = IPPROTO_IPV6;
> +		path->tun.dst = dst;
>  		path->dev = ctx->dev;
>  		ctx->dev = dst->dev;
>  	}
>  
>  	err = dst->error;
> -	dst_release(dst);
> +	if (err)
> +		dst_release(dst);
>  
>  	return err;
>  }
> diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
> index 004dc75ac357a..56219b02e1227 100644
> --- a/net/netfilter/nf_flow_table_path.c
> +++ b/net/netfilter/nf_flow_table_path.c

[ ... ]

> @@ -205,44 +211,6 @@ static bool nft_flowtable_find_dev(const struct net_device *dev,
>  	return found;
>  }
>  
> -static int nft_flow_tunnel_update_route(const struct nft_pktinfo *pkt,
> -					struct flow_offload_tunnel *tun,
> -					struct nf_flow_route *route,
> -					enum ip_conntrack_dir dir)
> -{
> -	struct dst_entry *cur_dst = route->tuple[dir].dst;
> -	struct dst_entry *tun_dst = NULL;
> -	struct flowi fl = {};
> -
> -	switch (nft_pf(pkt)) {

[ ... ]

> -	nf_route(nft_net(pkt), &tun_dst, &fl, false, nft_pf(pkt));
> -	if (!tun_dst)
> -		return -ENOENT;
> -
> -	route->tuple[dir].dst = tun_dst;
> -	dst_release(cur_dst);
> -
> -	return 0;
> -}
> -
>  static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
>  				struct nf_flow_route *route,
>  				const struct nf_conn *ct,
> @@ -267,12 +235,13 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
>  		route->tuple[!dir].in.encap[i].proto = info.encap[i].proto;
>  	}
>  
> -	if (info.num_tuns &&
> -	    !nft_flow_tunnel_update_route(pkt, &info.tun, route, dir)) {
> +	if (info.num_tuns) {
>  		route->tuple[!dir].in.tun.src_v6 = info.tun.dst_v6;
>  		route->tuple[!dir].in.tun.dst_v6 = info.tun.src_v6;
>  		route->tuple[!dir].in.tun.l3_proto = info.tun.l3_proto;
>  		route->tuple[!dir].in.num_tuns = info.num_tuns;
> +		dst_release(route->tuple[dir].dst);
> +		route->tuple[dir].dst = info.tun_dst;
>  	}

[Severity: High]
Can the dst installed here end up belonging to a different address family
than the flow it is attached to?

The removed nft_flow_tunnel_update_route() derived the replacement dst from
nf_route(nft_net(pkt), &tun_dst, &fl, false, nft_pf(pkt)), so the dst family
always matched the flow's l3proto.  The dst that now arrives in
info.tun_dst follows the tunnel underlay instead:

net/ipv4/ipip.c:ipip_fill_forward_path() always stores an IPv4 rtable:

	path->tun.l3_proto = IPPROTO_IPIP;
	path->tun.dst = &rt->dst;

net/ipv6/ip6_tunnel.c:ip6_tnl_fill_forward_path() always stores an IPv6
rt6_info:

	path->tun.l3_proto = IPPROTO_IPV6;
	path->tun.dst = dst;

Neither handler consults the tunnel payload proto, and neither
nft_dev_path_info() nor nft_dev_forward_path() checks the family before
storing it in route->tuple[dir].dst.

The consumers cast that dst according to the inner flow family.  In
net/netfilter/nf_flow_table_ip.c:nf_flow_offload_ipv6_hook():

	rt = dst_rt6_info(tuplehash->tuple.dst_cache);
	...
	neigh = ip_neigh_gw6(rt->dst.dev, rt6_nexthop(rt, ip6_daddr));

and in nf_flow_offload_ip_hook():

	rt = dst_rtable(tuplehash->tuple.dst_cache);
	... rt_nexthop(rt, ip_daddr) ...

net/netfilter/nf_flow_table_core.c:flow_offload_dst_cookie() does the same:

	if (flow_tuple->l3proto == NFPROTO_IPV6)
		return rt6_get_cookie(dst_rt6_info(flow_tuple->dst_cache));

For an ip6_tunnel in ip4ip6 mode (t->parms.proto == IPPROTO_IPIP) carrying
IPv4 flows, ip6_tnl_fill_forward_path() still reports l3_proto =
IPPROTO_IPV6 and hands out an rt6_info, which nf_flow_offload_ip_hook()
then reads through dst_rtable() and rt_nexthop()/ip_mtu_locked().  That
configuration needs no misconfiguration to reach.

The mirror case is an IPv6 flow routed out an ipip device: an ipip device
with mtu 1480 gets an inet6_dev at NETDEV_REGISTER, so fib6_nh_init()
accepts "ip -6 route add ... dev tunlX", and nft_dev_fill_forward_path()
deliberately skips the ether-device requirement for tunnels, so
ipip_fill_forward_path() hands back an rtable that becomes the dst_cache of
an IPv6 flow.  rt6_nexthop() then reads rt6i_flags and rt6i_gateway past
the end of a struct rtable allocated from the ip_dst_cache slab, and the
result is used as the IPv6 next hop passed to ip_neigh_gw6().

nf_flow_dst_check() does not catch this either: dst->ops->check() for the
stored rtable is ipv4_dst_check(), which only tests rt_is_expired().

Would it make sense to validate info.tun.l3_proto (or the dst's
ops->family) against the flow's l3proto before installing info.tun_dst?

[Severity: Low]
Should this carry a Fixes: tag?

Before this change the tunnel information was only installed when the
second lookup succeeded:

	if (info.num_tuns &&
	    !nft_flow_tunnel_update_route(pkt, &info.tun, route, dir)) {

while route->tuple[!dir].in.ifindex, route->tuple[dir].out.ifindex, the
encaps, h_source/h_dest and xmit_type had already been written and
nft_dev_forward_path() still returned 0.  On nf_route() failure the flow was
offloaded with the underlay device as output but num_tuns == 0, so inner
packets left the underlay without IPIP/IP6IP6 encapsulation, which matches
the "incomplete forward path" described in the commit message.

git blame points that construct at d30301ba4b07ac ("netfilter: flowtable:
tunnel route update"), with the tunnel forward path support itself from
ab427db1788581, so the bug is in released kernels rather than in-flight
code from this series.  A Fixes: d30301ba4b07ac tag would help stable
tracking.

It may also be worth noting in the changelog that this is not standalone
backportable: neither dev_fill_forward_path_release() nor the
net_device_path tun.dst field exist before the earlier "net: pass dst via
net_device_path in dev_fill_forward_path()" commit in this series.