Re: [PATCH v6 net-next 08/15] ipv4: Replace &arp_tbl with arp_table(net).

[netdev-bot+sashiko@kernel.org]

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

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

- [High] mlx5e_tc_update_neigh_used_value()
  (drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c) now…
- [Low] Documentation/networking/switchdev.rst still instructs switchdev
  driver authors to resolve nexthops against, and monitor updates to,…
- [Low] The patch introduces new reads of the RCU-protected netns pointer
  dev->nd_net via the non-RCU accessor dev_net() in paths that run only…

Pre-existing issues:
- [Medium] The neighbour fallback path in mlx5e_ipsec_init_macs()
  (drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c, label…

--

ipv4: Replace &arp_tbl with arp_table(net).

In preparation for per-netns neighbour tables, this converts all users of
the global arp_tbl to the arp_table(net) accessor, makes arp_tbl static in
net/ipv4/arp.c and drops its EXPORT_SYMBOL. It also removes some driver
checks that compared n->tbl against &arp_tbl and &nd_tbl.

A few questions below.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
> index 8b827201935e..33737a29ab97 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
> @@ -393,20 +393,10 @@ void mlx5e_tc_update_neigh_used_value(struct mlx5e_neigh_hash_entry *nhe)
>  	struct mlx5e_encap_entry *e = NULL;
>  	struct mlx5e_tc_flow *flow;
>  	struct mlx5_fc *counter;
> -	struct neigh_table *tbl;
>  	bool neigh_used = false;
>  	struct neighbour *n;
>  	u64 lastuse;
>  
> -	if (m_neigh->family == AF_INET)
> -		tbl = &arp_tbl;
> -#if IS_ENABLED(CONFIG_IPV6)
> -	else if (m_neigh->family == AF_INET6)
> -		tbl = &nd_tbl;
> -#endif
> -	else
> -		return;
> -
>  	/* mlx5e_get_next_valid_encap() releases previous encap before returning
>  	 * next one.
>  	 */
> @@ -447,12 +437,23 @@ void mlx5e_tc_update_neigh_used_value(struct mlx5e_neigh_hash_entry *nhe)
>  	trace_mlx5e_tc_update_neigh_used_value(nhe, neigh_used);
>  
>  	if (neigh_used) {
> +		struct net_device *dev = READ_ONCE(nhe->neigh_dev);
> +		struct net *net = dev_net(dev);
> +		struct neigh_table *tbl;

[Severity: High]
Is nhe->neigh_dev safe to dereference here?  As far as I can tell no
reference is taken on it.  It is only stored:

drivers/net/ethernet/mellanox/mlx5/core/en/rep/neigh.c:mlx5e_rep_neigh_entry_create() {
	...
	WRITE_ONCE((*nhe)->neigh_dev, neigh_dev);
	...
}

and there is no netdev_hold()/dev_hold() for it anywhere, and no
NETDEV_UNREGISTER handling in the mlx5 rep neigh code that would clear it.

The rest of the driver only ever compares the field, never dereferences it:

drivers/net/ethernet/mellanox/mlx5/core/en/rep/neigh.c:mlx5e_rep_neigh_update() {
	...
	same_dev = READ_ONCE(nhe->neigh_dev) == n->dev;
	...
}

Before this patch the pointer was only handed to neigh_lookup(), which
never loads from the netdev (arp_hashfn() uses hash32_ptr(dev) and the
lookup only compares n->dev == dev), so a stale pointer was harmless.
Now dev_net(dev) reads dev->nd_net and the result is dereferenced again by
arp_table(net) as net->neigh_tables[NEIGH_ARP_TABLE].

If the cached route egress device (for example a vlan or bond on top of the
uplink) was unregistered and freed while the nhe is still alive, does this
read freed memory and then index a garbage struct net?  The reachable path
is the periodic work:

drivers/net/ethernet/mellanox/mlx5/core/en/rep/neigh.c:mlx5e_rep_neigh_stats_work() {
	...
	while ((nhe = mlx5e_get_next_nhe(rpriv, nhe)) != NULL)
		mlx5e_tc_update_neigh_used_value(nhe);
	...
}

and rtnl_lock() held there does not keep an already-unregistered netdev
alive, since free_netdev() runs from netdev_run_todo() after rtnl_unlock().

Also, note 3 in the changelog says:

  "3. mlx5e_tc_update_neigh_used_value() dereferences nhe->neigh_dev
      in trace_mlx5e_tc_update_neigh_used_value()."

Does that justification hold?  The tracepoint dereference lives in
TP_fast_assign():

drivers/net/ethernet/mellanox/mlx5/core/diag/en_tc_tracepoint.h:
	TP_STRUCT__entry(__string(devname, nhe->neigh_dev->name)

which only runs when the tracepoint is enabled, so it does not show the
pointer is safe to dereference unconditionally.

Would deriving the netns from a refcounted device such as e->out_dev or
priv->netdev work instead?

>  		nhe->reported_lastuse = jiffies;
>  
> +#if IS_ENABLED(CONFIG_IPV6)
> +		if (m_neigh->family != AF_INET)
> +			tbl = &nd_tbl;
> +		else
> +#endif
> +			tbl = arp_table(net);
> +
>  		/* find the relevant neigh according to the cached device and
>  		 * dst ip pair
>  		 */
> -		n = neigh_lookup(tbl, &m_neigh->dst_ip, READ_ONCE(nhe->neigh_dev));
> +		n = neigh_lookup(tbl, &m_neigh->dst_ip, dev);
>  		if (!n)
>  			return;
>  
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> index db260e3d1412..37a8ddee3ea1 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> @@ -364,9 +365,10 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
>  	return;
>  
>  neigh:
> -	n = neigh_lookup(&arp_tbl, pkey, netdev);
> +	tbl = arp_table(dev_net(netdev));
> +	n = neigh_lookup(tbl, pkey, netdev);
>  	if (!n) {
> -		n = neigh_create(&arp_tbl, pkey, netdev);
> +		n = neigh_create(tbl, pkey, netdev);
>  		if (IS_ERR(n))
>  			return;
>  		neigh_event_send(n, NULL);

[Severity: Medium]
This isn't a bug introduced by this patch, since the pre-patch code already
used &arp_tbl unconditionally here, but should this fallback pick the table
based on addrs->family?

The AF_INET6 branch above can reach this label with a 16-byte key:

drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c:mlx5e_ipsec_init_macs() {
	...
			pkey = &addrs->saddr.a6;
	...
	case AF_INET6:
		if (!IS_ENABLED(CONFIG_IPV6) ||
		    ip6_dst_lookup(dev_net(netdev), NULL, &rt_dst_entry, &fl6))
			goto neigh;
	...
}

and the ARP table has .family = AF_INET and .key_len = 4, so the lookup
hashes and compares only the first 4 bytes of the IPv6 address.  Can
neigh_create() then install a bogus ARP entry and send an ARP request for
an address made from those 4 bytes, leaving the IPv6 packet-offload SA with
attrs->drop set or an unrelated MAC?  A separate fix using the netns ND
table for AF_INET6 seems needed.

> diff --git a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> index ca30702f8878..551db6417405 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> @@ -235,7 +236,8 @@ void nfp_tunnel_keep_alive(struct nfp_app *app, struct sk_buff *skb)
>  		if (!netdev)
>  			continue;
>  
> -		n = neigh_lookup(&arp_tbl, &ipv4_addr, netdev);
> +		tbl = arp_table(dev_net(netdev));
> +		n = neigh_lookup(tbl, &ipv4_addr, netdev);
>  		if (!n)
>  			continue;
>  

[ ... ]

> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 459f19f7071e..03e932744e3e 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -1841,11 +1841,12 @@ static int vxlan_err_lookup(struct sock *sk, struct sk_buff *skb)
>  
>  static int arp_reduce(struct net_device *dev, struct sk_buff *skb, __be32 vni)
>  {
> +	struct neigh_table *tbl = arp_table(dev_net(dev));
>  	struct vxlan_dev *vxlan = netdev_priv(dev);
> +	struct neighbour *n;
>  	struct arphdr *parp;
>  	u8 *arpptr, *sha;
>  	__be32 sip, tip;
> -	struct neighbour *n;

[ ... ]

> diff --git a/include/net/arp.h b/include/net/arp.h
> index f8d18b1f8b28..e932def63d62 100644
> --- a/include/net/arp.h
> +++ b/include/net/arp.h
> @@ -7,8 +7,6 @@
>  #include <linux/hash.h>
>  #include <net/neighbour.h>
>  
> -extern struct neigh_table arp_tbl;
> -
>  static inline struct neigh_table *arp_table(struct net *net)
>  {
>  	return net->neigh_tables[NEIGH_ARP_TABLE];
> @@ -25,10 +23,12 @@ static inline u32 arp_hashfn(const void *pkey, const struct net_device *dev, u32
>  #ifdef CONFIG_INET
>  static inline struct neighbour *__ipv4_neigh_lookup_noref(struct net_device *dev, u32 key)
>  {
> +	struct neigh_table *tbl = arp_table(dev_net(dev));
> +
>  	if (dev->flags & (IFF_LOOPBACK | IFF_POINTOPOINT))
>  		key = INADDR_ANY;
>  
> -	return ___neigh_lookup_noref(&arp_tbl, neigh_key_eq32, arp_hashfn, &key, dev);
> +	return ___neigh_lookup_noref(tbl, neigh_key_eq32, arp_hashfn, &key, dev);
>  }

[Severity: Low]
Should these new netns reads use dev_net_rcu() instead of dev_net()?

possible_net_t holds a struct net __rcu *net published with
rcu_assign_pointer() via write_pnet(), and the two accessors differ:

include/net/net_namespace.h:
	static inline struct net *read_pnet(const possible_net_t *pnet)
	{
		return rcu_dereference_protected(pnet->net, true);
	}
	...
	static inline struct net *read_pnet_rcu(const possible_net_t *pnet)
	{
		return rcu_dereference(pnet->net);
	}

dev_net() goes through read_pnet(), which asserts unconditionally that the
caller holds RTNL or a device reference, so it silently turns off
lockdep/sparse RCU checking on these new dereferences.

The sites added by this patch that run under rcu_read_lock() only are
__ipv4_neigh_lookup_noref() and ip_neigh_gw4() (reached from
ip_finish_output2()), br_do_proxy_suppress_arp(), the vxlan
arp_reduce()/route_shortcircuit() xmit paths, and nfp_tunnel_keep_alive(),
which is called inside its own rcu_read_lock().  None of them held RTNL or
a netns reference before, and none of them read dev->nd_net at all before
this patch.

The dev_net() uses in inetdev_init(), inetdev_destroy() and arp_ifdown()
look fine since RTNL is held there.

> diff --git a/include/net/route.h b/include/net/route.h
> index 45290177a33c..6b55de2e4df8 100644
> --- a/include/net/route.h
> +++ b/include/net/route.h
> @@ -405,8 +405,11 @@ static inline struct neighbour *ip_neigh_gw4(struct net_device *dev,
>  	struct neighbour *neigh;
>  
>  	neigh = __ipv4_neigh_lookup_noref(dev, (__force u32)daddr);
> -	if (unlikely(!neigh))
> -		neigh = __neigh_create(&arp_tbl, &daddr, dev, false);
> +	if (unlikely(!neigh)) {
> +		struct neigh_table *tbl = arp_table(dev_net(dev));
> +
> +		neigh = __neigh_create(tbl, &daddr, dev, false);
> +	}
>  
>  	return neigh;
>  }
> diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
> index 87a40e2f1c50..92a3abb1e357 100644
> --- a/net/bridge/br_arp_nd_proxy.c
> +++ b/net/bridge/br_arp_nd_proxy.c
> @@ -193,7 +193,7 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
>  		return;
>  	}
>  
> -	n = neigh_lookup(&arp_tbl, &tip, vlandev);
> +	n = neigh_lookup(arp_table(dev_net(vlandev)), &tip, vlandev);
>  	if (n) {
>  		struct net_bridge_fdb_entry *f;
>  		u8 ha[ETH_ALEN] __aligned(2);
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 824c65c70408..a44fa68fdd07 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -149,7 +149,7 @@ static const struct neigh_ops arp_direct_ops = {
>  	.connected_output =	neigh_direct_output,
>  };
>  
> -struct neigh_table arp_tbl = {
> +static struct neigh_table arp_tbl = {
>  	.family		= AF_INET,
>  	.key_len	= 4,
>  	.protocol	= cpu_to_be16(ETH_P_IP),
> @@ -182,7 +182,6 @@ struct neigh_table arp_tbl = {
>  	.gc_thresh2	= 512,
>  	.gc_thresh3	= 1024,
>  };
> -EXPORT_SYMBOL(arp_tbl);

[Severity: Low]
Should Documentation/networking/switchdev.rst be updated in this patch too?

Its Nexthop Resolution section still tells switchdev driver authors to use
the symbol that becomes file-local here, in four places:

  "address discovery comes via the ARP (or ND) process and is available via
   the arp_tbl neighbor table."

  "The driver can monitor for updates to arp_tbl using the netevent notifier
   NETEVENT_NEIGH_UPDATE. ... The driver implements ndo_neigh_destroy to know
   when arp_tbl neighbor entries are purged from the port."

At this commit, git grep arp_tbl matches only net/ipv4/arp.c and
Documentation/networking/switchdev.rst, and the doc points at rocker as the
reference implementation while rocker itself is converted to
arp_table(&init_net) here.  The doc also never mentions arp_table(net) or
the new requirement to pick the table belonging to the device's netns (or
init_net for netns-immutable devices).  The text is still unchanged at the
end of the series (d4be111038a59be6217f9b018ec033041ab7bf2a).

[ ... ]

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