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

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.

[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.

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?

 	bucket = rcu_dereference_protected(nh->rt6i_exception_bucket,
 					  lockdep_is_held(&rt6_exception_lock));
 	if (!bucket) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918082209.2853582-1-kuniyu%40google.com