Re: [PATCH v1 net-next 7/8] ipv4: Use ipv4_neigh_lookup() and friends.

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] mlxsw: kernel panic via NULL pointer dereference in hw neighbour
  events

Pre-existing issues:
- [Medium] In mlx5e_ipsec_init_macs()
  (drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c), the…

--

ipv4: Use ipv4_neigh_lookup() and friends.

This commit replaces open-coded neigh_lookup(), neigh_create(),
__neigh_lookup() and __neigh_lookup_errno() calls on the ARP table with
the ipv4_neigh_lookup() and ipv4_neigh_create() helpers. Callers no
longer need to fetch the netns and neigh_table themselves. It touches
several drivers as well as net/ipv4 and the bridge.

> 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 37a8ddee3ea1e..16edd3263aedf 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c

[ ... ]

> @@ -365,10 +364,9 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
>  	return;
>  
>  neigh:
> -	tbl = arp_table(dev_net(netdev));
> -	n = neigh_lookup(tbl, pkey, netdev);
> +	n = ipv4_neigh_lookup(netdev, pkey);
>  	if (!n) {
> -		n = neigh_create(tbl, pkey, netdev);
> +		n = ipv4_neigh_create(netdev, pkey);

[Severity: Medium]
This patch didn't introduce this issue, but the new helper names make
it easier to see. Can this fallback put IPv6 keys into the ARP table?

For AF_INET6 SAs, pkey points at the 16-byte addrs->saddr.a6 or
addrs->daddr.a6. Earlier in mlx5e_ipsec_init_macs(), the IPv6 case
jumps here when CONFIG_IPV6 is disabled or the route lookup fails:

	case AF_INET6:
		if (!IS_ENABLED(CONFIG_IPV6) ||
		    ip6_dst_lookup(dev_net(netdev), NULL, &rt_dst_entry, &fl6))
			goto neigh;

The shared path also jumps here when dst_neigh_lookup() returns NULL.

ipv4_neigh_lookup() and ipv4_neigh_create() always use
arp_table(dev_net(dev)), and its key length is 4. That means the first
32 bits of the IPv6 address are treated as an IPv4 address:

- On a miss, an ARP entry for an unrelated IPv4 address is created on
  netdev, neigh_event_send() sends an ARP request for it, and
  attrs->drop is set.

- On a hit, neigh_ha_snapshot() copies the MAC of an unrelated IPv4
  neighbour into the IPv6 SA's dmac.

The old code used the ARP table here too. The patch only changes how
the table is obtained.

Should this fallback check addrs->family and use ipv6_neigh_lookup()
and ipv6_neigh_create() for AF_INET6, or skip the neighbour fallback
for IPv6?

>  		if (IS_ERR(n))
>  			return;
>  		neigh_event_send(n, NULL);

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> @@ -2432,10 +2430,9 @@ static void mlxsw_sp_router_neigh_ent_ipv4_process(struct mlxsw_sp *mlxsw_sp,
>  		return;
>  	}
>  
> -	tbl = arp_table(net);
>  	dipn = htonl(dip);
>  	dev = mlxsw_sp_rif_dev(mlxsw_sp->router->rifs[rif]);
> -	n = neigh_lookup(tbl, &dipn, dev);
> +	n = ipv4_neigh_lookup(dev, &dipn);
>  	if (!n)
>  		return;
>  

[Severity: High]
Can dev be NULL here?

mlxsw_sp_rif_dev() returns NULL when the RIF has no crif. That is the
case for the IP-in-IP underlay RIFs, which mlxsw_sp_ul_rif_create()
allocates without a crif. The only check above is that
mlxsw_sp->router->rifs[rif] is non-NULL. If the device reports
neighbour activity on such a RIF through RAUHTD, the delayed neighbour
update work reaches this point with dev == NULL.

Before this patch that case was harmless. The netns came from
mlxsw_sp_net(mlxsw_sp), not from dev, and neigh_lookup() with a NULL
dev simply matched nothing and returned NULL.

ipv4_neigh_lookup() now derives the table itself via
arp_table(dev_net(dev)). That dereferences the NULL dev, so the kernel
oopses in the router's periodic neighbour work instead of skipping the
entry. Routed IP-in-IP setups on Spectrum would hit this during
normal traffic.

The IPv6 counterpart, mlxsw_sp_router_neigh_ent_ipv6_process(),
passes the same possibly-NULL dev to ipv6_neigh_lookup(). It looks
like it has the same problem.

Should both functions return early when dev is NULL, before the
lookup? Or should the new helpers tolerate a NULL dev the way
neigh_lookup() effectively did? Checking in the callers seems simpler
and keeps the helpers strict.

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