Re: [PATCH v1 net-next 5/5] ipv6: Batch rt6_uncached_list_flush_dev() for dying netns.

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · 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 ->pre_exit_batch() callback can crash the IPv6 init
  error path.
- [Medium] Race between the batch flush and rt6_uncached_list_add().

--

ipv6: Batch rt6_uncached_list_flush_dev() for dying netns.

This commit flushes IPv6 uncached routes for dying netns from a new
->pre_exit_batch() callback, which runs about once per cleanup_net().
Once NET_PRE_EXIT_DONE is set, the per-device rt6_uncached_list_flush_dev()
now returns early. rt6_uncached_list_add() now checks check_net() under
ul->lock, and a route for a dying netns is moved straight to
blackhole_netdev instead of being linked.

Closes: https://lore.kernel.org/netdev/20260917-hash-bucket-route-lists-v3-0-30493a37b6eb@cloudflare.com/
Link: https://lore.kernel.org/netdev/aq2B8PSfjn-xau4V@20HS2G4/

> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 475ced827ec57..7747e4f20fee6 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c

[ ... ]

> @@ -143,7 +159,12 @@ void rt6_uncached_list_add(struct rt6_info *rt)
>  	rt->dst.rt_uncached_list = ul;
>  
>  	spin_lock_bh(&ul->lock);
> -	list_add_tail(&rt->dst.rt_uncached, &ul->head);
> +
> +	if (check_net(dst_dev_net_rcu(&rt->dst)))
> +		list_add_tail(&rt->dst.rt_uncached, &ul->head);
> +	else
> +		rt6_uncached_list_replace(rt);
> +
>  	spin_unlock_bh(&ul->lock);
>  }
>  
> @@ -162,6 +183,9 @@ static void rt6_uncached_list_flush_dev(struct net_device *dev)
>  {
>  	int cpu;
>  
> +	if (dev && net_pre_exit_done(dev_net(dev)))
> +		return;
> +
>  	for_each_possible_cpu(cpu) {
>  		struct uncached_list *ul = per_cpu_ptr(&rt6_uncached_list, cpu);
>  		struct rt6_info *rt, *safe;

[Severity: Medium]
Because of this early return, the batch flush from ->pre_exit_batch() is
the only flush for devices in a dying netns. Can it miss a route that is
being added at the same time?

The dev == NULL path still skips a CPU's list with a lockless check,
before it takes ul->lock:

		if (list_empty(&ul->head))
			continue;

Here is the sequence I have in mind:

CPU A, rt6_uncached_list_add() (e.g. from ip6_pol_route() or
icmp6_dst_alloc()):
  spin_lock_bh(&ul->lock);        /* its own per-cpu list, empty */
  check_net() returns true
  <delayed by a hardirq, vCPU preemption or RT preemption>

CPU B, cleanup_net()->ops_undo_list():
  ops_pre_exit_list()
    ip6_route_net_pre_exit_batch()
      rt6_uncached_list_flush_dev(NULL)
        list_empty() on CPU A's list is true, so it is skipped

CPU A:
  list_add_tail(&rt->dst.rt_uncached, &ul->head);
  spin_unlock_bh(&ul->lock);

CPU B:
  synchronize_rcu();
  WRITE_ONCE(net->undo_state, NET_PRE_EXIT_DONE);
  ...
  NETDEV_UNREGISTER: addrconf_ifdown()->rt6_disable_ip()->
    rt6_uncached_list_flush_dev(dev) returns early

->pre_exit_batch() runs before the synchronize_rcu() in ops_undo_list(),
so CPU A's RCU/BH section is not waited for before the scan.

Would the late route then stay on the uncached list? It would still hold
its dst.dev reference (dev_tracker) and its rt6i_idev reference, and it
would never be moved to blackhole_netdev.

Before this patch, the per-device flush always ran at NETDEV_UNREGISTER,
after the synchronize_net() in unregister_netdevice_many().

If something holds that dst for a long time, could netdev_wait_allrefs()
hang and stall netns cleanup?

One option is to drop the list_empty() shortcut when dev is NULL. Another
is to run the batch flush after the grace period and before
NET_PRE_EXIT_DONE is published. Would either of those work?

[ ... ]

> @@ -6801,6 +6819,11 @@ static int __net_init ip6_route_net_init(struct net *net)
>  	goto out;
>  }
>  
> +static void __net_exit ip6_route_net_pre_exit_batch(struct list_head *net_exit_list)
> +{
> +	rt6_uncached_list_flush_dev(NULL);
> +}
> +

[Severity: Medium]
Could this callback crash on the ip6_route_init() error path?

ip6_route_init() registers ip6_route_net_ops before the per-cpu uncached
lists are initialized. They are only set up at the very end of the
function:

	ret = register_pernet_subsys(&ip6_route_net_ops);
	if (ret)
		goto out_register_inetpeer;
	...
	for_each_possible_cpu(cpu) {
		struct uncached_list *ul = per_cpu_ptr(&rt6_uncached_list, cpu);

		INIT_LIST_HEAD(&ul->head);
		spin_lock_init(&ul->lock);
	}

If a later step fails, the unwind calls
unregister_pernet_subsys(&ip6_route_net_ops). Those steps are fib6_init(),
xfrm6_init(), fib6_rules_init(), registering ip6_route_net_late_ops,
rtnl_register_many(), register_netdevice_notifier() and
bpf_iter_register().

With CONFIG_NET_NS=y, the same undo also runs inside
register_pernet_subsys() if ip6_route_net_init() fails for a netns. That
path goes through __register_pernet_operations()->ops_undo_single().

Both paths reach ops_pre_exit_list(). It calls ->pre_exit_batch() even
when net_exit_list is empty:

	if (ops->pre_exit_batch)
		ops->pre_exit_batch(net_exit_list);

which then calls:

ip6_route_net_pre_exit_batch()
  rt6_uncached_list_flush_dev(NULL)

At this point the static per-cpu heads are still zeroed. With head.next
== NULL, list_empty(&ul->head) returns false. spin_lock_bh() then takes a
lock that was never initialized. list_for_each_entry_safe() also derives
pos from the NULL next pointer and dereferences it.

Before this patch, ip6_route_net_ops had no ->pre_exit_batch(), so this
rollback did not touch the lists.

Would it make sense to initialize the per-cpu uncached lists before
registering ip6_route_net_ops?

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