Re: [PATCH v7 5/5] 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 4 potential issue(s) to consider:

New issues:
- [Critical] Unconditional release of ST index reference during MR cleanup leads to refcount underflow and WARN_ON_ONCE for ST index 0.
- [High] Pre-allocated FRMR pool handles via `ib_frmr_pools_set_pinned` fail to acquire ST index references, leading to underflow on destruction.

Pre-existing issues:
- [High] Off-by-one array index in `mlx5r_create_mkeys` error path destroys the wrong mkey and leaks successful allocations.
- [High] Hardware mkeys created during pinned pool expansion are leaked if pushing them to the pool queue fails.
--

commit b05f2090a2616db2c2747f4768f18e80acf1a2d2
Author: Zhiping Zhang <zhipingz@meta.com>
Date:   Thu Jun 11 16:11:20 2026 +0000

    RDMA/mlx5: get tph for p2p access when registering dma-buf mr
    
    Query dma-buf TPH metadata when registering a dma-buf MR for peer-to-
    peer access to a PCIe endpoint and use it to program requester-side TPH
    on the outbound mkey. If the exporter has no metadata, fall back to the
    existing no-TPH path.

> diff --git a/drivers/infiniband/core/frmr_pools.c b/drivers/infiniband/core/frmr_pools.c
> index 5e992ff3d7cff..61a77847118e6 100644
> --- a/drivers/infiniband/core/frmr_pools.c
> +++ b/drivers/infiniband/core/frmr_pools.c
> @@ -92,7 +92,8 @@ static void destroy_all_handles_in_queue(struct ib_device *device,
>  	u32 count;
>  
>  	while (pop_frmr_handles_page(pool, queue, &page, &count)) {
> -		pools->pool_ops->destroy_frmrs(device, page->handles, count);
> +		pools->pool_ops->destroy_frmrs(device, &pool->key,
> +					       page->handles, count);
>  		kfree(page);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but can hardware mkeys be permanently leaked
during pinned pool expansion?

If push_handle_to_queue_locked() fails inside ib_frmr_pools_set_pinned():

ib_frmr_pools_set_pinned()
   ret = pools->pool_ops->create_frmrs(device, key, handles, needed_handles);
   for (i = 0; i < needed_handles; i++) {
       ret = push_handle_to_queue_locked(&pool->queue, handles[i]);
       if (ret)
           goto end;
   }

The array is freed, but the un-pushed hardware mkeys (from index i to
needed_handles - 1) do not appear to be cleaned up or tracked anywhere.

[ ... ]

> diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
> index 14c1aec1edbbc..3ca4b36deecfc 100644
> --- a/drivers/infiniband/hw/mlx5/mr.c
> +++ b/drivers/infiniband/hw/mlx5/mr.c

[ ... ]

> @@ -273,7 +304,8 @@ static int mlx5r_create_mkeys(struct ib_device *device, struct ib_frmr_key *key,
>  
>  	st_index = key->kernel_vendor_key &
>  		   MLX5_FRMR_POOLS_KERNEL_KEY_ST_INDEX_MASK;
> -	ph = key->kernel_vendor_key & MLX5_FRMR_POOLS_KERNEL_KEY_PH_MASK;
> +	ph = (key->kernel_vendor_key & MLX5_FRMR_POOLS_KERNEL_KEY_PH_MASK) >>
> +	     MLX5_FRMR_POOLS_KERNEL_KEY_PH_SHIFT;

[Severity: High]
This is a pre-existing issue, but does the error path here destroy the wrong
mkey and leak the successful allocations?

If mlx5_core_create_mkey() fails at loop index i, the code jumps to free_in:

free_in:
    kfree(in);
    if (err)
        for (; i > 0; i--)
            mlx5_core_destroy_mkey(dev->mdev, handles[i]);

Because the loop evaluates i > 0, executes for handles[i], and then
decrements, it will destroy the handle at index i (which just failed and is
invalid) and terminate before reaching index 0. Are successfully created
handles (like handles[0]) silently leaked?

[ ... ]

> @@ -311,6 +344,9 @@ static void mlx5r_destroy_mkeys(struct ib_device *device, u32 *handles,
>  			pr_warn_ratelimited(
>  				"mlx5_ib: failed to destroy mkey %d: %d",
>  				handles[i], err);
> +		else
> +			mlx5_ib_put_frmr_st_handle_ref(dev,
> +						       key->kernel_vendor_key);
>  	}
>  }

[Severity: High]
Does this improperly drop ST index references for pre-allocated pool handles?

Pre-allocated FRMR pool handles via ib_frmr_pools_set_pinned() directly call
create_frmrs() to program the hardware but do not appear to acquire the ST
index reference for them. When these handles are later destroyed during
pool aging or teardown, dropping the reference unconditionally here could
result in a deterministic refcount underflow for whichever ST index is
encoded in the key.

[ ... ]

> @@ -1419,6 +1520,8 @@ static int mlx5r_handle_mkey_cleanup(struct mlx5_ib_mr *mr)
>  		dma_resv_unlock(
>  			to_ib_umem_dmabuf(mr->umem)->attach->dmabuf->resv);
>  	}
> +	if (!ret)
> +		mlx5_ib_mr_put_frmr_st_handle_ref(mr);
>  	return ret;
>  }

[Severity: Critical]
Could this unconditional release cause a refcount underflow and WARN_ON_ONCE
for ST index 0?

In mlx5_mr_cache_alloc(), kernel_vendor_key is explicitly initialized to 0.
Similarly, MRs created via reg_create() remain with 0 from kzalloc, and no
ST reference is ever acquired. 

When these MRs are deregistered and hit this cleanup path,
mlx5_ib_put_frmr_st_handle_ref() extracts 0 and forwards it to
mlx5_st_dealloc_index(). Since 0 does not match the sentinel
MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX (which is 0xFFFF), it causes an
unbalanced refcount_dec_and_test() for ST index 0. Can this corrupt
legitimate PCIe endpoint steering tags?

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