Re: [PATCH v2 net-next 1/2] ipv4: Batch rt_flush_dev() in netdev_run_todo().

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 3 · 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] Retrying the NETDEV_UNREGISTER broadcast no longer flushes IPv4
  uncached routes.
- [High] Moving the IPv4 uncached-route device swap from the
  NETDEV_UNREGISTER notifier into `netdev_run_todo()` removes the RCU…
- [High] ipv4: NULL pointer dereference in rt_flush_dev() with
  uninitialized list

--

ipv4: Batch rt_flush_dev() in netdev_run_todo().

This commit takes the IPv4 uncached route flush out of the per-device
NETDEV_UNREGISTER path. It now calls rt_flush_dev(NULL) once per batched
unregistration in netdev_run_todo(), without holding RTNL. rt_flush_dev()
returns early for dismantling devices. When passed NULL, it moves every
route whose device is NETREG_UNREGISTERED over to blackhole_netdev.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index a8eb382f40caf..9cedc17b5bdc2 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -11838,6 +11838,9 @@ void netdev_run_todo(void)
>  		linkwatch_sync_dev(dev);
>  	}
>  
> +	if (!list_empty(&list))
> +		rt_flush_dev(NULL);
> +
>  	cnt = 0;
>  	while (!list_empty(&list)) {
>  		dev = netdev_wait_allrefs_any(&list);
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index d7da2f1acbb59..1b641901f10dc 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1587,6 +1587,9 @@ void rt_flush_dev(struct net_device *dev)
>  	struct rtable *rt, *safe;
>  	int cpu;
>  
> +	if (dev && dev->dismantle)
> +		return;
> +

[Severity: High]
Can this call reach rt_flush_dev() before ip_rt_init() has set up
rt_uncached_list?

The per-cpu rt_uncached_list entries are only initialised in
ip_rt_init(), which does INIT_LIST_HEAD() and spin_lock_init() for
each possible CPU. That runs from inet_init(), an fs_initcall. Until
then ul->head.next is still NULL.

Before this patch, rt_flush_dev() was only reached through
fib_netdev_event(). That notifier is registered from ip_fib_init(),
which ip_rt_init() calls after it initialises the lists, so the
ordering was implicitly safe.

netdev_run_todo() has no such dependency. It runs from rtnl_unlock()
whenever net_todo_list is non-empty. Any netdev unregistration before
fs_initcall time would now call rt_flush_dev(NULL) on zeroed lists.
One example is an earlier initcall that registers a device and then
unregisters it on a later failure.

In that case list_empty(&ul->head) returns false, because head->next
is NULL rather than &ul->head. The loop then takes the uninitialised
ul->lock and enters list_for_each_entry_safe(). That derives pos from
the NULL head->next and immediately loads pos->dst.rt_uncached.next,
which is address 0. The result is a NULL pointer dereference during
boot.

The IPv6 patch later in the series seems to handle the same case in
rt6_uncached_list_flush_dev() by skipping a list whose head.next is
still NULL. Should rt_flush_dev() get a similar guard? Or should
netdev_run_todo() skip the call until IPv4 routing is initialised?

[Severity: High]
With this early return, does the NETDEV_UNREGISTER that
netdev_wait_allrefs_any() resends still flush any IPv4 uncached routes?

netdev_wait_allrefs_any() resends NETDEV_UNREGISTER about once a second so
protocols can drop late references. That reaches
fib_netdev_event()->rt_flush_dev(dev). dev->dismantle is already set, so
the first notifier call and every retry now return straight away.

The only flush left for a dying device is the single rt_flush_dev(NULL) in
netdev_run_todo(). It runs once, before the wait loop, and no grace period
follows it.

Can an uncached route with the dying device be added after that pass? One
possible path goes through xfrm_bundle_create():

xfrm_bundle_create()
    rcu_read_lock();
    dev = dst_dev_rcu(dst);
    ...
    xfrm_fill_dst(xdst_prev, dev, fl)
        xfrm4_fill_dst()
            netdev_hold(dev, &xdst->u.dst.dev_tracker, GFP_ATOMIC);
            ...
            rt_add_uncached_list(&xdst->u.rt);

Say the sweep moves dst_orig to blackhole_netdev after dev is read but
before the new xdst is linked. The xdst then goes onto rt_uncached_list
after the sweep has already passed that CPU's list, and it still holds a
reference on the dying device.

Before this patch, synchronize_net() in unregister_netdevice_many_notify()
ran after the notifier flush, and each retry ran rt_flush_dev(dev) again.
Nothing now seems to come back to such an entry.

Could unregister_netdevice then loop on "waiting for %s to become free" for
as long as the xdst lives? For a bundle cached on an idle connected socket,
that could be forever.

>  	for_each_possible_cpu(cpu) {
>  		struct uncached_list *ul = &per_cpu(rt_uncached_list, cpu);
>  
> @@ -1595,10 +1598,14 @@ void rt_flush_dev(struct net_device *dev)
>  
>  		spin_lock_bh(&ul->lock);
>  		list_for_each_entry_safe(rt, safe, &ul->head, dst.rt_uncached) {
> -			if (rt->dst.dev != dev)
> +			struct net_device *rt_dev = rt->dst.dev;
> +
> +			if (dev ? rt_dev != dev :
> +			    READ_ONCE(rt_dev->reg_state) != NETREG_UNREGISTERED)
>  				continue;
> +
>  			rcu_assign_pointer(rt->dst.dev_rcu, blackhole_netdev);
> -			netdev_ref_replace(dev, blackhole_netdev,
> +			netdev_ref_replace(rt_dev, blackhole_netdev,
>  					   &rt->dst.dev_tracker, GFP_ATOMIC);

[Severity: High]
Does moving the swap here remove the RCU grace period between publishing
blackhole_netdev and freeing the old net_device?

Before, rt_flush_dev(dev) ran from the NETDEV_UNREGISTER notifier. Then
unregister_netdevice_many_notify() called synchronize_net(), so any reader
that had loaded the old rt->dst.dev was done before teardown.

The swap now runs after every synchronize_net(), and also after the
rcu_barrier() in netdev_run_todo(). If netdev_ref_replace() drops the last
extra reference, netdev_wait_allrefs_any() returns right away:

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

netdev_run_todo() then goes on to free_netdev() and kobject_put(), and
netdev_release() frees the device directly:

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

Could an RCU reader that loaded the old device still be using it at that
point? One example is rt_is_expired():

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

Another is xfrm_bundle_create(). It reads dst_dev_rcu(dst) and then calls
xfrm4_fill_dst()->netdev_hold(dev), which would write to the refcount of a
freed device.

The ul->lock spinlock only serializes list writers, not these readers. The
IPv6 patch that follows in the series uses the same pattern
(rt6_uncached_list_flush_dev(NULL) in netdev_run_todo()), so the problem
remains at the end of the series.

Does this need a synchronize_net() after the batched flush?

>  			list_del_init(&rt->dst.rt_uncached);
>  		}

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