[PATCH net-next v1 0/4] net/mlx5: Don't free CQs whose destroy failed

[PATCH net-next v1 0/4] net/mlx5: Don't free CQs whose destroy failed

[Yuho Choi]

mlx5_core_destroy_cq() can fail, and when it does the CQ may still
exist in firmware with access to its buffer and doorbell record. It
also returns early in that case, without waiting for EQ handlers that
may already hold a reference to the CQ. The four helpers in this series
ignore that error and free the CQ buffer anyway, while their teardown
paths also free the object that embeds the struct mlx5_core_cq.

Keep the buffer and the object that embeds the CQ when DESTROY_CQ
fails, as the regular mlx5_ib CQ destroy path does. The HWS send ring
CQs are polled and never armed, so the HWS patch keeps only their
buffer rather than passing the error up through the queue open and
close paths.

The issues were found with a static analysis tool from ongoing, not yet
published research, and confirmed by code inspection. None of them was
reproduced, so this targets net-next; I can resend for net if you
prefer.

Compile-tested only (x86_64 defconfig with MLX5_CORE_EN, MLX5_ESWITCH,
MLX5_SW_STEERING and MLX5_HW_STEERING, W=1).

Yuho Choi (4):
  net/mlx5: Don't free the ASO CQ if destroying it fails
  net/mlx5: Don't free the WC test CQ if destroying it fails
  net/mlx5: DR, Don't free the send ring CQ if destroying it fails
  net/mlx5: HWS, Don't free the send ring CQ buffer if destroying the CQ
    fails

 .../net/ethernet/mellanox/mlx5/core/lib/aso.c  | 18 +++++++++++++-----
 .../mellanox/mlx5/core/steering/hws/send.c     |  4 +++-
 .../mellanox/mlx5/core/steering/sws/dr_send.c  |  4 +++-
 drivers/net/ethernet/mellanox/mlx5/core/wc.c   | 17 ++++++++++++-----
 4 files changed, 31 insertions(+), 12 deletions(-)


base-commit: 2f2f7eeef8b0a1a9b9ca998231419873ccae8d4e
-- 
2.43.0

[PATCH net-next v1 1/4] net/mlx5: Don't free the ASO CQ if destroying it fails

[Yuho Choi]

mlx5_aso_destroy_cq() ignores the result of mlx5_core_destroy_cq() and
frees the CQ buffer and doorbell record anyway, and its callers then
free the struct mlx5_aso that embeds the CQ. If DESTROY_CQ fails, the
CQ may still exist in firmware and retain access to its buffer and
doorbell record. mlx5_core_destroy_cq() also returns early in that case,
without waiting for EQ handlers that may already hold a reference to
the CQ.

Return the error from mlx5_aso_destroy_cq() and keep both the buffer
and the struct mlx5_aso when it fails, as the regular mlx5_ib CQ destroy
path does. A device in internal error is not affected, since the mlx5
core reports success for DESTROY_CQ in that state.

Fixes: cdd04f4d4d71 ("net/mlx5: Add support to create SQ and CQ for ASO")
Assisted-by: LLM
Signed-off-by: Yuho Choi <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.

 .../net/ethernet/mellanox/mlx5/core/lib/aso.c  | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c
index 614cd57a40d74..6a9d31d539e9a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c
@@ -112,10 +112,14 @@ static int create_aso_cq(struct mlx5_aso_cq *cq, void *cqc_data)
 	return err;
 }
 
