Re: [PATCH v6 net-next 06/15] neighbour: Store arp_tbl and nd_tbl in net->neigh_tables[].

[netdev-bot+sashiko@kernel.org]

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

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

- [Low] Commit message rationale for the new `#ifdef CONFIG_PROC_FS`
  guard in arp_net_init() is factually wrong.

Pre-existing issues:
- [High] Concern (verified RESOLVED later in this series): at this
  commit, ndisc_init() (net/ipv6/ndisc.c:2015-2041) calls…

--

neighbour: Store arp_tbl and nd_tbl in net->neigh_tables[].

This adds a per-netns neigh_tables[] array to struct net along with
neigh_table_register()/neigh_table_unregister() helpers, and stores
&arp_tbl and &nd_tbl there from arp_net_init() and ndisc_net_init() as
preparation for making neigh_table per-netns.  New arp_table(net) and
nd_table(net) accessors are added.

The commit message says:

    proc_create_net() is guarded with CONFIG_PROC_FS because it
    returns NULL when =n and setup_net() fails and panic()s.

[Severity: Low]
Is the panic() part of this description accurate?

arp_net_ops is registered from arp_init():

net/ipv4/arp.c:arp_init() {
	neigh_table_init(NEIGH_ARP_TABLE, &arp_tbl);

	dev_add_pack(&arp_packet_type);
	register_pernet_subsys(&arp_net_ops);
	...
}

arp_init() runs from inet_init() (fs_initcall), while the only setup_net()
call that panics is setup_net(&init_net) in net_ns_init(), which is a
pure_initcall and has already completed by then.  For an ops registered
later, a failing ->init is handled in __register_pernet_operations():

	error = ops_init(ops, net); /* on failure the ops are undone and the error returned */

and arp_init() is void, so that -ENOMEM is silently discarded.

Wouldn't the actual consequence on CONFIG_PROC_FS=n be that arp_net_ops
never gets registered, so net->neigh_tables[NEIGH_ARP_TABLE] stays NULL in
every netns, which would then break arp_table(net) once the follow-up
patches convert the ARP users over?  Could the changelog describe that
instead?

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index caa52bf64a2d..286acb7cd504 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1899,6 +1899,18 @@ int neigh_table_clear(int index, struct neigh_table *tbl)
>  	return 0;
>  }
>  
> +int neigh_table_register(struct net *net, struct neigh_table *tbl, int index)
> +{
> +	net->neigh_tables[index] = tbl;
> +
> +	return 0;
> +}
> +
> +void neigh_table_unregister(struct net *net, int index)
> +{
> +	net->neigh_tables[index] = NULL;
> +}
> +
>  static struct neigh_table *neigh_find_table(int family)
>  {
>  	struct neigh_table *tbl = NULL;
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index d409f606aec0..bad17d5aeafc 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -1495,15 +1495,34 @@ static const struct seq_operations arp_seq_ops = {
>  
>  static int __net_init arp_net_init(struct net *net)
>  {
> +	int err;
> +
> +	err = neigh_table_register(net, &arp_tbl, NEIGH_ARP_TABLE);
> +	if (err)
> +		goto err;
> +
> +#ifdef CONFIG_PROC_FS
>  	if (!proc_create_net("arp", 0444, net->proc_net, &arp_seq_ops,
> -			sizeof(struct neigh_seq_state)))
> -		return -ENOMEM;
> +			     sizeof(struct neigh_seq_state))) {
> +		err = -ENOMEM;
> +		goto err_proc_create;
> +	}
> +#endif
> +
>  	return 0;
> +
> +#ifdef CONFIG_PROC_FS
> +err_proc_create:
> +	neigh_table_unregister(net, NEIGH_ARP_TABLE);
> +#endif
> +err:
> +	return err;
>  }

[ ... ]

> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 75515fd99383..dc4c083fe27a 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -1974,12 +1974,16 @@ static int __net_init ndisc_net_init(struct net *net)
>  	struct sock *sk;
>  	int err;
>  
> +	err = neigh_table_register(net, &nd_tbl, NEIGH_ND_TABLE);
> +	if (err)
> +		goto err;
> +
>  	err = inet_ctl_sock_create(&sk, PF_INET6,
>  				   SOCK_RAW, IPPROTO_ICMPV6, net);
>  	if (err < 0) {
>  		net_err_ratelimited("NDISC: Failed to initialize the control socket (err %d)\n",
>  				    err);
> -		return err;
> +		goto err_sock_create;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but while unwinding is being
added here, does the neighbour table in ndisc_init() still leak on the
sysctl failure path?

net/ipv6/ndisc.c:ndisc_init() {
	neigh_table_init(NEIGH_ND_TABLE, &nd_tbl);

#ifdef CONFIG_SYSCTL
	err = neigh_sysctl_register(NULL, &nd_tbl.parms,
				    ndisc_ifinfo_sysctl_change);
	if (err)
		goto out_unregister_pernet;
out:
#endif
	return err;

#ifdef CONFIG_SYSCTL
out_unregister_pernet:
	unregister_pernet_subsys(&ndisc_net_ops);
	goto out;
#endif
}

If neigh_sysctl_register() returns -ENOBUFS (kmemdup or
register_net_sysctl_sz() failure), only the pernet ops are unregistered and
neigh_table_clear(NEIGH_ND_TABLE, &nd_tbl) is never called, so nd_tbl->stats,
nd_tbl->nht, nd_tbl->phash_buckets and the /proc/net/stat/ndisc_cache entry
stay around.

neigh_table_init() also leaves work queued and the proxy timer armed on the
abandoned table:

net/core/neighbour.c:neigh_table_init() {
	INIT_DEFERRABLE_WORK(&tbl->gc_work, neigh_periodic_work);
	queue_delayed_work(system_power_efficient_wq, &tbl->gc_work,
			tbl->parms.reachable_time);
	INIT_DEFERRABLE_WORK(&tbl->managed_work, neigh_managed_work);
	queue_delayed_work(system_power_efficient_wq, &tbl->managed_work, 0);
	...
}

inet6_init() jumps to ndisc_fail on ndisc_init() failure and never calls
ndisc_cleanup(), so nothing balances the initialised table.

This does get resolved later in the same series: after "neighbour:
Namespacify neigh_tables." ndisc_init() becomes just
register_pernet_subsys(&ndisc_net_ops), and both neigh_table_register() and
neigh_sysctl_register() move into ndisc_net_init() with full unwinding
through neigh_sysctl_unregister() and neigh_table_unregister().  Noting it
here only for completeness.

>  	net->ipv6.ndisc_sk = sk;
> @@ -1990,11 +1994,17 @@ static int __net_init ndisc_net_init(struct net *net)
>  	inet6_clear_bit(MC6_LOOP, sk);
>  
>  	return 0;
> +
> +err_sock_create:
> +	neigh_table_unregister(net, NEIGH_ND_TABLE);
> +err:
> +	return err;
>  }

[ ... ]

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