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

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

[Binbin Deng]

The QPN-to-netdev hash table is protected by ht_lock, but the NAPI lookup
path traverses and dereferences nodes without taking that lock.  A child
interface teardown can therefore remove and free the node while the parent
RQ is still processing completions for that child:

CPU0                                    CPU1
mlx5i_pkey_get_netdev()
node = mlx5i_find_qpn_to_netdev_node()
                                        mlx5i_pkey_del_qpn()
                                        node = mlx5i_find_qpn_to_netdev_node()
                                        spin_lock_bh(&ht->ht_lock)
                                        hlist_del_init(&node->hlist)
                                        spin_unlock_bh(&ht->ht_lock)
                                        kfree(node)
read node->netdev or node->hlist.next

At that point CPU0 is reading freed memory.  The race is reachable when a
PKEY child interface goes down while its completions are still queued on
the shared parent RQ; this is the same lifetime case already described by
the RX path comment.  It can lead to a KASAN use-after-free, corruption of
the RX path, or a panic.

Take ht_lock around hash lookups, and keep the deletion lookup and unlink
in one critical section.  The NAPI path copies the netdev pointer while
the lock prevents the mapping node from being freed.

Found during a static-analysis-assisted audit: candidate sites were
surfaced by a Coccinelle rule, and the reader/writer paths were
confirmed by deep code inspection. Due to a lack of hardware
infrastructure, the issue was not observed at runtime.

Fixes: 7e7f4780c340 ("net/mlx5e: IPoIB, Use hash-table to map between QPN to child netdev")
Assisted-by: Coccinelle
Signed-off-by: Binbin Deng <18983559317@163.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib_vlan.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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 4120afd097c3..91b203c6d603 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,10 +129,12 @@ 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;
-
+	spin_unlock_bh(&ipriv->qpn_htbl->ht_lock);
+
 	return node->netdev;
 }
 
-- 
2.43.0

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

[Dragos Tatulea]

On 02.11.26 08:41, Binbin Deng wrote:
> The QPN-to-netdev hash table is protected by ht_lock, but the NAPI lookup
> path traverses and dereferences nodes without taking that lock.  A child
> interface teardown can therefore remove and free the node while the parent
> RQ is still processing completions for that child:
> 
> CPU0                                    CPU1
> mlx5i_pkey_get_netdev()
> node = mlx5i_find_qpn_to_netdev_node()
>                                         mlx5i_pkey_del_qpn()
>                                         node = mlx5i_find_qpn_to_netdev_node()
>                                         spin_lock_bh(&ht->ht_lock)
>                                         hlist_del_init(&node->hlist)
>                                         spin_unlock_bh(&ht->ht_lock)
>                                         kfree(node)
> read node->netdev or node->hlist.next
> 
> At that point CPU0 is reading freed memory.  The race is reachable when a
> PKEY child interface goes down while its completions are still queued on
> the shared parent RQ; this is the same lifetime case already described by
> the RX path comment.  It can lead to a KASAN use-after-free, corruption of
> the RX path, or a panic.
> 
> Take ht_lock around hash lookups, and keep the deletion lookup and unlink
> in one critical section.  The NAPI path copies the netdev pointer while
> the lock prevents the mapping node from being freed.
> 
> Found during a static-analysis-assisted audit: candidate sites were
> surfaced by a Coccinelle rule, and the reader/writer paths were
> confirmed by deep code inspection. Due to a lack of hardware
> infrastructure, the issue was not observed at runtime.
> 
> Fixes: 7e7f4780c340 ("net/mlx5e: IPoIB, Use hash-table to map between QPN to child netdev")
> Assisted-by: Coccinelle
> Signed-off-by: Binbin Deng <18983559317@163.com>
Already WIP by Prathamesh:
https://lore.kernel.org/all/20261031225749.14214-1-prathameshdeshpande7@gmail.com/

Thanks,
Dragos

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 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] The use-after-free fix in mlx5i_pkey_get_netdev()
  (drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib_vlan.c) is…
- [High] The patch adds a spinlock leak and deadlock to
  mlx5i_pkey_get_netdev()…
- [Medium] The patch takes one spinlock, shared by the whole parent, for
  every RX completion in the NAPI fast path.

--

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

This commit takes ht_lock around the QPN-to-netdev hash lookups in
mlx5i_pkey_get_netdev() and mlx5i_pkey_del_qpn(). It also puts the
deletion lookup and the unlink in a single critical section. The aim is
to stop the NAPI RX path from reading a qpn_to_netdev node that a child
PKEY interface teardown has already freed.

> 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

[ ... ]

> @@ -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);

[Severity: Medium]
mlx5i_complete_rx_cqe() calls mlx5i_pkey_get_netdev() once per CQE on
every RX channel. Child interfaces share the parent's table:

mlx5i_pkey_dev_init() {
    ...
	ipriv->qpn_htbl = parent_ipriv->qpn_htbl;
    ...
}

With this change, every received packet on every RX queue of the
parent and its PKEY children takes the same ht_lock. Is a shared
per-packet spinlock what was intended here?

The table is only written when a child is created or destroyed. Would
RCU fit better? That would mean hlist_for_each_entry_rcu() in
mlx5i_find_qpn_to_netdev_node(), hlist_add_head_rcu() and
hlist_del_rcu() under ht_lock in the writers, and kfree_rcu() in place
of kfree(node) in mlx5i_pkey_del_qpn().

NAPI already runs with BH disabled, which counts as an RCU read-side
section. So the spin_lock_bh() in the softirq reader would not be needed
either.

The commit message does not mention this tradeoff.

>  	node = mlx5i_find_qpn_to_netdev_node(ipriv->qpn_htbl->buckets, qpn);
>  	if (!node)
>  		return NULL;

[Severity: High]
Does this early return leave ht_lock held and BH disabled?

The only caller, mlx5i_complete_rx_cqe(), expects lookup misses:

	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)) {

That is the same teardown case this patch is meant to fix. After one
miss, the next CQE on that CPU calls spin_lock_bh() on the same lock and
spins forever.

Other RX channels, mlx5i_pkey_add_qpn() and mlx5i_pkey_del_qpn() would
also block on the shared ht_lock. Before this patch, the same situation
only dropped the skb.

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

[Severity: High]
Can node be freed after spin_unlock_bh() but before node->netdev is
read?

The commit message says:

    The NAPI path copies the netdev pointer while
    the lock prevents the mapping node from being freed.

However, the pointer is read after the lock has been dropped. A child
teardown on another CPU can run in that window:

CPU0                                    CPU1
mlx5i_pkey_get_netdev()
  spin_lock_bh(ht_lock)
  node = mlx5i_find_qpn_to_netdev_node()
  spin_unlock_bh(ht_lock)
                                        mlx5i_dev_cleanup()
                                          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;

The free is not deferred, so the race from the commit message is still
there with a smaller window. mlx5i_complete_rx_cqe() would then use the
stale netdev through mlx5i_epriv(netdev) and
priv->channel_stats[rq->ix].

Would it work to copy the pointer before unlocking? For example:

	ndev = node ? node->netdev : NULL;
	spin_unlock_bh(&ipriv->qpn_htbl->ht_lock);
	return ndev;

That would also fix the early return above. Another option is to convert
the table to RCU and use kfree_rcu().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102074107.32859-1-18983559317%40163.com