Re: [PATCH v6 net-next 11/15] neighbour: Convert neigh_table.entries to refcount_t.

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] neigh_table_free() frees RCU-published, table-owned resources
  with no grace period, while the objects that point at the table are…
- [Low] Teardown ordering concern: neigh_table_clear() ends with
  neigh_table_put(), dropping the base reference installed by…

--

neighbour: Convert neigh_table.entries to refcount_t.

This converts neigh_table.entries from atomic_t to refcount_t and adds
neigh_table_get()/neigh_table_put() helpers plus a new neigh_table_free()
so that the table resources are released only when the last neighbour is
gone.  neigh_table_clear() now just drops the base reference taken in
neigh_table_init().

A couple of questions about the new teardown path below.

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 36488dbd1512..7dc8f0cdbb45 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -55,6 +55,23 @@ static void neigh_notify(struct neighbour *n, int type, int flags, u32 pid);
>  static void __neigh_notify(struct neighbour *n, int type, int flags, u32 pid);
>  static void pneigh_ifdown(struct neigh_table *tbl, struct net_device *dev,
>  			  bool skip_perm);
> +static void neigh_table_free(struct neigh_table *tbl);
> +
> +static void neigh_table_get(struct neigh_table *tbl)
> +{
> +	refcount_inc(&tbl->entries);
> +}
> +
> +static void neigh_table_put(struct neigh_table *tbl)
> +{
> +	if (refcount_dec_and_test(&tbl->entries))
> +		neigh_table_free(tbl);
> +}
> +
> +static int neigh_table_entries(struct neigh_table *tbl)
> +{
> +	return refcount_read(&tbl->entries) - 1;
> +}
>  
>  #ifdef CONFIG_PROC_FS
>  static const struct seq_operations neigh_stat_seq_ops;

[ ... ]

> @@ -1845,6 +1862,7 @@ void neigh_table_init(struct neigh_table *tbl)
>  	tbl->last_flush = now;
>  	tbl->last_rand	= now + tbl->parms.reachable_time * 20;
>  
> +	refcount_set(&tbl->entries, 1);
>  	spin_lock_init(&tbl->lock);
>  	mutex_init(&tbl->phash_lock);
>  	skb_queue_head_init_class(&tbl->proxy_queue,

[ ... ]

> @@ -924,7 +941,7 @@ void neigh_destroy(struct neighbour *neigh)
>  
>  	neigh_dbg(2, "neigh %p is destroyed\n", neigh);
>  
> -	atomic_dec(&neigh->tbl->entries);
> +	neigh_table_put(neigh->tbl);
>  	kfree_rcu(neigh, rcu);
>  }
>  EXPORT_SYMBOL(neigh_destroy);

[ ... ]

> @@ -1874,6 +1892,21 @@ void neigh_table_init(struct neigh_table *tbl)
>  	panic("cannot allocate memory");
>  }
>  
> +static void neigh_table_free(struct neigh_table *tbl)
> +{
> +	struct neigh_hash_table *nht;
> +
> +	free_percpu(tbl->stats);
> +	tbl->stats = NULL;
> +
> +	kfree(tbl->phash_buckets);
> +	tbl->phash_buckets = NULL;
> +
> +	nht = rcu_dereference_protected(tbl->nht, 1);
> +	tbl->nht = NULL;
> +	neigh_hash_free_rcu(&nht->rcu);
> +}

[Severity: Medium]
Is it safe to free these RCU-published members synchronously here?
neigh_table_free() calls free_percpu(tbl->stats), kfree(tbl->phash_buckets)
and invokes the RCU callback neigh_hash_free_rcu() directly, with no
grace period in between.

At the same time neigh_destroy() drops the table reference before the
neighbour body is retired:

	neigh_table_put(neigh->tbl);
	kfree_rcu(neigh, rcu);

If that put is the last one, the table (the pointee) is reclaimed
immediately while the neighbour holding neigh->tbl survives a full grace
period.

Lockless readers hold no-ref neighbour pointers under rcu_read_lock() and
dereference the table through them, for example in __neigh_event_send():

	NEIGH_CACHE_STAT_INC(neigh->tbl, unres_discards);

which is reached from the transmit path
ip_finish_output2()/ip6_finish_output2() -> neigh_output() ->
neigh_event_send().  tbl->nht is read via rcu_dereference() in
___neigh_lookup_noref(), and tbl->phash_buckets is declared __rcu and
walked by pneigh_lookup().

