Re: [PATCH net-next v3 2/3] ipv6: hash uncached routes by device

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)

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] Partially-repaired uncached routes are unlinked from the list,
  permanently pinning the second device.

Pre-existing issues:
- [Low] `rt6_uncached_list_flush()` tests `list_empty(&ul->head)` before
  acquiring `ul->lock` (net/ipv6/route.c:178-181) and returns on the…

--

ipv6: hash uncached routes by device

The patch replaces the single per-CPU uncached route list with a 64 bucket
per-CPU hash table keyed on rt6i_idev->dev (falling back to dst_dev()), so
ordinary device teardown only visits one bucket per CPU.  Loopback and L3
master devices scan all buckets because they can be referenced through
dst_dev() instead.  The flush loop itself is moved into a new helper,
rt6_uncached_list_flush().

> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 7535b09068a06..cda81e91be652 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c

[ ... ]

> @@ -133,11 +134,23 @@ struct uncached_list {
>  	struct list_head	head;
>  };
>  
> -static DEFINE_PER_CPU_ALIGNED(struct uncached_list, rt6_uncached_list);
> +#define RT6_UNCACHED_HASH_BITS	6
> +#define RT6_UNCACHED_HASH_SIZE	BIT(RT6_UNCACHED_HASH_BITS)
> +
> +struct rt6_uncached_table {
> +	struct uncached_list buckets[RT6_UNCACHED_HASH_SIZE];
> +};
> +
> +static DEFINE_PER_CPU_ALIGNED(struct rt6_uncached_table, rt6_uncached_table);
>  
>  void rt6_uncached_list_add(struct rt6_info *rt)
>  {
> -	struct uncached_list *ul = raw_cpu_ptr(&rt6_uncached_list);
> +	struct rt6_uncached_table *table = raw_cpu_ptr(&rt6_uncached_table);
> +	struct uncached_list *ul;
> +	struct net_device *dev;
> +
> +	dev = rt->rt6i_idev ? rt->rt6i_idev->dev : dst_dev(&rt->dst);
> +	ul = &table->buckets[hash_ptr(dev, RT6_UNCACHED_HASH_BITS)];
>  
>  	rt->dst.rt_uncached_list = ul;
>  
> @@ -157,40 +170,58 @@ void rt6_uncached_list_del(struct rt6_info *rt)
>  	}
>  }
>  
> +static void rt6_uncached_list_flush(struct uncached_list *ul,
> +				    struct net_device *dev)
> +{
> +	struct rt6_info *rt, *safe;
> +
> +	if (list_empty(&ul->head))
> +		return;
> +
> +	spin_lock_bh(&ul->lock);

[Severity: Low]
This isn't a bug introduced by this patch, the same unlocked list_empty()
test existed in rt6_uncached_list_flush_dev() before (with continue instead
of return), and net/ipv4/route.c uses the same idiom.

Still, the test reads ul->head outside ul->lock while
rt6_uncached_list_add() mutates the same head under the lock from softirq
context:

net/ipv6/route.c:rt6_uncached_list_add() {
	spin_lock_bh(&ul->lock);
	list_add_tail(&rt->dst.rt_uncached, &ul->head);
	spin_unlock_bh(&ul->lock);
}

Can the flush skip a bucket that an adder has just populated, so a route
still referencing the device being torn down is left on the list for that
pass?

As far as I can tell such a route is picked up again, because
netdev_wait_allrefs_any() rebroadcasts NETDEV_UNREGISTER roughly once per
second while references remain, and addrconf_ifdown() calls
rt6_disable_ip(dev, event) before its "idev = __in6_dev_get(dev); if (!idev)
return -ENODEV;" early return, so the flush re-runs.  Would it still be
worth moving the list_empty() check under the lock, since the fast path is
already per-bucket now?

> +	list_for_each_entry_safe(rt, safe, &ul->head, dst.rt_uncached) {
> +		struct net_device *rt_dev = dst_dev(&rt->dst);
> +		struct inet6_dev *rt_idev = rt->rt6i_idev;
> +		bool handled = false;
> +
> +		if (rt_idev && rt_idev->dev == dev) {
> +			rt->rt6i_idev = in6_dev_get(blackhole_netdev);
> +			in6_dev_put(rt_idev);
> +			handled = true;
> +		}
> +
> +		if (rt_dev == dev) {
> +			rcu_assign_pointer(rt->dst.dev_rcu, blackhole_netdev);
> +			netdev_ref_replace(rt_dev, blackhole_netdev,
> +					   &rt->dst.dev_tracker, GFP_ATOMIC);
> +			handled = true;
> +		}
> +		if (handled)
> +			list_del_init(&rt->dst.rt_uncached);
> +	}
> +	spin_unlock_bh(&ul->lock);
> +}

[Severity: High]
The single handled flag unlinks the route as soon as either of its two
device references matched, even when the other one still points at a
different live device.  Does that break the scan_all fallback this patch
relies on?

The divergent case is the one the changelog describes.  For
RTF_LOCAL/RTF_ANYCAST routes ip6_rt_get_dev_rcu() returns the netns
loopback or the L3 master:

net/ipv6/route.c:ip6_rt_get_dev_rcu() {
	if (res->fib6_flags & (RTF_LOCAL | RTF_ANYCAST)) {
		if (netif_is_l3_slave(dev) && !rt6_need_strict(...))
			dev = l3mdev_master_dev_rcu(dev) ? : dev_net(dev)->loopback_dev;
		else if (!netif_is_l3_master(dev))
			dev = dev_net(dev)->loopback_dev;
	...
}

while ip6_rt_copy_init() keeps the original interface's inet6_dev via
rt->rt6i_idev = in6_dev_get(nh->fib_nh_dev), so dst_dev(&rt->dst) is vrf0 or
lo and rt6i_idev->dev is eth0.  Such a clone reaches the uncached list from
the FLOWI_FLAG_KNOWN_NH branch of ip6_pol_route():

net/ipv6/route.c:ip6_pol_route() {
	rt = ip6_rt_cache_alloc(&res, &fl6->daddr, NULL);

	if (rt) {
		...
		rt6_uncached_list_add(rt);
}

Removing eth0 then takes only the first branch, sets handled = true and
unlinks the entry, while rt->dst.dev_rcu / dst.dev_tracker still hold a
reference on vrf0.

Deleting vrf0 afterwards walks all 64 buckets, but the entry is no longer on
any list, so scan_all cannot repair it.  The per-second NETDEV_UNREGISTER
rebroadcast from netdev_wait_allrefs_any() has the same problem.  Since the
flush does not set dst->obsolete, a dst parked in sk_dst_cache or in a
delayed qdisc can hold that reference for an unbounded time, so
unregister_netdevice keeps waiting for vrf0 to become free and the deleting
task stays in netdev_run_todo; when the pinned device is a netns loopback,
cleanup_net() is stuck too.

The reverse order has the mirror problem: deleting the VRF or loopback first
unlinks on the dst_dev() match while rt6i_idev still pins eth0 through
inet6_dev->dev_tracker.

Would it be better to unlink only when no device reference remains, for
example:

		if (handled && dst_dev(&rt->dst) != dev &&
		    (!rt->rt6i_idev || rt->rt6i_idev->dev != dev))
			list_del_init(&rt->dst.rt_uncached);

or more directly, only delist once both rt6i_idev->dev and dst_dev() have
been repointed at blackhole_netdev?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917-hash-bucket-route-lists-v3-0-30493a37b6eb%40cloudflare.com