Re: [PATCH net-next v1 2/4] net/mlx5: Don't free the WC test 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] 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