The same file retires nht only after 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);
	...
}

Note also that neigh_table_put() can now reach this free from softirq
context, since neigh_timer_handler() -> neigh_release() -> neigh_destroy()
runs long after neigh_table_clear() returned, and neigh_destroy() releases
the netdev reference before the table reference, so netdev_wait_allrefs()
does not order it either.

Most reader windows do look drained in practice (cleanup_net() runs
synchronize_rcu_expedited() before the pernet exit ops, and
unregister_netdevice_many_notify() runs synchronize_net() before the
neighbour flush), so I could not show a reader that is still in flight at
that instant.  Still, would it be better to defer this free with
call_rcu()/kfree_rcu(), or to drop the table reference from the
neighbour's own RCU callback, especially since the follow-up patch adds
kfree(tbl) to this function?

>  /*
>   * Only called from ndisc_cleanup(), which means this is dead code
>   * because we no longer can unload IPv6 module.
> @@ -1881,26 +1914,15 @@ void neigh_table_init(struct neigh_table *tbl)
>  int neigh_table_clear(struct neigh_table *tbl)
>  {
>  	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_shutdown_sync(&tbl->proxy_timer);
>  
>  	neigh_ifdown(tbl, NULL);
> -	DEBUG_NET_WARN_ON_ONCE(atomic_read(&tbl->entries));
> -
>  	remove_proc_entry(tbl->id, net->proc_net_stat);
>  
> -	free_percpu(tbl->stats);
> -	tbl->stats = NULL;
> -
> -	kfree(tbl->phash_buckets);
> -	tbl->phash_buckets = NULL;
> -
> -	nht = rcu_dereference_protected(tbl->nht, 1);
> -	tbl->nht = NULL;
> -	neigh_hash_free_rcu(&nht->rcu);
> +	neigh_table_put(tbl);
>  
>  	return 0;
>  }

[Severity: Low]
This is more of an ordering question.  neigh_table_clear() drops the base
reference installed by refcount_set(&tbl->entries, 1) in
neigh_table_init(), but at this commit the table is still published: in
ndisc_cleanup() the call order is neigh_table_clear(&nd_tbl) followed by
unregister_pernet_subsys(&ndisc_net_ops).

Once the base reference is gone, neigh_table_entries() returns
refcount_read() - 1, which is entries - 1 in general and -1 when the last
neighbour is gone.  That value feeds the __u32 uAPI field:

	.ndtc_entries		= neigh_table_entries(tbl),

and the %08x in neigh_stat_seq_show(), where it would print as ffffffff.

Checking the rest of the series, this looks unreachable in the final
state: neigh_table_unregister() sets net->neigh_tables[index] = NULL
before calling neigh_table_clear(), the proc entry is removed and drained
before the put, and ndisc_cleanup() no longer calls neigh_table_clear()
at all.  At this commit the only caller is the inet6_init() error path
that the comment above already describes as dead code.

Would it still be worth dropping the base reference only after the table
is unpublished, so the biased accessor is never read without it?

> @@ -2275,7 +2297,7 @@ static int neightbl_fill_info(struct sk_buff *skb, struct neigh_table *tbl,
>  		struct ndt_config ndc = {
>  			.ndtc_key_len		= tbl->key_len,
>  			.ndtc_entry_size	= tbl->entry_size,
> -			.ndtc_entries		= atomic_read(&tbl->entries),
> +			.ndtc_entries		= neigh_table_entries(tbl),
>  			.ndtc_last_flush	= jiffies_to_msecs(flush_delta),
>  			.ndtc_last_rand		= jiffies_to_msecs(rand_delta),
>  			.ndtc_proxy_qlen	= READ_ONCE(tbl->proxy_queue.qlen),
> @@ -3495,7 +3517,7 @@ static int neigh_stat_seq_show(struct seq_file *seq, void *v)
>  	seq_printf(seq, "%08x %08lx %08lx %08lx   %08lx %08lx %08lx   "
>  			"%08lx         %08lx         %08lx         "
>  			"%08lx       %08lx            %08lx\n",
> -		   atomic_read(&tbl->entries),
> +		   neigh_table_entries(tbl),
>  
>  		   st->allocs,
>  		   st->destroys,

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