Skip to contentSkip to conversation

vdpa/mlx5: Don't free QP buffers if destroying the QP fails

PATCH RESEND Superseded Yuho Choi 4 participants 5 messages Oct 16 – Nov 2

Subsystems (3)
Stable branch virtio core Mellanox MLX5 vdpa driver

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Yuho Choi

author
#0 lore raw

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

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 [email protected]
Signed-off-by Yuho Choi <[email protected]>

---
No code changes, only added Cc: stable. Resending as the original posting got no comments:
https://lore.kernel.org/all/[email protected]/

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(-)
drivers/vdpa/mlx5/net/mlx5_vnet.c +4 -1
index 8563fec2855d..a01cd890bb58 100644
@@ -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

Signature · 1 line
2.43.0
AI review
#1 lore raw
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 7 lines

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.

Quoted 7 lines

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 [email protected]
Signed-off-by Yuho Choi <[email protected]>

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

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
maintainer
#2 lore raw
In reply to Yuho Choi

Hi,

Thanks for your patch and sorry about the late reply.

Quoted 11 lines · 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.

This commit message is long and hard to read. Could you make it shorter and more readable please?

Quoted 2 lines
Fixes 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
Cc [email protected]

Nothing against the change. But it does not deserve a fixes tag.

Quoted 18 lines
Signed-off-by Yuho Choi <[email protected]>

---
No code changes, only added Cc: stable. Resending as the original posting got no comments:
https://lore.kernel.org/all/[email protected]/

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(-)
drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0
index 8563fec2855d..a01cd890bb58 100644

drivers/vdpa/mlx5/net/mlx5_vnet.c static void qp_destroy(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_qp *vqp) · line 488

Quoted 7 lines
drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -1
@@ -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

author
#3 lore raw
Quoted 11 lines · Dragos Tatulea

This commit message is long and hard to read. Could you make it shorter and more readable please?

Quoted 2 lines
Fixes 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
Cc [email protected]

Nothing against the change. But it does not deserve a fixes tag.

Quoted 2 lines

+ /* 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 for the review, Dragos. I'll send a v2 with a shorter commit message, the improved warning, and without the Fixes and stable tags.

maintainer
#4 lore raw
Quoted 8 lines · Yuho Choi
Quoted 7 lines · Dragos Tatulea

This commit message is long and hard to read. Could you make it shorter and more readable please?

Quoted 2 lines
Fixes 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
Cc [email protected]

Nothing against the change. But it does not deserve a fixes tag.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

+ /* 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 for the review, Dragos. I'll send a v2 with a shorter commit message, the improved warning, and without the Fixes and stable tags.

didn't get a v2