Re: [PATCH v6 net-next 12/15] neighbour: Namespacify neigh_tables.

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] neigh_table_unregister() (net/core/neighbour.c:1963-1969)
  retracts the table with a plain store `net->neigh_tables[index] =…
- [Low] tools/testing/selftests/net/test_neigh.sh (extern_valid_common)
  still drives "ip ntable change ... thresh1/2/3, base_reachable" in the…
- [Low] The new per-netns neigh_table allocations are not memcg-
  accounted: neigh_table_register() uses kmemdup(tbl, sizeof(*tbl),…
- [Low] neigh_table_inherit() copies init_net's gc_interval,
  gc_thresh1/2/3 and the whole parms.data[] array with bare READ_ONCE()…

--

neighbour: Namespacify neigh_tables.

This makes struct neigh_table a per-netns object: neigh_table_register()
now kmemdup()s the template table, optionally inherits init_net's GC and
default parms values, and calls neigh_table_init() plus
neigh_sysctl_register() for each namespace. A new knob,
net.core.neigh_inherit_init_net, controls the inheritance.

A few questions on the new allocation and teardown paths below.

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 7dc8f0cdbb45..414422bcc0ec 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c

[ ... ]

> @@ -1905,16 +1905,12 @@ static void neigh_table_free(struct neigh_table *tbl)
>  	nht = rcu_dereference_protected(tbl->nht, 1);
>  	tbl->nht = NULL;
>  	neigh_hash_free_rcu(&nht->rcu);
> +
> +	kfree(tbl);
>  }
>
> -/*
> - * Only called from ndisc_cleanup(), which means this is dead code
> - * because we no longer can unload IPv6 module.
> - */
> -int neigh_table_clear(struct neigh_table *tbl)
> +static void neigh_table_clear(struct net *net, struct neigh_table *tbl)
>  {
> -	struct net *net __maybe_unused = &init_net;
> -
>  	cancel_delayed_work_sync(&tbl->managed_work);
>  	cancel_delayed_work_sync(&tbl->gc_work);
>  	timer_shutdown_sync(&tbl->proxy_timer);
> @@ -1923,20 +1919,61 @@ int neigh_table_clear(struct neigh_table *tbl)
>  	remove_proc_entry(tbl->id, net->proc_net_stat);
>
>  	neigh_table_put(tbl);
> +}
>
> -	return 0;
> +int sysctl_neigh_inherit_init_net __read_mostly = 1;
> +
> +static void neigh_table_inherit(struct net *net, struct neigh_table *tbl,
> +				int index)
> +{
> +	const struct neigh_table *init_tbl = init_net.neigh_tables[index];
> +	bool inherit = READ_ONCE(sysctl_neigh_inherit_init_net);
> +	int i;
> +
> +	if (net_eq(net, &init_net) || !inherit)
> +		return;
> +
> +	tbl->gc_interval = READ_ONCE(init_tbl->gc_interval);
> +	tbl->gc_thresh1 = READ_ONCE(init_tbl->gc_thresh1);
> +	tbl->gc_thresh2 = READ_ONCE(init_tbl->gc_thresh2);
> +	tbl->gc_thresh3 = READ_ONCE(init_tbl->gc_thresh3);
> +
> +	for (i = 0; i < NEIGH_VAR_DATA_MAX; i++)
> +		tbl->parms.data[i] = READ_ONCE(init_tbl->parms.data[i]);

[Severity: Low]
Should this snapshot of init_net's table be taken under init_tbl->lock?

neightbl_set() updates exactly these fields under spin_lock_bh(&tbl->lock),
with the comment:

	/*
	 * We acquire tbl->lock to be nice to the periodic timers and
	 * make sure they always see a consistent set of values.
	 */
	spin_lock_bh(&tbl->lock);
	...
	WRITE_ONCE(tbl->gc_thresh1, nla_get_u32(tb[NDTA_THRESH1]));

An "unshare -n" concurrent with RTM_SETNEIGHTBL on init_net can therefore
copy a mix of old and new threshold values into the new namespace.

For what it's worth, the same fields are already written without tbl->lock
via the sysctl path that this patch registers per netns (neigh_sysctl_register()
points NEIGH_VAR_GC_THRESH1..3 and NEIGH_VAR_GC_INTERVAL .data at
&tbl->gc_thresh1..3 and &tbl->gc_interval with plain proc_dointvec* handlers),
and nothing enforces an ordering relation among the thresholds, so the
resulting combination is one an administrator could configure directly.

> }
>
>  int neigh_table_register(struct net *net, struct neigh_table *tbl, int index)
>  {
> +	int err;
> +
> +	tbl = kmemdup(tbl, sizeof(*tbl), GFP_KERNEL);
> +	if (!tbl) {
> +		err = -ENOMEM;
> +		goto err;
> +	}

[Severity: Low]
Should these per-netns allocations be memcg accounted?

Two tables (ARP and ND) are now created for every network namespace, and
namespace creation is reachable by unprivileged users through user
namespaces. Besides this kmemdup(), neigh_table_init() allocates
neigh_hash_alloc(3), kzalloc(phsize, GFP_KERNEL) for phash_buckets and
alloc_percpu(struct neigh_statistics), the last of which scales with
nr_cpu_ids for each of the two tables per namespace.

neigh_sysctl_register() in the same file already uses the accounted flag
for its per-netns allocation:

	t = kmemdup(&neigh_sysctl_template, sizeof(*t), GFP_KERNEL_ACCOUNT);

Would GFP_KERNEL_ACCOUNT here and alloc_percpu_gfp(..., GFP_KERNEL_ACCOUNT)
in neigh_table_init() be more consistent with that?

[Severity: Low]
Does this break the forced-GC and periodic-GC subtests in
tools/testing/selftests/net/test_neigh.sh?

extern_valid_common() still runs the thresholds and counter readback in the
initial netns while the entries are created inside $ns1:

	run_cmd "ip ntable change name $tbl_name thresh3 10 thresh2 9 thresh1 8"

With per-netns tables, neigh_table_inherit() only snapshots values at
registration time, and setup() creates ns1/ns2 before this command runs, so
$ns1's thresholds are unaffected. The same applies to the
"ip -j -s ntable show" readback of forced_gc_runs/periodic_gc_runs, which now
reports init_net's per-netns tbl->stats rather than $ns1's.

The last patch of this series, "selftest: net: Specify netns for ip ntable in
test_neigh.sh.", converts these commands to "ip -n $ns1 ntable ...", so the
only residual effect is a two-commit window where the selftest can fail.

> +
> +	neigh_table_inherit(net, tbl, index);
> +
> +	err = neigh_table_init(net, tbl);
> +	if (err)
> +		goto free_table;
> +
>  	net->neigh_tables[index] = tbl;
>
>  	return 0;
> +
> +free_table:
> +	kfree(tbl);
> +err:
> +	return err;
>  }
>
>  void neigh_table_unregister(struct net *net, int index)
>  {
> +	struct neigh_table *tbl = net->neigh_tables[index];
> +
>  	net->neigh_tables[index] = NULL;
> +	neigh_table_clear(net, tbl);
>  }

[Severity: Medium]
Is a grace period needed between retracting the slot and freeing the table?

The store is a plain one, and neigh_table_clear() -> neigh_table_put() ->
neigh_table_free() now ends in kfree(tbl), so the table can be freed
immediately after the slot is cleared.

Readers load the same slot with a plain load under rcu_read_lock() and then
dereference it, for example neigh_xmit():

	rcu_read_lock();
	net = dev_net_rcu(dev);
	tbl = net->neigh_tables[index];
	if (!tbl) { ... }
	...
	neigh = __neigh_create(tbl, addr, dev, false);

which goes on to take spin_lock_bh(&tbl->lock), rcu_dereference(tbl->nht) and
touch the per-cpu tbl->stats. Before this patch the pointer referred to the
static arp_tbl/nd_tbl, so a stale read was harmless; now it is freed memory.

Some drivers also cache the pointer in their own state across work items, for
instance mlxsw_sp_nexthop4_init() in
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c:

	nh->neigh_tbl = arp_table(net);

I was not able to construct a concrete racing reader: cleanup_net() runs
synchronize_rcu_expedited() and unregisters all netdevs before the subsys
exit ops, and the netlink and /proc readers hold a netns reference. But the
preceding patch, "neighbour: Remove __rcu from neigh_tables[]", justified
dropping the RCU annotation with "neigh_table cannot be registered
dynamically", which no longer holds once the slots are published and freed at
runtime.

Would an RCU-annotated store plus kfree_rcu() of the table remove the
dependence on that ordering?

>
>  static struct neigh_table *neigh_find_table(struct net *net, int family)

[ ... ]

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