Re: [PATCH 3/3] net: mana: remove double CQ cleanup in mana_create_rxq error path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized CQ ID in error path clobbers global `cq_table[0]`
- [High] Uninitialized `tx_object` handles in `mana_create_txq` error path destroy valid WQ object 0
--

commit fbbff2b6d2993ad6206b12c69d7d995282d02696
Author: Dipayaan Roy <dipayanroy@linux.microsoft.com>

    net: mana: remove double CQ cleanup in mana_create_rxq error path

This commit removes a redundant mana_deinit_cq() call in the error path of
mana_create_rxq(). Previously, mana_destroy_rxq() was called first, which
already cleans up the CQ and frees the rxq structure. The subsequent
mana_deinit_cq() call caused a use-after-free by accessing the embedded
CQ from the already-freed structure.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index f2a6ea162dc3..9afc786b297a 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -2799,9 +2799,6 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
>  
>  	mana_destroy_rxq(apc, rxq, false);

This isn't a bug introduced by this commit, but while reviewing this error
path I noticed a pre-existing issue. Does this sequence clobber the global
cq_table[0]?

When mana_gd_create_mana_wq_cq() allocates a queue via kzalloc_obj(),
queue->id is initialized to 0.

The actual hardware queue ID is only assigned later when
mana_create_wq_obj() successfully completes.

If mana_create_wq_obj() fails in mana_create_rxq(), the error path calls:
mana_destroy_rxq() -> mana_deinit_cq() -> mana_gd_destroy_queue() ->
mana_gd_destroy_cq()

Inside mana_gd_destroy_cq(), it uses queue->id (which is still 0) to clear
the gc->cq_table[id] array entry:

mana_gd_destroy_cq() {
	u32 id = queue->id;
	...
	gc->cq_table[id] = NULL;
}

This unintentionally clobbers the cq_table entry for CQ 0, which is a
valid queue ID (e.g., belonging to the hardware control channel or
another active queue).

Could this lead to dropped completions for CQ 0 and potentially hang the
device? Should queue->id be explicitly initialized to INVALID_QUEUE_ID?

I also noticed a similar pre-existing issue in the mana_create_txq()
error path. Can this destroy valid WQ object 0?

In mana_create_txq(), apc->tx_qp is allocated using kzalloc_objs(), meaning
all tx_object fields initially default to 0.

The initialization loop sets tx_object = INVALID_MANA_HANDLE iteratively.

If queue creation fails midway (e.g., at i = 0), mana_create_txq() jumps to
the error path and calls mana_destroy_txq().

mana_destroy_txq() loops over all apc->num_queues and unconditionally calls
mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i].tx_object).

For the queue indices that were not yet reached by the initialization loop,
tx_object remains 0. Because 0 is a valid hardware handle, this issues
spurious hardware requests to destroy WQ object 0 multiple times.

Could this inadvertently destroy an active queue belonging to another VF/PF
or the hardware control channel? Should all tx_object handles be initialized
to INVALID_MANA_HANDLE before the queue creation loop begins?

> -	if (cq)
> -		mana_deinit_cq(apc, cq);
> -
>  	return NULL;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260430035935.1859220-1-dipayanroy@linux.microsoft.com?part=3