Re: [PATCH v1] net/mlx5e: IPoIB: Fix race in QPN to netdev mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] net/mlx5e: use-after-free of node in mlx5i_pkey_get_netdev() after unlock
- [High] net/mlx5e: missing spin_unlock_bh() on error in mlx5i_pkey_get_netdev()
--

commit ce671ddaa82c0cc9dedd5f6d773ead36eb9647fc
Author: Binbin Deng <18983559317@163.com>

net/mlx5e: IPoIB: Fix race in QPN to netdev mapping

This commit adds spin_lock_bh(&ht->ht_lock) protection around QPN-to-netdev
hash table lookups in mlx5i_pkey_del_qpn() and mlx5i_pkey_get_netdev() to
prevent concurrent node removal and freeing during NAPI RX processing.

> 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..8aea23e0183b7 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib_vlan.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib_vlan.c
> @@ -109,13 +109,14 @@ int mlx5i_pkey_del_qpn(struct net_device *netdev, u32 qpn)
>  	struct mlx5i_pkey_qpn_ht *ht = ipriv->qpn_htbl;
>  	struct qpn_to_netdev *node;
>  
> +	spin_lock_bh(&ht->ht_lock);
>  	node = mlx5i_find_qpn_to_netdev_node(ht->buckets, qpn);
>  	if (!node) {
>  		mlx5_core_warn(epriv->mdev, "QPN to netdev delete from HT failed\n");
> +		spin_unlock_bh(&ht->ht_lock);
>  		return -EINVAL;
>  	}
>  
> -	spin_lock_bh(&ht->ht_lock);
>  	hlist_del_init(&node->hlist);
>  	spin_unlock_bh(&ht->ht_lock);
>  	kfree(node);
> @@ -128,9 +129,11 @@ struct net_device *mlx5i_pkey_get_netdev(struct net_device *netdev, u32 qpn)
>  	struct mlx5i_priv *ipriv = netdev_priv(netdev);
>  	struct qpn_to_netdev *node;
>  
> +	spin_lock_bh(&ipriv->qpn_htbl->ht_lock);
>  	node = mlx5i_find_qpn_to_netdev_node(ipriv->qpn_htbl->buckets, qpn);
>  	if (!node)
>  		return NULL;

[Severity: High]
Does this early return in mlx5i_pkey_get_netdev() in
drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib_vlan.c leave
ipriv->qpn_htbl->ht_lock held and bottom halves disabled when
mlx5i_find_qpn_to_netdev_node() returns NULL?

During NAPI RX completion processing in
drivers/net/ethernet/mellanox/mlx5/core/en_rx.c:mlx5i_complete_rx_cqe(),
a child interface going down can still have unprocessed CQEs queued on the
shared parent RQ:

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

	/* No mapping present, cannot process SKB. This might happen if a child
	 * interface is going down while having unprocessed CQEs on parent RQ
	 */
	if (unlikely(!netdev)) {

When that happens, mlx5i_find_qpn_to_netdev_node() returns NULL and
mlx5i_pkey_get_netdev() returns without calling
spin_unlock_bh(&ipriv->qpn_htbl->ht_lock), which will deadlock on the next
attempt to acquire ht_lock.

> +	spin_unlock_bh(&ipriv->qpn_htbl->ht_lock);
>  
>  	return node->netdev;
>  }

[Severity: Critical]
Can dereferencing node->netdev after releasing ipriv->qpn_htbl->ht_lock in
mlx5i_pkey_get_netdev() in
drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib_vlan.c still trigger a
use-after-free of node?

Once spin_unlock_bh(&ipriv->qpn_htbl->ht_lock) is called, a concurrent
mlx5i_pkey_del_qpn() on another CPU can acquire ht->ht_lock, unlink node,
and free it before mlx5i_pkey_get_netdev() reads node->netdev:

CPU0                                    CPU1
mlx5i_pkey_get_netdev()
  spin_lock_bh(&ht->ht_lock)
  node = mlx5i_find_qpn_to_netdev_node()
  spin_unlock_bh(&ht->ht_lock)
                                        mlx5i_pkey_del_qpn()
                                          spin_lock_bh(&ht->ht_lock)
                                          hlist_del_init(&node->hlist)
                                          spin_unlock_bh(&ht->ht_lock)
                                          kfree(node)
  return node->netdev;

Should mlx5i_pkey_get_netdev() read node->netdev into a local variable
before calling spin_unlock_bh(&ipriv->qpn_htbl->ht_lock)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102074107.32859-1-18983559317@163.com?part=1