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

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().

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

[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