Re: [PATCH net-next v2 2/3] ipv6: add ip6_dst_lookup_flow_cookie

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The stale-cookie race that this patch describes as a general
  flaw of 'callers that keep a dst in a persistent cache' is still…
- [Medium] The gen0/gen1 check in ip6_dst_lookup_flow_cookie() only
  notices a FIB change if the writer updates routing state before…

--

ipv6: add ip6_dst_lookup_flow_cookie

This commit adds ip6_dst_lookup_flow_cookie(). It returns the dst
together with a validation cookie that is sampled between two reads of
the IPv6 generation, and the cookie is 0 if the generation moved during
the lookup. For nexthop-object dsts, ip6_dst_check() now also requires
the cookie to match rt->sernum.

> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 25fe0ba98b1a6..1b273714f7cc3 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c

[ ... ]

> @@ -1300,6 +1299,54 @@ struct dst_entry *ip6_dst_lookup_flow(struct net *net, const struct sock *sk, st
>  }
>  EXPORT_SYMBOL_GPL(ip6_dst_lookup_flow);
>  
> +/**
> + *	ip6_dst_lookup_flow_cookie - perform route lookup on flow with ipsec
> + *				     and return the matching dst cookie

[ ... ]

> + *	Same as ip6_dst_lookup_flow, but also returns the cookie describing
> + *	the routing state the dst was obtained from. A caller caching the dst
> + *	should use this cookie when validating it with dst_check. If the
> + *	IPv6 FIB changed during the lookup, a route dst no longer validates
> + *	with it. An xfrm bundle is validated by xfrm and ignores the cookie.

[Severity: High]
The commit message describes the stale cookie as a problem for "Callers
that keep a dst in a persistent cache", and names an xfrm policy
insertion as one trigger. This patch converts no callers.

At the end of the series, only tcp_v6_connect(), ip6_datagram_dst_update()
and ip6_sk_dst_lookup_flow() are converted, by "ipv6: use the lookup
cookie when caching the dst in sockets".

Are the remaining persistent caches still exposed to the same race? The
TCP re-route path, for example, still does this:

net/ipv6/inet6_connection_sock.c:inet6_csk_route_socket() {
    ...
	dst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_p);

	if (!IS_ERR(dst))
		ip6_dst_store(sk, dst, false, false);
    ...
}

ip6_dst_store() reads rt6_get_cookie() only after the lookup has
returned. This path runs from inet6_csk_xmit() whenever __sk_dst_check()
fails, and that happens right after a rt_genid_bump_ipv6().

tcp_v6_syn_recv_sock() has the same pattern for accepted sockets. Its
window runs from inet6_csk_route_req() to ip6_dst_store().
inet6_sk_rebuild_header() has the same pattern too.

dst_cache_set_ip6() computes the cookie itself:

net/core/dst_cache.c:dst_cache_set_ip6() {
    ...
	dst_cache_per_cpu_dst_set(idst, dst,
				  rt6_get_cookie(dst_rt6_info(dst)));
    ...
}

It has no parameter for a cookie from the caller. Without a new dst_cache
API, these users can't adopt the new helper:

- udp_tunnel6_dst_lookup() (vxlan, geneve, bareudp)
- ip6_tunnel
- wireguard
- ovpn
- the seg6, rpl, ioam6 and ila lwtunnels
- tipc

Suppose this sequence lands between ip6_dst_lookup_flow() and
rt6_get_cookie() in any of these paths:

xfrm_policy_insert()->rt_genid_bump_ipv6()->__fib6_flush_trees()

The plain route would then be cached with the post-insertion serial and
keep passing rt6_check().

Wouldn't that socket or tunnel keep sending without the new IPsec policy
until the next FIB change? That is the plaintext leak this series sets
out to close.

> + *
> + *	Return: a valid dst pointer on success or a pointer encoded error code.
> + */
> +struct dst_entry *ip6_dst_lookup_flow_cookie(struct net *net,
> +					     const struct sock *sk,
> +					     struct flowi6 *fl6,
> +					     const struct in6_addr *final_dst,
> +					     u32 *cookie)
> +{
> +	const struct rt6_info *rt;
> +	struct dst_entry *dst;
> +	int gen0, gen1;
> +
> +	gen0 = rt_genid_ipv6(net);
> +	/* read the generation before looking at the FIB or xfrm policies */
> +	smp_rmb();
> +
> +	dst = ip6_dst_lookup_flow(net, sk, fl6, final_dst);
> +	if (IS_ERR(dst))
> +		return dst;
> +
> +	rt = dst_rt6_info(dst);
> +	*cookie = rt->sernum ? gen0 : rt6_get_cookie(rt);
> +
> +	/* keep the FIB and cookie reads before the final generation read */
> +	smp_rmb();
> +	gen1 = rt_genid_ipv6(net);
> +	if (unlikely(gen0 != gen1))
> +		*cookie = 0;

[Severity: Medium]
Can the gen0/gen1 check catch writers that bump the serial before they
finish updating the routing state?

fib6_ifup() clears the nh flags, bumps the serial, and only then
rebalances:

net/ipv6/route.c:fib6_ifup() {
    ...
		rt->fib6_nh->fib_nh_flags &= ~arg->nh_flags;
		fib6_update_sernum_upto_root(net, rt);
		rt6_multipath_rebalance(rt);
    ...
}

fib6_ifdown() uses the same order for NETDEV_DOWN on multipath routes,
and also for NETDEV_CHANGE:

net/ipv6/route.c:fib6_ifdown() {
    ...
			rt6_multipath_nh_flags_set(rt, dev, RTNH_F_DEAD |
						   RTNH_F_LINKDOWN);
			fib6_update_sernum(net, rt);
			rt6_multipath_rebalance(rt);
		}
		return -2;
    ...
}

The -2 return makes the walker skip the other siblings, so no second
bump follows the rebalance.

Suppose gen0 reads the new serial N+1 and fib6_select_path() runs before
rt6_multipath_rebalance(). The lookup then sees the old
fib_nh_upper_bound values. rt6_get_cookie() returns fn_sernum N+1 and
gen1 is also N+1, so the returned cookie is valid. rt6_check() keeps
accepting the dst after that.

rt6_score_route() doesn't check RTNH_F_DEAD or RTNH_F_LINKDOWN. Could
the cached dst then stay pinned to a nexthop that was just marked dead
or link-down, or to a stale weight split, until some unrelated FIB
change?

The writer ordering predates this patch. Still, the kdoc above says "If
the IPv6 FIB changed during the lookup, a route dst no longer validates
with it", and that doesn't seem to hold for these writers.

> +
> +	return dst;
> +}
> +EXPORT_SYMBOL_GPL(ip6_dst_lookup_flow_cookie);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794384146.git.ralf%40mandelbit.com