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