Re: [PATCH v1 net] ipv6: Prevent rt6_insert_exception() for dying fib6_info.

[Ido Schimmel <idosch@nvidia.com>]

On Mon, Sep 21, 2026 at 08:24:07AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · 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).
> 
> - [Medium] The new guard in rt6_insert_exception()
>   (net/ipv6/route.c:1732) trusts f6i->fib6_destroying, but that bit is…
> 
> --
> 
> ipv6: Prevent rt6_insert_exception() for dying fib6_info.
> 
> This reorders fib6_purge_rt() so that fib6_drop_pcpu_from() runs before
> rt6_flush_exceptions(), and adds a f6i->fib6_destroying check in
> rt6_insert_exception() so a dying fib6_info can no longer gain a new
> exception route.  The goal is to stop the fib6_info<->exception reference
> cycle that stalls netdev unregistration.
> 
> > diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> > index 9ea75703b38d..9ff761962b45 100644
> > --- a/net/ipv6/ip6_fib.c
> > +++ b/net/ipv6/ip6_fib.c
> > @@ -1043,8 +1043,8 @@ static void fib6_purge_rt(struct fib6_info *rt, struct fib6_node *fn,
> >  	struct fib6_table *table = rt->fib6_table;
> >  
> >  	/* Flush all cached dst in exception table */
> > -	rt6_flush_exceptions(rt);
> >  	fib6_drop_pcpu_from(rt);
> > +	rt6_flush_exceptions(rt);
> >  
> >  	if (rt->nh) {
> >  		spin_lock(&rt->nh->lock);
> > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > index 08bd68f1b5bb..884d9ab0d50d 100644
> > --- a/net/ipv6/route.c
> > +++ b/net/ipv6/route.c
> > @@ -1729,6 +1729,11 @@ static int rt6_insert_exception(struct rt6_info *nrt,
> >  
> >  	spin_lock_bh(&rt6_exception_lock);
> >  
> > +	if (f6i->fib6_destroying) {
> > +		err = -ENOENT;
> > +		goto out;
> > +	}
> > +
> 
> [Severity: Medium]
> Can this check observe fib6_destroying == 0 after fib6_purge_rt() has
> already set it?
> 
> fib6_destroying shares one u8 storage unit with three other bits in
> struct fib6_info:
> 
> include/net/ip6_fib.h:
> 	u8				should_flush:1,
> 					dst_nocount:1,
> 					dst_nopolicy:1,
> 					fib6_destroying:1,
> 					unused:4;
> 
> Adjacent bitfields are a single memory location, so every assignment to
> any of them is a non-atomic read-modify-write of the whole byte.
> 
> The setter in fib6_drop_pcpu_from() runs with only the table's tb6_lock
> held:
> 
> net/ipv6/ip6_fib.c:fib6_drop_pcpu_from() {
> 	f6i->fib6_destroying = 1;
> 	mb(); /* paired with the cmpxchg() in rt6_make_pcpu_route() */
> 	...
> }
> 
> One peer writer of the same byte is addrconf_disable_policy_idev(), which
> holds only rtnl_net_lock plus idev->lock and ifa->lock:
> 
> net/ipv6/addrconf.c:addrconf_disable_policy_idev() {
> 	...
> 			rcu_read_lock();
> 			ifa->rt->dst_nopolicy = val ? true : false;
> 	...
> }
> 
> Is there any lock common to both paths?  inet6_rtm_delroute() is
> registered with RTNL_FLAG_DOIT_UNLOCKED, and
> ip6_route_del()->__ip6_del_rt()->fib6_del()->fib6_purge_rt() takes only
> rcu_read_lock() and tb6_lock, so a write to
> net.ipv6.conf.<dev>.disable_policy on a host route (ifa->rt) does not
> appear to be serialized against the deletion path.
> 
> If the disable_policy writer loaded the byte before fib6_destroying was
> set and stores its copy back afterwards, the bit is cleared again.  A
> third thread in ip6_rt_update_pmtu() or rt6_do_redirect() that still
> holds the pre-removal fib6_info would then read fib6_destroying == 0 here
> and insert an exception after the final rt6_flush_exceptions(), which
> recreates the fib6_info<->exception cycle and the
> 
> 	unregister_netdevice: waiting for gre6 to become free
> 
> stall described in the commit message.  The same lost update also lets
> rt6_make_pcpu_route() re-add per-CPU routes after
> __fib6_drop_pcpu_from() has run.

It's a pre-existing issue and not a problem introduced by this patch.
The likelihood of this happening is close to zero.

The current patch needs to be backported up to v5.3 and the issue being
flagged here is only visible since v6.16 (bd11ff421d36), so either
handle the issue in a follow-up or in patch 2/2 of v2.

> 
> Note the load here is a plain bitfield read with no READ_ONCE(), and the
> store side is a plain RMW, so this is also visible to KCSAN as a data
> race.  rt6_multipath_flush() writes should_flush under tb6_lock, so
> dst_nopolicy looks like the one unsynchronized peer.
> 
> Would it make sense to give this flag its own storage accessed with
> WRITE_ONCE()/READ_ONCE() (or to serialize all writers of the containing
> byte), and to document which lock protects it?
> 
> The comment above the store in fib6_drop_pcpu_from() still mentions only
> rt6_make_pcpu_route() and the cmpxchg() as the counterpart; could it also
> mention the new rt6_exception_lock reader added here?

I already mentioned this and it's minor.