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 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Off-by-one error in mlx5r_create_mkeys error handling path causes hardware mkey leak and invalid destroy.
- [High] Hardware mkeys are leaked in `ib_frmr_pools_set_pinned` if pushing a handle to the pool's queue fails.
--

commit 08f720e58b52ed548a72c452a9bd15258e2e9264
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, falling back to no-TPH if absent. It also transfers
the ST-table reference ownership to the hardware mkey handle rather than
the transient MR object, and updates the pool API to handle this transfer.

> diff --git a/drivers/infiniband/core/frmr_pools.c b/drivers/infiniband/core/frmr_pools.c
> --- a/drivers/infiniband/core/frmr_pools.c
> +++ b/drivers/infiniband/core/frmr_pools.c
[ ... ]
> @@ -430,16 +430,16 @@ int ib_frmr_pools_set_pinned(struct ib_device *device, struct ib_frmr_key *key,
>  	ret = pools->pool_ops->create_frmrs(device, key, handles,
>  					    needed_handles);
>  	if (ret) {
>  		kfree(handles);
>  		return ret;
>  	}
>  
>  	spin_lock(&pool->lock);
>  	for (i = 0; i < needed_handles; i++) {
>  		ret = push_handle_to_queue_locked(&pool->queue,
>  						  handles[i]);
>  		if (ret)
>  			goto end;
>  	}
>  
>  end:
>  	spin_unlock(&pool->lock);
>  	kfree(handles);

[Severity: High]
This is a pre-existing issue, but can this error path leak the successfully
created hardware mkeys?

In ib_frmr_pools_set_pinned(), up to needed_handles hardware mkeys are
successfully created via create_frmrs(). If push_handle_to_queue_locked()
fails, the code jumps to the end: label and frees the handles array.

However, the mkeys that were successfully created but not yet pushed don't
appear to be explicitly destroyed here. Could this lead to a bulk resource
leak if queue page allocation fails?

> diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
> --- a/drivers/infiniband/hw/mlx5/mr.c
> +++ b/drivers/infiniband/hw/mlx5/mr.c
[ ... ]
> @@ -335,16 +335,16 @@ static int mlx5r_create_mkeys(struct ib_device *device, struct ib_frmr_key *key,
>  	for (i = 0; i < count; i++) {
>  		assign_mkey_variant(dev, handles + i, in);
>  		err = mlx5_core_create_mkey(dev->mdev, handles + i, in, inlen);
>  		if (err)
>  			goto free_in;
>  	}
>  free_in:
>  	kfree(in);
>  	if (err)
>  		for (; i > 0; i--)
>  			mlx5_core_destroy_mkey(dev->mdev, handles[i]);

[Severity: Critical]
This is a pre-existing issue, but does this rollback loop properly clean up
the successfully created mkeys?

When mlx5_core_create_mkey() fails at index i, the rollback loop 
for (; i > 0; i--) attempts to destroy handles[i]. Because the creation
failed, handles[i] still contains the unallocated index preset by
assign_mkey_variant().

Executing mlx5_core_destroy_mkey() on this value could send a destroy
command for an unowned mkey, potentially causing hardware state corruption.

Additionally, since the loop stops at i > 0, does it permanently leak the
successfully created mkey at handles[0]?

>  	return err;
>  }

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