-static void mlx5_aso_destroy_cq(struct mlx5_aso_cq *cq)
+static int mlx5_aso_destroy_cq(struct mlx5_aso_cq *cq)
 {
-	mlx5_core_destroy_cq(cq->mdev, &cq->mcq);
-	mlx5_wq_destroy(&cq->wq_ctrl);
+	int err = mlx5_core_destroy_cq(cq->mdev, &cq->mcq);
+
+	/* The device may still access an undestroyed CQ's buffer; keep it. */
+	if (!err)
+		mlx5_wq_destroy(&cq->wq_ctrl);
+	return err;
 }
 
 static int mlx5_aso_create_cq(struct mlx5_core_dev *mdev, int numa_node,
@@ -326,7 +330,9 @@ struct mlx5_aso *mlx5_aso_create(struct mlx5_core_dev *mdev, u32 pdn)
 	return aso;
 
 err_sq:
-	mlx5_aso_destroy_cq(&aso->cq);
+	/* An undestroyed CQ may still be in use; keep the ASO that holds it. */
+	if (mlx5_aso_destroy_cq(&aso->cq))
+		return ERR_PTR(err);
 err_cq:
 	kfree(aso);
 	return ERR_PTR(err);
@@ -335,7 +341,9 @@ struct mlx5_aso *mlx5_aso_create(struct mlx5_core_dev *mdev, u32 pdn)
 void mlx5_aso_destroy(struct mlx5_aso *aso)
 {
 	mlx5_aso_destroy_sq(aso);
-	mlx5_aso_destroy_cq(&aso->cq);
+	/* An undestroyed CQ may still be in use; keep the ASO that holds it. */
+	if (mlx5_aso_destroy_cq(&aso->cq))
+		return;
 	kfree(aso);
 }
 
-- 
2.43.0

[PATCH net-next v1 2/4] net/mlx5: Don't free the WC test CQ if destroying it fails

[Yuho Choi]

mlx5_wc_destroy_cq() ignores the result of mlx5_core_destroy_cq() and
frees the CQ buffer and doorbell record anyway, and mlx5_core_test_wc()
then frees the struct mlx5_wc_sq that embeds the CQ. If DESTROY_CQ
fails, the CQ may still exist in firmware and retain access to its
buffer and doorbell record. mlx5_core_destroy_cq() also returns early in
that case, without waiting for EQ handlers that may already hold a
reference to the CQ.

Return the error from mlx5_wc_destroy_cq() and keep both the buffer and
the struct mlx5_wc_sq when it fails, as the regular mlx5_ib CQ destroy
path does. A device in internal error is not affected, since the mlx5
core reports success for DESTROY_CQ in that state.

Fixes: d98995b4bf98 ("net/mlx5: Reimplement write combining test")
Assisted-by: LLM
Signed-off-by: Yuho Choi <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/net/ethernet/mellanox/mlx5/core/wc.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/wc.c b/drivers/net/ethernet/mellanox/mlx5/core/wc.c
index 7d3d4d739e2cd..df1b1555ed8f4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/wc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/wc.c
@@ -148,10 +148,14 @@ static int mlx5_wc_create_cq(struct mlx5_core_dev *mdev, struct mlx5_wc_cq *cq)
 	return err;
 }
 
