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

[Simon Horman]

On Tue, Nov 10, 2026 at 05:53:17AM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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 &&

I think it would be best to address this issue.