Re: [PATCH v6 net-next 10/15] neighbour: Clean up neigh_table_init() and neigh_table_clear().

[netdev-bot+sashiko@kernel.org]

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

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

- [High] net/core/neighbour.c:neigh_table_clear() now reclaims the RCU-
  published neighbour hash table synchronously: `nht =…

Pre-existing issues:
- [Low] neigh_table_clear() (net/core/neighbour.c:1881-1906) frees table-
  wide resources (free_percpu(tbl->stats) at 1895,…

--

neighbour: Clean up neigh_table_init() and neigh_table_clear().

This reworks neigh_table_init() to allocate everything up front and unwind
on failure, and reorders neigh_table_clear() to mirror it. The teardown
side also switches to timer_shutdown_sync(), drops the explicit
pneigh_queue_purge(), replaces pr_crit() with DEBUG_NET_WARN_ON_ONCE(), and
calls neigh_hash_free_rcu() directly instead of via call_rcu().

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 34da4cdc813d..36488dbd1512 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c

[ ... neigh_table_init() hunk snipped ... ]

> @@ -1869,26 +1880,27 @@ void neigh_table_init(struct neigh_table *tbl)
>   */
>  int neigh_table_clear(struct neigh_table *tbl)
>  {
> -	/* It is not clean... Fix it to unload IPv6 module safely */
> +	struct net *net __maybe_unused = &init_net;
> +	struct neigh_hash_table *nht;
> +
>  	cancel_delayed_work_sync(&tbl->managed_work);
>  	cancel_delayed_work_sync(&tbl->gc_work);
> -	timer_delete_sync(&tbl->proxy_timer);
> -	pneigh_queue_purge(&tbl->proxy_queue, NULL, tbl->family);
> +	timer_shutdown_sync(&tbl->proxy_timer);
> +
>  	neigh_ifdown(tbl, NULL);
> -	if (atomic_read(&tbl->entries))
> -		pr_crit("neighbour leakage\n");
> +	DEBUG_NET_WARN_ON_ONCE(atomic_read(&tbl->entries));

[Severity: Low]

This isn't a bug introduced by this patch, but the diagnostic swap makes a
pre-existing lifecycle problem invisible on production kernels: with
CONFIG_DEBUG_NET=n, DEBUG_NET_WARN_ON_ONCE() compiles to nothing, so the
"still have entries" condition that previously always printed is no longer
reported.

The table-owned resources are still freed unconditionally just below, and a
neighbour destroyed afterwards touches them:

net/core/neighbour.c:neigh_destroy() {
	NEIGH_CACHE_STAT_INC(neigh->tbl, destroys);
	...
	atomic_dec(&neigh->tbl->entries);
}

The unconditional freeing itself is unchanged from the baseline, and the
next patch in the series, "neighbour: Convert neigh_table.entries to
refcount_t", defers freeing of tbl->stats, tbl->phash_buckets and tbl->nht
into neigh_table_free() once the last reference is dropped.

Is dropping the production-visible message intentional for this
intermediate commit?

>  
> -	call_rcu(&rcu_dereference_protected(tbl->nht, 1)->rcu,
> -		 neigh_hash_free_rcu);
> -	tbl->nht = NULL;
> +	remove_proc_entry(tbl->id, net->proc_net_stat);
> +
> +	free_percpu(tbl->stats);
> +	tbl->stats = NULL;
>  
>  	kfree(tbl->phash_buckets);
>  	tbl->phash_buckets = NULL;
>  
> -	remove_proc_entry(tbl->id, init_net.proc_net_stat);
> -
> -	free_percpu(tbl->stats);
> -	tbl->stats = NULL;
> +	nht = rcu_dereference_protected(tbl->nht, 1);
> +	tbl->nht = NULL;
> +	neigh_hash_free_rcu(&nht->rcu);

[Severity: High]

Can this free the neighbour hash table while lockless readers still hold a
pointer to it?

neigh_hash_free_rcu() is an RCU callback body, so calling it directly
releases both the bucket array and the table object immediately:

net/core/neighbour.c:neigh_hash_free_rcu() {
	...
	kfree(nht->hash_heads);
	kfree(nht);
}

Nothing in the new teardown sequence waits for a grace period:
cancel_delayed_work_sync() twice, timer_shutdown_sync(), neigh_ifdown()
and remove_proc_entry(). neigh_ifdown() only takes tbl->lock, which the
readers never take.

The readers dereference tbl->nht under RCU with no NULL check:

include/net/neighbour.h:___neigh_lookup_noref() {
	struct neigh_hash_table *nht = rcu_dereference(tbl->nht);
	...
	hash_val = hash(pkey, dev, nht->hash_rnd) >> (32 - nht->hash_shift);
	neigh_for_each_in_bucket_rcu(n, &nht->hash_heads[hash_val])
}

so a reader that already loaded nht reads hash_shift, hash_rnd and
hash_heads out of freed slab memory and then follows whatever hlist
contents are left there. neigh_lookup(), neigh_dump_table(),
neigh_seq_start() and neightbl_fill_info() have the same shape.

Is the table still published while this runs? ndisc_cleanup() calls
neigh_table_clear(&nd_tbl) before unregister_pernet_subsys(&ndisc_net_ops),
and the pernet exit path is what clears net->neigh_tables[NEIGH_ND_TABLE],
so nd_tbl remains installed in every netns during the free. An
unprivileged RTM_GETNEIGH dump reaches it holding only rcu_read_lock():

net/core/neighbour.c:neigh_dump_info() {
	rcu_read_lock();
	for (t = 0; t < NEIGH_NR_TABLES; t++) {
		tbl = net->neigh_tables[t];

and ndisc_cleanup() is live today from the module-init error unwind:

net/ipv6/af_inet6.c:inet6_init() {
igmp_fail:
	ndisc_cleanup();

The peer site retiring the same object type still uses a grace period:

net/core/neighbour.c:neigh_hash_grow() {
	rcu_assign_pointer(tbl->nht, new_nht);
	call_rcu(&old_nht->rcu, neigh_hash_free_rcu);
}

Would keeping call_rcu()/kfree_rcu(), or inserting a synchronize_net()
before the free, be preferable here? The plain NULL store into the __rcu
pointer tbl->nht predates this patch; what is new is the access to freed
memory. This also does not appear to be addressed later in the series: at
the end of the range the same unsynchronised free lives in
neigh_table_free(), now additionally doing kfree(tbl), while
neigh_table_unregister() clears net->neigh_tables[index] and calls
neigh_table_clear()/neigh_table_put() with no grace period in between.

>  
>  	return 0;
>  }

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