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

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

[Prathamesh Deshpande]

mlx5i_pkey_get_netdev() reads the QPN hash table without ht_lock,
while mlx5i_pkey_del_qpn() removes entries under that lock and then
frees them.

PKEY child interfaces share the parent's RX resources, so RX lookups
can overlap child removal and access a freed entry. A lookup for
another QPN in the same bucket can also be affected.

Hold ht_lock during lookup and save node->netdev before unlocking.

Fixes: 7e7f4780c340 ("net/mlx5e: IPoIB, Use hash-table to map between QPN to child netdev")
Signed-off-by: Prathamesh Deshpande <prathameshdeshpande7@gmail.com>
---
 .../ethernet/mellanox/mlx5/core/ipoib/ipoib_vlan.c   | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 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..6c24ae8e7662 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);
 
-	return node->netdev;
+	return result;
 }
 
 static int mlx5i_pkey_open(struct net_device *netdev);
-- 
2.43.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Dragos Tatulea]

On 30.10.26 01:22, Prathamesh Deshpande wrote:
> mlx5i_pkey_get_netdev() reads the QPN hash table without ht_lock,
> while mlx5i_pkey_del_qpn() removes entries under that lock and then
> frees them.
> 
> PKEY child interfaces share the parent's RX resources, so RX lookups
> can overlap child removal and access a freed entry. A lookup for
> another QPN in the same bucket can also be affected.
>
I don't think that all these cases can happen. But I can see how a
CQE completion on the parent can read a pointer from the hash
bucket that just got deleted.

Please improve the commit message.

> Hold ht_lock during lookup and save node->netdev before unlocking.
> 
Holding the spinlock in the datapath is not a good idea. AFAIU
using hlist RCU variants will be enough.

Thanks,
Dragos

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

[Prathamesh Deshpande]

On Fri, 30 Oct 2026 12:18:59 +0100, Dragos Tatulea wrote:
> I don't think that all these cases can happen. But I can see how a
> CQE completion on the parent can read a pointer from the hash
> bucket that just got deleted.
>
> Please improve the commit message.

Thanks for reviewing. I will narrow the commit message to the parent CQE
lookup racing with hash-entry removal.

> Holding the spinlock in the datapath is not a good idea. AFAIU
> using hlist RCU variants will be enough.

I will switch to RCU hlist operations with deferred freeing and send v2
after validation.

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