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

[netdev-bot+sashiko@kernel.org]

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