Re: [PATCH v1 RESEND] vdpa/mlx5: Don't free QP buffers if destroying the QP fails

[Dragos Tatulea <dtatulea@nvidia.com>]

Hi,

Thanks for your patch and sorry about the late reply.

On 16.10.26 05:58, Yuho Choi wrote:
> qp_destroy() only warns when DESTROY_QP fails and then frees the RQ
> buffer and doorbell record of the driver-owned QP anyway. The QP still
> exists in firmware, so the device can keep reading WQEs and the doorbell
> record from memory that has been returned to the allocator. This is
> reachable from teardown_vq(), which destroys the QPs even when
> destroy_virtqueue() fails and the virtqueue still uses them.
> 
> Return after the warning, as cq_destroy() does, and leak the buffers
> instead. A device in internal error is not affected, since the mlx5 core
> reports success for DESTROY_QP in that state.
> 
This commit message is long and hard to read. Could you make it
shorter and more readable please?

> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> Cc: stable@vger.kernel.org
Nothing against the change. But it does not deserve a fixes tag.

> Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
> ---
> No code changes, only added Cc: stable. Resending as the original posting
> got no comments:
> https://lore.kernel.org/all/20260926215632.1343807-1-oss.patchbox@gmail.com/
> 
> This was found with a static analysis tool from ongoing, not yet
> published research, and confirmed by code inspection. It has not been
> reproduced on hardware.
> 
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 8563fec2855d..a01cd890bb58 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -483,8 +483,11 @@ static void qp_destroy(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_qp *vqp)
>  	MLX5_SET(destroy_qp_in, in, opcode, MLX5_CMD_OP_DESTROY_QP);
>  	MLX5_SET(destroy_qp_in, in, qpn, vqp->mqp.qpn);
>  	MLX5_SET(destroy_qp_in, in, uid, ndev->mvdev.res.uid);
> -	if (mlx5_cmd_exec_in(ndev->mvdev.mdev, destroy_qp, in))
> +	if (mlx5_cmd_exec_in(ndev->mvdev.mdev, destroy_qp, in)) {
> +		/* The QP still owns its buffers */
>  		mlx5_vdpa_warn(&ndev->mvdev, "destroy qp 0x%x\n", vqp->mqp.qpn);
While here, could you improve the error message please?
mlx5_vdpa_warn(..., "destroy %s qp 0x%x failed %s\n", vqp->fw ? "fw" : "sw");

Thanks,
Dragos