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

On Thu, Sep 3, 2026 at 8:10 PM [off-list ref] wrote:

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 =…

This is false-positive.

IPv6 is unloadable and neigh_table_clear() is not called
except for nd_init() failure, which we don't care.

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

quoted

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 ... ]

quoted

@@ -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?

quoted

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

quoted

      return 0;
 }

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