Re: [PATCH v3 net-next 5/6] ipv4: Batch rt_flush_dev() in netdev_run_todo().

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

Critical: 0 · High: 1 · Medium: 0 · 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] Missing RCU grace period between the device-pointer swap in
  rt_flush_dev(NULL) and freeing the net_device.

--

ipv4: Batch rt_flush_dev() in netdev_run_todo().

This patch takes the IPv4 uncached route flush out of the per-device
unregister path. netdev_run_todo() now calls rt_flush_dev(NULL) once per
batch, without RTNL. With a NULL dev, rt_flush_dev() moves every uncached
route whose device has reached NETREG_UNREGISTERED over to
blackhole_netdev.

Closes: https://lore.kernel.org/netdev/20260917-hash-bucket-route-lists-v3-0-30493a37b6eb@cloudflare.com/ (local)

[ ... ]

[Severity: High]

Is there still an RCU grace period between the rt->dst.dev swap and
freeing the old net_device?

Before this patch, fib_netdev_event()->rt_flush_dev(dev) did the swap
during NETDEV_UNREGISTER. That was before the second synchronize_net() in
unregister_netdevice_many_notify(), and before the rcu_barrier() at the
top of netdev_run_todo(). So any RCU reader that had already loaded the
old rt->dst.dev was done before the device could be freed.

With this patch, the swap and the reference drop both happen after those
calls:

net/ipv4/route.c:rt_flush_dev() {
    ...
	rcu_assign_pointer(rt->dst.dev_rcu, blackhole_netdev);
	netdev_ref_replace(rt_dev, blackhole_netdev,
			   &rt->dst.dev_tracker, GFP_ATOMIC);
    ...
}

If that drops the last extra reference, netdev_wait_allrefs_any() returns
from its first check, before any rcu_barrier() or msleep():

	list_for_each_entry(dev, list, todo_list)
		if (netdev_refcnt_read(dev) == 1)
			return dev;

netdev_run_todo() then goes straight on to priv_destructor, free_netdev()
and kobject_put(). Some drivers instead call free_netdev() right after
unregister_netdev() returns. Either way, netdev_release() frees the
device without waiting:

	/* no need to wait for rcu grace period:
	 * device is dead and about to be freed.
	 */
	kfree(rcu_access_pointer(dev->ifalias));
	kvfree(dev);

The only synchronize_net() left on this path is in netdev_napi_exit(). It
runs only when dev->napi_list is not empty, so most virtual devices skip
it.

Meanwhile, a sender whose socket has an uncached IPv4 route to this device
in sk_dst_cache can be in sk_dst_check()->ipv4_dst_check()->rt_is_expired():

	rcu_read_lock();
	res = rth->rt_genid != rt_genid_ipv4(dev_net_rcu(rth->dst.dev));
	rcu_read_unlock();

rt_flush_dev() does not mark the dst obsolete, so this check still runs.
If the reader loads the old rth->dst.dev and is then preempted (under
PREEMPT_RCU) or delayed while unregistration finishes, can it read
dev->nd_net from freed memory? Other readers that use dst_dev_rcu() rely
on RCU to keep the device alive in the same way.

Deleting the device needs only CAP_NET_ADMIN in a user namespace's netns
(for example "ip link del"), so an unprivileged user with userns can
reach this path.

The later patch "ipv6: Batch rt6_uncached_list_flush_dev() in
netdev_run_todo()" seems to follow the same pattern for IPv6. At the end
of the series, netdev_run_todo() still calls rt_flush_dev(NULL) and
rt6_uncached_list_flush_dev(NULL) and goes directly into the free loop.

Would a synchronize_net() after the flush calls in netdev_run_todo() be
needed here, or is there something else that guarantees a grace period
before the device is freed?

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