Re: [PATCH net-next v2 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: 0 (1) · Medium: 0 · Low: 1

- [Low] Changelog/code mismatch: the commit message states "Hash uncached
  routes by their inet6 device", but rt6_uncached_list_add()…

Pre-existing issues:
- [High] rt6_uncached_list_flush() (net/ipv6/route.c:177-207) unlinks a
  route from its uncached list as soon as EITHER of its two device…

--

ipv6: hash uncached routes by device

This replaces the single per-CPU uncached route list with a per-CPU hash
table plus a separate "mismatch" list, so that device teardown only has to
walk the bucket belonging to the device being removed. A new
CONFIG_IPV6_UNCACHED_ROUTE_HASH_BITS controls the bucket count.

Two questions below, one about the changelog wording and one about the
flush path.

    Hash uncached routes by their inet6
    device so ordinary device teardown only visits the matching bucket on each
    CPU.

[Severity: Low]
Is this description accurate?  In rt6_uncached_list_add() the bucket key is
the dst device, not the inet6 device:

	struct net_device *rt_dev = dst_dev(&rt->dst);
	...
		ul = &table->buckets[hash_ptr(rt_dev,
					      CONFIG_IPV6_UNCACHED_ROUTE_HASH_BITS)];

rt6i_idev is only used as a discriminator to divert a route onto the
mismatch list, and rt6_uncached_list_flush_dev() likewise computes
hash_ptr(dev) for the device being unregistered.

Since ip6_rt_get_dev_rcu() can make dst.dev and rt6i_idev->dev refer to
different devices, which of the two is the key matters when auditing which
routes a given flush pass can reach.  Could the changelog say "hash uncached
routes by their dst device" instead?

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

[ ... ]

> @@ -133,11 +134,27 @@ struct uncached_list {
>  	struct list_head	head;
>  };
>  
> -static DEFINE_PER_CPU_ALIGNED(struct uncached_list, rt6_uncached_list);
> +#define RT6_UNCACHED_HASH_SIZE	BIT(CONFIG_IPV6_UNCACHED_ROUTE_HASH_BITS)
> +
> +struct rt6_uncached_table {
> +	struct uncached_list buckets[RT6_UNCACHED_HASH_SIZE];
> +	/* Routes that must be discoverable through two different devices. */
> +	struct uncached_list mismatch;
> +};
> +
> +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 net_device *rt_dev = dst_dev(&rt->dst);
> +	struct uncached_list *ul;
> +
> +	if (rt->rt6i_idev && rt->rt6i_idev->dev != rt_dev)
> +		ul = &table->mismatch;
> +	else
> +		ul = &table->buckets[hash_ptr(rt_dev,
> +					      CONFIG_IPV6_UNCACHED_ROUTE_HASH_BITS)];
>  
>  	rt->dst.rt_uncached_list = ul;
>  
> @@ -157,40 +174,51 @@ 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);
> +	list_for_each_entry_safe(rt, safe, &ul->head, dst.rt_uncached) {
> +		struct inet6_dev *rt_idev = rt->rt6i_idev;
> +		struct net_device *rt_dev = dst_dev(&rt->dst);
> +		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) {
> +			rt->dst.dev = 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);

[Severity: High]
This isn't a bug introduced by this patch, the handled/list_del_init()
sequence is textually identical to the pre-patch loop in
rt6_uncached_list_flush_dev().  But does the new mismatch list turn it into
a net_device reference leak?

handled is set independently by the rt6i_idev branch and the dst.dev branch,
and the route is then unlinked even when only one of the two matched.

For a route on table->mismatch the two fields point at two different real
devices by construction:

	if (rt->rt6i_idev && rt->rt6i_idev->dev != rt_dev)
		ul = &table->mismatch;

Such routes are produced by ordinary local traffic through an enslaved
interface.  ip6_rt_cache_alloc() takes dst.dev from 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;

while ip6_rt_copy_init() sets the inet6 device from the nexthop:

	rt->rt6i_idev = dev ? in6_dev_get(dev) : NULL;

So deleting eth0 first retargets only rt6i_idev to blackhole_netdev, sets
handled and calls list_del_init(), leaving dst.dev still holding the VRF
master or loopback device together with its dst.dev_tracker reference.

After the unlink, rt6_uncached_list_flush_dev() can no longer find that
route:

	ul = &table->buckets[hash_ptr(dev,
				      CONFIG_IPV6_UNCACHED_ROUTE_HASH_BITS)];
	rt6_uncached_list_flush(ul, dev);
	rt6_uncached_list_flush(&table->mismatch, dev);

the route is on neither the keyed bucket nor mismatch, so a later teardown
of the VRF master or loopback never runs netdev_ref_replace() on it.  If the
dst is pinned past that point by sk_dst_cache or a queued skb, does
unregister_netdevice() then loop in netdev_wait_allrefs() printing
"unregister_netdevice: waiting for <dev> to become free", blocking the
netdev todo queue and netns teardown?

The reverse order looks symmetric: deleting the VRF master or loopback first
leaves the eth0 inet6_dev reference in place, which pins eth0 through
idev->dev_tracker.

This also matches the comment added just above:

	/* Routes that must be discoverable through two different devices. */
	struct uncached_list mismatch;

Would it be better to keep the route linked (or relink it into the surviving
device's bucket) until both device references captured at insertion time
have been retargeted, rather than unlinking on the first match?

The accompanying selftest only deletes eth0 and checks that the delete
completes promptly, so it would not observe the surviving reference on the
VRF master or loopback.

>  	}
> +	spin_unlock_bh(&ul->lock);
> +}
> +
>  static void rt6_uncached_list_flush_dev(struct net_device *dev)
>  {
>  	int cpu;
>  
>  	for_each_possible_cpu(cpu) {
> -		struct uncached_list *ul = per_cpu_ptr(&rt6_uncached_list, cpu);
> -		struct rt6_info *rt, *safe;
> +		struct rt6_uncached_table *table;
> +		struct uncached_list *ul;
>  
> +		table = per_cpu_ptr(&rt6_uncached_table, cpu);
> +		ul = &table->buckets[hash_ptr(dev,
> +					      CONFIG_IPV6_UNCACHED_ROUTE_HASH_BITS)];
> +		rt6_uncached_list_flush(ul, dev);
> +		rt6_uncached_list_flush(&table->mismatch, dev);
>  	}
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914-hash-bucket-route-lists-v2-0-29f6297d8a5a%40cloudflare.com