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

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

[Yuho Choi]

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.

Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
Cc: stable@vger.kernel.org
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);
+		return;
+	}
 	if (!vqp->fw) {
 		mlx5_db_free(ndev->mvdev.mdev, &vqp->db);
 		rq_buf_free(ndev, vqp);

base-commit: fd179f8a05be3ccae366b9b96e176b51fbe54aab
-- 
2.43.0

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

[sashiko-bot]

> 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.
> 
> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016035851.771496-1-oss.patchbox@gmail.com?part=1

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

[Dragos Tatulea]

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