-static void mlx5_wc_destroy_cq(struct mlx5_wc_cq *cq)
+static int mlx5_wc_destroy_cq(struct mlx5_wc_cq *cq)
 {
-	mlx5_core_destroy_cq(cq->mdev, &cq->mcq);
-	mlx5_wq_destroy(&cq->wq_ctrl);
+	int err = mlx5_core_destroy_cq(cq->mdev, &cq->mcq);
+
+	/* The device may still access an undestroyed CQ's buffer; keep it. */
+	if (!err)
+		mlx5_wq_destroy(&cq->wq_ctrl);
+	return err;
 }
 
 static int create_wc_sq(struct mlx5_core_dev *mdev, void *sqc_data,
@@ -360,6 +364,7 @@ static int mlx5_wc_poll_cq(struct mlx5_wc_sq *sq)
 static void mlx5_core_test_wc(struct mlx5_core_dev *mdev)
 {
 	unsigned int offset = 0;
+	bool keep_sq = false;
 	struct mlx5_wc_sq *sq;
 	int i, err;
 
@@ -396,11 +401,13 @@ static void mlx5_core_test_wc(struct mlx5_core_dev *mdev)
 	mlx5_wc_destroy_sq(sq);
 
 err_create_sq:
-	mlx5_wc_destroy_cq(&sq->cq);
+	/* An undestroyed CQ may still be in use; keep the SQ that holds it. */
+	keep_sq = mlx5_wc_destroy_cq(&sq->cq);
 err_create_cq:
 	mlx5_free_bfreg(mdev, &sq->bfreg);
 err_alloc_bfreg:
-	kfree(sq);
+	if (!keep_sq)
+		kfree(sq);
 
 	if (mdev->wc_state == MLX5_WC_STATE_UNSUPPORTED)
 		mlx5_core_warn(mdev, "Write combining is not supported\n");
-- 
2.43.0

[PATCH net-next v1 3/4] net/mlx5: DR, Don't free the send ring CQ if destroying it fails

[Yuho Choi]

dr_destroy_cq() ignores the result of mlx5_core_destroy_cq() and frees
the CQ buffer, doorbell record and struct mlx5dr_cq anyway. If
DESTROY_CQ fails, the CQ may still exist in firmware and retain access
to its buffer and doorbell record. mlx5_core_destroy_cq() also returns
early in that case, without waiting for EQ handlers that may already
hold a reference to the CQ.

Keep the CQ and its buffer when DESTROY_CQ fails, as the regular mlx5_ib
CQ destroy path does. A device in internal error is not affected, since
the mlx5 core reports success for DESTROY_CQ in that state.

Fixes: 297cccebdc5a ("net/mlx5: DR, Expose an internal API to issue RDMA operations")
Assisted-by: LLM
Signed-off-by: Yuho Choi <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.

 .../net/ethernet/mellanox/mlx5/core/steering/sws/dr_send.c    | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_send.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_send.c
index 79e3c29d1044e..8fc41adb4c756 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_send.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_send.c
@@ -1130,7 +1130,9 @@ static struct mlx5dr_cq *dr_create_cq(struct mlx5_core_dev *mdev,
 
 static void dr_destroy_cq(struct mlx5_core_dev *mdev, struct mlx5dr_cq *cq)
 {
-	mlx5_core_destroy_cq(mdev, &cq->mcq);
+	/* The device may still use an undestroyed CQ; keep it. */
+	if (mlx5_core_destroy_cq(mdev, &cq->mcq))
+		return;
 	mlx5_wq_destroy(&cq->wq_ctrl);
 	kfree(cq);
 }
-- 
2.43.0

[PATCH net-next v1 4/4] net/mlx5: HWS, Don't free the send ring CQ buffer if destroying the CQ fails

[Yuho Choi]

hws_send_ring_close_cq() ignores the result of mlx5_core_destroy_cq()
and frees the CQ buffer and doorbell record with mlx5_wq_destroy()
anyway. If DESTROY_CQ fails, the CQ may still exist in firmware and
retain access to that memory after it has been returned to the
allocator.

Leak the buffer instead when the CQ cannot be destroyed. A device in
internal error is not affected, since the mlx5 core reports success for
DESTROY_CQ in that state.

Fixes: 2ca62599aa0b ("net/mlx5: HWS, added send engine and context handling")
Assisted-by: LLM
Signed-off-by: Yuho Choi <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/net/ethernet/mellanox/mlx5/core/steering/hws/send.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
index df3b93499eebb..21e2d0a26e88a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
@@ -1005,7 +1005,9 @@ static int hws_send_ring_open_cq(struct mlx5_core_dev *mdev,
 
 static void hws_send_ring_close_cq(struct mlx5hws_send_ring_cq *cq)
 {
-	mlx5_core_destroy_cq(cq->mdev, &cq->mcq);
+	/* A CQ that was not destroyed can still access its buffer, leak it */
+	if (mlx5_core_destroy_cq(cq->mdev, &cq->mcq))
+		return;
 	mlx5_wq_destroy(&cq->wq_ctrl);
 }
 
-- 
2.43.0

Re: [PATCH net-next v1 0/4] net/mlx5: Don't free CQs whose destroy failed

[Junxian Huang]

Hi Yuho,

On 2026/10/16 12:57, Yuho Choi wrote:
> mlx5_core_destroy_cq() can fail, and when it does the CQ may still
> exist in firmware with access to its buffer and doorbell record. It
> also returns early in that case, without waiting for EQ handlers that
> may already hold a reference to the CQ. The four helpers in this series
> ignore that error and free the CQ buffer anyway, while their teardown
> paths also free the object that embeds the struct mlx5_core_cq.
> 
> Keep the buffer and the object that embeds the CQ when DESTROY_CQ

Are the buffer and object freed somewhere else later? Or are they
leaked forever?

Thanks,
Junxian

> fails, as the regular mlx5_ib CQ destroy path does. The HWS send ring
> CQs are polled and never armed, so the HWS patch keeps only their
> buffer rather than passing the error up through the queue open and
> close paths.
> 
> The issues were found with a static analysis tool from ongoing, not yet
> published research, and confirmed by code inspection. None of them was
> reproduced, so this targets net-next; I can resend for net if you
> prefer.
> 
> Compile-tested only (x86_64 defconfig with MLX5_CORE_EN, MLX5_ESWITCH,
> MLX5_SW_STEERING and MLX5_HW_STEERING, W=1).
> 
> Yuho Choi (4):
>   net/mlx5: Don't free the ASO CQ if destroying it fails
>   net/mlx5: Don't free the WC test CQ if destroying it fails
>   net/mlx5: DR, Don't free the send ring CQ if destroying it fails
>   net/mlx5: HWS, Don't free the send ring CQ buffer if destroying the CQ
>     fails
> 
>  .../net/ethernet/mellanox/mlx5/core/lib/aso.c  | 18 +++++++++++++-----
>  .../mellanox/mlx5/core/steering/hws/send.c     |  4 +++-
>  .../mellanox/mlx5/core/steering/sws/dr_send.c  |  4 +++-
>  drivers/net/ethernet/mellanox/mlx5/core/wc.c   | 17 ++++++++++++-----
>  4 files changed, 31 insertions(+), 12 deletions(-)
> 
> 
> base-commit: 2f2f7eeef8b0a1a9b9ca998231419873ccae8d4e

Re: [PATCH net-next v1 0/4] net/mlx5: Don't free CQs whose destroy failed

[Yuho Choi]

On Fri, 16 Oct 2026 at 02:59, Junxian Huang <huangjunxian6@hisilicon.com> wrote:
>
> Are the buffer and object freed somewhere else later? Or are they
> leaked forever?

They are leaked; nothing frees them later. This only happens when the
firmware is alive but rejects DESTROY_CQ. A device in internal error
still frees everything as before.

Freeing them later would mean tracking them until the device is torn
down, which seemed like too much for this failure case. I can look into
reclaiming them if you think it is worth it.

Thanks,
Yuho

Re: [PATCH net-next v1 0/4] net/mlx5: Don't free CQs whose destroy failed

[Leon Romanovsky]

On Fri, Oct 16, 2026 at 12:57:45AM -0400, Yuho Choi wrote:
> mlx5_core_destroy_cq() can fail

No, it should not fail. If it does, the firmware is completely broken
or some other catastrophic bug.

> nd when it does the CQ may still exist in firmware with access to its buffer and doorbell record. It
> also returns early in that case, without waiting for EQ handlers that
> may already hold a reference to the CQ. The four helpers in this series
> ignore that error and free the CQ buffer anyway, while their teardown
> paths also free the object that embeds the struct mlx5_core_cq.
> 
> Keep the buffer and the object that embeds the CQ when DESTROY_CQ
> fails, as the regular mlx5_ib CQ destroy path does. The HWS send ring
> CQs are polled and never armed, so the HWS patch keeps only their
> buffer rather than passing the error up through the queue open and
> close paths.
> 
> The issues were found with a static analysis tool from ongoing, not yet
> published research, and confirmed by code inspection. None of them was
> reproduced, so this targets net-next; I can resend for net if you
> prefer.

Please drop this series. The flow is impossible in current HW implementation.

Thanks

Re: [PATCH net-next v1 0/4] net/mlx5: Don't free CQs whose destroy failed

[Yuho Choi]

On Sun, 18 Oct 2026 at 08:55, Leon Romanovsky <leon@kernel.org> wrote:
> Please drop this series. The flow is impossible in current HW implementation.

Thanks for the explanation, Leon. I'll drop it.

Thanks,
Yuho

Re: [PATCH net-next v1 1/4] net/mlx5: Don't free the ASO CQ if destroying it fails

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete within the same object.
- [Low] When DESTROY_CQ fails, the patch keeps the struct mlx5_aso and
  its CQ buffer and doorbell, but nothing ever frees them.

--

net/mlx5: Don't free the ASO CQ if destroying it fails

With this patch, mlx5_aso_destroy_cq() returns the status of
mlx5_core_destroy_cq() and no longer frees the CQ buffer and doorbell
record when DESTROY_CQ fails. When that happens, mlx5_aso_create() and
mlx5_aso_destroy() also skip freeing the struct mlx5_aso that embeds
the CQ.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c
> index 614cd57a40d74..2d5f8cae6be19 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c

[ ... ]

> @@ -326,7 +330,9 @@ struct mlx5_aso *mlx5_aso_create(struct mlx5_core_dev *mdev, u32 pdn)
>  	return aso;
>  
>  err_sq:
> -	mlx5_aso_destroy_cq(&aso->cq);
> +	/* An undestroyed CQ may still be in use; keep the ASO that holds it. */
> +	if (mlx5_aso_destroy_cq(&aso->cq))
> +		return ERR_PTR(err);

[Severity: Low]
This may be intentional, but when this branch is taken the only pointer
to aso is dropped. Nothing frees the CQ buffer, the doorbell or the
struct mlx5_aso later.

mlx5_aso_destroy() has the same behaviour further down. It is still
declared void in lib/aso.h, so mlx5e_ipsec_aso_cleanup(),
mlx5e_macsec_aso_cleanup() and mlx5e_flow_meters_cleanup() cannot tell
that the ASO was kept. Each of them goes on to free its own state.

The commit message says keeping this memory is intended. The CQ context
only references the EQ, UAR, doorbell and PAS, so the callers freeing the
PD, MR or ctx afterwards does not affect the kept CQ itself.

Is a permanent leak the intended result here? Would it help to return
the status, or to warn, so the kept object is visible to callers?

>  err_cq:
>  	kfree(aso);
>  	return ERR_PTR(err);
> @@ -335,7 +341,9 @@ struct mlx5_aso *mlx5_aso_create(struct mlx5_core_dev *mdev, u32 pdn)
>  void mlx5_aso_destroy(struct mlx5_aso *aso)
>  {
>  	mlx5_aso_destroy_sq(aso);
> -	mlx5_aso_destroy_cq(&aso->cq);
> +	/* An undestroyed CQ may still be in use; keep the ASO that holds it. */
> +	if (mlx5_aso_destroy_cq(&aso->cq))
> +		return;
>  	kfree(aso);
>  }

[Severity: High]
Is the SQ in the same struct mlx5_aso still exposed to the problem this
patch fixes for the CQ? mlx5_aso_destroy_sq() runs first and always
frees the SQ WQE buffer and doorbell record:

drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c:mlx5_aso_destroy_sq() {
	mlx5_core_destroy_sq(sq->cq.mdev, sq->sqn);
	mlx5_aso_free_sq(sq);
}

mlx5_core_destroy_sq() in transobj.c returns void and ignores the result
of mlx5_cmd_exec_in():

void mlx5_core_destroy_sq(struct mlx5_core_dev *dev, u32 sqn)
{
	...
	mlx5_cmd_exec_in(dev, destroy_sq, in);
}

As a result, a failed DESTROY_SQ is never noticed. Then
mlx5_aso_free_sq()->mlx5_wq_destroy() frees the frag buffer that was
programmed as PAS in CREATE_SQ. It also returns the dbr slot to the
shared pgdir through mlx5_db_free(), and that slot can then be given to
another queue.

DESTROY_SQ can fail in the same ways DESTROY_CQ can: -ENOMEM from the
command message allocation, timeouts, or FW status errors. Only the
internal-error case is turned into success, and
mlx5_internal_err_ret_value() lists MLX5_CMD_OP_DESTROY_SQ alongside
DESTROY_CQ.

The ASO SQ is created with cqn = aso->cq.mcq.cqn, so an SQ that firmware
failed to destroy stays bound to the CQ. That is probably the most likely
reason for the following DESTROY_CQ to fail. In that case the new branch
keeps a struct mlx5_aso whose SQ DMA memory has already been freed. Can
the device then still DMA to freed WQE pages, or use a doorbell slot that
now belongs to another queue?

The create error path follows the same pattern:

mlx5_aso_create()
  mlx5_aso_create_sq()
    mlx5_aso_create_sq_rdy()
      mlx5_aso_set_sq_rdy() fails
      mlx5_core_destroy_sq()     <- result ignored
    err_free_asosq:
      mlx5_aso_free_sq()         <- SQ ring and doorbell freed
  err_sq:
    mlx5_aso_destroy_cq()        <- new CQ-only check

Because mlx5_aso_destroy() still returns void, mlx5e_ipsec_aso_cleanup()
also goes on to dma_unmap_single() and kfree() the ctx buffer that ASO SQ
WQEs target, even when the SQ may not have been destroyed:

	mlx5_aso_destroy(aso->aso);
	dma_unmap_single(pdev, aso->dma_addr, sizeof(aso->ctx),
			 DMA_BIDIRECTIONAL);
	kfree(aso);

Should mlx5_core_destroy_sq() return its status? The SQ buffer, doorbell
and struct mlx5_aso could then be kept when DESTROY_SQ fails, the same way
this patch handles the CQ.

The later patches in this series do not touch aso.c or transobj.c, so
this gap is still there at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016045849.813696-6-oss.patchbox%40gmail.com

Re: [PATCH net-next v1 2/4] net/mlx5: Don't free the WC test CQ if destroying it fails

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net/mlx5: SQ DMA use-after-free due to dropped DESTROY_SQ error
- [Low] When DESTROY_CQ fails, mlx5_core_test_wc() now deliberately leaks
  the struct mlx5_wc_sq, the CQ frag buffer and the CQ doorbell record.

--

net/mlx5: Don't free the WC test CQ if destroying it fails

This patch makes mlx5_wc_destroy_cq() return the result of
mlx5_core_destroy_cq() and skip freeing the CQ buffer and doorbell record
when DESTROY_CQ fails. mlx5_core_test_wc() then also keeps the struct
mlx5_wc_sq that embeds the CQ instead of freeing it.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/wc.c b/drivers/net/ethernet/mellanox/mlx5/core/wc.c
> index 7d3d4d739e2cd..01e9800ff6fd9 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/wc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/wc.c

[ ... ]

> @@ -396,11 +401,13 @@ static void mlx5_core_test_wc(struct mlx5_core_dev *mdev)
>  	mlx5_wc_destroy_sq(sq);
>  
>  err_create_sq:
> -	mlx5_wc_destroy_cq(&sq->cq);
> +	/* An undestroyed CQ may still be in use; keep the SQ that holds it. */
> +	keep_sq = mlx5_wc_destroy_cq(&sq->cq);
>  err_create_cq:
>  	mlx5_free_bfreg(mdev, &sq->bfreg);
>  err_alloc_bfreg:
> -	kfree(sq);
> +	if (!keep_sq)
> +		kfree(sq);
>  
>  	if (mdev->wc_state == MLX5_WC_STATE_UNSUPPORTED)
>  		mlx5_core_warn(mdev, "Write combining is not supported\n");

[Severity: High]
The commit message argues that a failed DESTROY_CQ can leave the CQ
alive in firmware with access to its buffer and doorbell record. Does
the same argument not apply to the SQ that is torn down just before
this?

mlx5_wc_destroy_sq() frees the SQ work queue unconditionally after
asking firmware to destroy it:

	mlx5_core_destroy_sq(sq->cq.mdev, sq->sqn);
	mlx5_wq_destroy(&sq->wq_ctrl);

mlx5_core_destroy_sq() in transobj.c returns void and discards the
result of mlx5_cmd_exec_in(). A DESTROY_SQ failure, such as a command
timeout, is therefore never visible here.

sq->wq_ctrl.buf and sq->wq_ctrl.db are then freed while the SQ may
still be live in the device. Once that memory is reused, the device
could read whatever lands there as WQEs or doorbell values and issue
DMA based on it.

The err_modify_sq path in create_wc_sq() has the same shape. It calls
mlx5_core_destroy_sq(), and the SQ buffer is freed on the error return
whether or not the destroy succeeded.

This also seems to weaken the new keep_sq handling. The struct
mlx5_wc_sq is kept when DESTROY_CQ fails, but mlx5_wc_destroy_sq()
has already released its SQ buffer.

If DESTROY_SQ failed silently, an SQ still bound to the CQ might even
be what makes DESTROY_CQ fail. In that case the CQ buffer would be
carefully kept while the SQ buffer that firmware still references has
already been freed.

Fixing this properly looks like it needs mlx5_core_destroy_sq() to
return an error, which affects callers outside wc.c. Would you be
willing to cover that in this series, so mlx5_wc_destroy_sq() can keep
sq->wq_ctrl on failure the same way mlx5_wc_destroy_cq() now keeps
cq->wq_ctrl? Or should that be a separate fix?

[Severity: Low]
Could this leak the struct mlx5_wc_sq, the CQ frag buffer and the CQ
doorbell record again on every retry?

When mlx5_wc_destroy_cq() fails, sq, cq->wq_ctrl.buf and cq->wq_ctrl.db
are kept. Nothing records this, and mdev->wc_state is not changed.

wc_state is only set in mlx5_wc_poll_cq() when a REQ CQE arrives. If the
poll times out, or if mlx5_wc_create_sq() fails, it stays
MLX5_WC_STATE_UNINITIALIZED. The create_sq failure path is also where
DESTROY_CQ is most likely to fail.

mlx5_wc_support_get() only skips the test when wc_state has been set:

	if (mdev->wc_state != MLX5_WC_STATE_UNINITIALIZED)
		goto unlock;

So the next call runs mlx5_core_test_wc() again.

Userspace can reach mlx5_wc_support_get() repeatedly through mlx5_ib.
Examples are mlx5_ib_alloc_ucontext(), the WC page mmap path and the UAR
alloc uverbs method.

Suppose firmware keeps rejecting DESTROY_CQ while the create commands
still succeed. Would each of those calls then leak another sq struct,
another DMA-coherent CQ buffer and another doorbell slot, with no upper
bound? Before this patch these were freed, unsafely, on each run.

Would it make sense to latch wc_state when the CQ has to be kept, for
example to MLX5_WC_STATE_UNSUPPORTED, so the test is not retried?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016045849.813696-6-oss.patchbox%40gmail.com