Re: [PATCH net] net/mlx5: IPOIB, Fix QPN lookup race

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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] The fix closes the use-after-free by putting
  spin_lock_bh()/spin_unlock_bh() on one device-wide lock, ht_lock, in…

--

net/mlx5: IPOIB, Fix QPN lookup race

This commit adds ht_lock around the QPN hash table lookup in
mlx5i_pkey_get_netdev(). Without the lock, the RX path can walk a bucket
while mlx5i_pkey_del_qpn() removes and frees a node. With the lock held,
node->netdev is read before the unlock.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib_vlan.c b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib_vlan.c
> index 4120afd097c36..6c24ae8e76621 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib_vlan.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib_vlan.c
> @@ -126,13 +126,17 @@ int mlx5i_pkey_del_qpn(struct net_device *netdev, u32 qpn)
>  struct net_device *mlx5i_pkey_get_netdev(struct net_device *netdev, u32 qpn)
>  {
>  	struct mlx5i_priv *ipriv = netdev_priv(netdev);
> +	struct mlx5i_pkey_qpn_ht *ht = ipriv->qpn_htbl;
> +	struct net_device *result = NULL;
>  	struct qpn_to_netdev *node;
>  
> -	node = mlx5i_find_qpn_to_netdev_node(ipriv->qpn_htbl->buckets, qpn);
> -	if (!node)
> -		return NULL;
> +	spin_lock_bh(&ht->ht_lock);
> +	node = mlx5i_find_qpn_to_netdev_node(ht->buckets, qpn);
> +	if (node)
> +		result = node->netdev;
> +	spin_unlock_bh(&ht->ht_lock);

[Severity: High]
Does this add a device-wide spinlock to the RX datapath for every received
IPoIB packet?

mlx5i_pkey_get_netdev() is called once per RX CQE from NAPI:

mlx5i_handle_rx_cqe()
  -> mlx5i_complete_rx_cqe()
       netdev = mlx5i_pkey_get_netdev(rq->netdev, qpn);

The hash table is allocated once per parent and shared with every PKEY
child in mlx5i_pkey_dev_init():

	ipriv->qpn_htbl = parent_ipriv->qpn_htbl;

The parent's own QPN is also added to the table through
mlx5i_dev_init()->mlx5i_pkey_add_qpn(). So even a plain parent interface
with no children now takes the same ht_lock on every packet, on every RX
channel and CPU. The lock's cacheline would bounce between all the NAPI
CPUs under load.

The only writers are mlx5i_pkey_add_qpn() (from ndo_init via
mlx5i_dev_init()) and mlx5i_pkey_del_qpn() (from ndo_uninit via
mlx5i_dev_cleanup()). Both already run under RTNL. The NAPI readers run in
BH context, which is already an RCU read-side section.

Would an RCU based lookup close the same use-after-free without putting a
lock on the datapath? That would use hlist_add_head_rcu() in
mlx5i_pkey_add_qpn(), hlist_del_rcu() plus kfree_rcu() in
mlx5i_pkey_del_qpn(), and hlist_for_each_entry_rcu() in
mlx5i_find_qpn_to_netdev_node(). It would replace the reader-side locking
added here rather than build on it.

The commit message also does not mention the per-packet locking cost.

>  
> -	return node->netdev;
> +	return result;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030002241.8508-1-prathameshdeshpande7%40gmail.com