Re: [PATCH v3 2/2] RDMA/mlx5: get tph for p2p access when registering dma-buf mr

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Use-after-handoff / Race condition of MR object after pushing to FRMR pool
- [High] Memory leak of `mlx5_st_idx_data` in `mlx5_st_dealloc_index()`
--

commit 2db4c0c9ad5981da4be78d996f10a6b611b880e0
Author: Zhiping Zhang <zhipingz@meta.com>
RDMA/mlx5: get tph for p2p access when registering dma-buf mr

This commit queries dma-buf TPH metadata when registering a dma-buf MR for
peer-to-peer access and translates the raw steering tag into an mlx5 ST
index. It also tracks the per-MR ownership of the allocated mlx5 ST index
on the MR and releases it once the firmware mkey no longer references it.

> diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
> index 3b6da45061a55..84d570f7cafb9 100644
> --- a/drivers/infiniband/hw/mlx5/mr.c
> +++ b/drivers/infiniband/hw/mlx5/mr.c
[ ... ]
> @@ -1378,8 +1439,15 @@ static int mlx5r_handle_mkey_cleanup(struct mlx5_ib_mr *mr)
>  	int ret;
>  
>  	if (mr->ibmr.frmr.pool && !mlx5_umr_revoke_mr_with_lock(mr) &&
> -	    !ib_frmr_pool_push(mr->ibmr.device, &mr->ibmr))
> +	    !ib_frmr_pool_push(mr->ibmr.device, &mr->ibmr)) {
> +		/*
> +		 * The mkey has been revoked: firmware no longer references
> +		 * dmabuf_st_index, so release it before this mr re-enters
> +		 * the FRMR cache for reuse by another registration.
> +		 */
> +		mlx5_ib_mr_put_dmabuf_st(mr);
>  		return 0;
> +	}

Does this introduce a race condition with MR reuse?

If ib_frmr_pool_push() succeeds, the MR is immediately inserted into the
FRMR pool and becomes available for concurrent reuse by other threads.
However, the boolean short-circuit evaluation causes
mlx5_ib_mr_put_dmabuf_st(mr) to execute after the push completes.

If another thread pops the MR and begins reinitialization before
mlx5_ib_mr_put_dmabuf_st(mr) executes, could the original thread read and
modify the newly owned MR state concurrently? This seems like it could
result in freeing the wrong steering tag index and corrupting the MR state.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/st.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/st.c
> index 997be91f0a135..c5058557c7f03 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/st.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/st.c
[ ... ]
> @@ -152,6 +147,20 @@ int mlx5_st_alloc_index(struct mlx5_core_dev *dev, enum tph_mem_type mem_type,
>  	mutex_unlock(&st->lock);
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(mlx5_st_alloc_index_by_tag);
> +
> +int mlx5_st_alloc_index(struct mlx5_core_dev *dev, enum tph_mem_type mem_type,
> +			unsigned int cpu_uid, u16 *st_index)
> +{
> +	u16 tag;
> +	int ret;
> +
> +	ret = pcie_tph_get_cpu_st(dev->pdev, mem_type, cpu_uid, &tag);
> +	if (ret)
> +		return ret;
> +
> +	return mlx5_st_alloc_index_by_tag(dev, tag, st_index);
> +}
>  EXPORT_SYMBOL_GPL(mlx5_st_alloc_index);
>  
>  int mlx5_st_dealloc_index(struct mlx5_core_dev *dev, u16 st_index)

While looking at these changes which tie the ST allocation to the MR
lifecycle, it appears there is a pre-existing memory leak in
mlx5_st_dealloc_index():

drivers/net/ethernet/mellanox/mlx5/core/lib/st.c:mlx5_st_dealloc_index() {
	if (refcount_dec_and_test(&idx_data->usecount)) {
		xa_erase(&st->idx_xa, st_index);
	}
}

When the reference count drops to 0, xa_erase() is called to remove the
element from the XArray, but the underlying pointer is never freed with
kfree().

Because this patch introduces per-MR allocation and deallocation of ST
indices tied to the lifetime of dma-buf MRs, could a user space application
repeatedly register and deregister dmabuf MRs to silently leak kernel
memory and eventually exhaust system memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512184755.4137227-1-zhipingz@meta.com?part=2