Re: [PATCH net-next 01/14] ipv4: annotate data-races around ip_rt_* sysctls

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In __ip_rt_update_pmtu() (net/ipv4/route.c), the patch adds two
  separate READ_ONCE() loads of net->ipv4.ip_rt_min_pmtu.

--

ipv4: annotate data-races around ip_rt_* sysctls

This adds READ_ONCE() to the lockless readers of several ip_rt_* route
sysctls in net/ipv4/route.c. Their sysctl handlers write them with
WRITE_ONCE(). The change was prompted by a KCSAN report on
net.ipv4.route.min_adv_mss in ipv4_default_advmss().

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index d0a5f6d77094c..83f49b2d4c36a 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c

[ ... ]

> @@ -1063,14 +1066,14 @@ static void __ip_rt_update_pmtu(struct rtable *rt, struct flowi4 *fl4, u32 mtu)
>  
>  	rcu_read_lock();
>  	net = dst_dev_net_rcu(dst);
> -	if (mtu < net->ipv4.ip_rt_min_pmtu) {
> +	if (mtu < READ_ONCE(net->ipv4.ip_rt_min_pmtu)) {
>  		lock = true;
> -		mtu = min(old_mtu, net->ipv4.ip_rt_min_pmtu);
> +		mtu = min(old_mtu, READ_ONCE(net->ipv4.ip_rt_min_pmtu));

[Severity: Low]
READ_ONCE() is volatile, so __ip_rt_update_pmtu() now always does two
separate loads of ip_rt_min_pmtu. If net.ipv4.route.min_pmtu is written
between them, can the check and the clamp see different values?

For example, take old_mtu=1500 and an ICMP FRAG_NEEDED carrying mtu=300:

  first load sees min_pmtu=552   -> lock = true
  sysctl write sets min_pmtu=68  (accepted, extra1 is ip_min_valid_pmtu)
  second load sees 68            -> mtu = min(1500, 68) = 68

update_or_create_fnhe() would then store pmtu=68 with fnhe_mtu_locked set,
and it stays that way for ip_rt_mtu_expires. The old setting would give
552 locked, and the new setting would give 300 unlocked.

Before this patch, the compiler could merge the two plain loads into one.
Only a privileged writer can trigger this, because min_pmtu is not exported
to netns owned by non-init user namespaces. Later patches in the series
don't seem to change this.

Could the value be read once into a local? This patch already does that
in ip_error() with burst and cost.

A smaller case of the same pattern affects ip_rt_mtu_expires in the same
function. It is re-read for the freshness check:

	    time_before(jiffies, READ_ONCE(dst->expires) -
				 READ_ONCE(net->ipv4.ip_rt_mtu_expires) / 2))

It is read again for each nexthop in the multipath loop, and once more for
the single-path update_or_create_fnhe() call. Could nexthops updated in
one call get different expiries?

ip_rt_redirect_number is also read twice in ip_rt_send_redirect():

	if (peer->n_redirects >= READ_ONCE(ip_rt_redirect_number)) {
	...
		    peer->n_redirects == READ_ONCE(ip_rt_redirect_number))

The second read only gates the "ignores redirects" warning, so there the
only effect is on logging. Should these be loaded into locals once as
well?

>  	}
>  
>  	if (rt->rt_pmtu == mtu && !lock &&

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109025425.328445-1-jiayuan.chen%40linux.dev