Re: [PATCH net v2 2/2] net: mana: guard TX wq object destroy with INVALID_MANA_HANDLE check

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch prevents destroying uninitialized TX WQ objects that hold the `INVALID_MANA_HANDLE` sentinel, but bypasses unreached elements in the zero-initialized array, causing handle `0` to be erroneously passed to firmware.
- [High] Hardware vport registrations and configurations are not cleanly reverted if subsequent queue allocations fail inside `mana_create_vport()`, permanently breaking the network interface.
--

commit 27abeb920ad813c7f2d581fe8f249fdc9e94666a
Author: Aditya Garg <gargaditya@linux.microsoft.com>

net: mana: guard TX wq object destroy with INVALID_MANA_HANDLE check

This commit adds a check in mana_destroy_txq() to prevent destroying
uninitialized TX WQ objects that hold the INVALID_MANA_HANDLE sentinel. This
avoids passing invalid handles to the firmware, which previously caused
errors and logs. The change mirrors existing RX-side cleanup patterns.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index c9b1df1ed1098..d7de4c4d25bbe 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -2334,7 +2334,8 @@ static void mana_destroy_txq(struct mana_port_context *apc)
>  			netif_napi_del_locked(napi);
>  			apc->tx_qp[i].txq.napi_initialized = false;
>  		}
> -		mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i].tx_object);
> +		if (apc->tx_qp[i].tx_object != INVALID_MANA_HANDLE)
> +			mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i].tx_object);

[Severity: High]
This is a pre-existing issue, but does this check safely handle all
uninitialized tx_object values?

In mana_create_txq(), apc->tx_qp is allocated using kzalloc_objs(), which
zero-initializes the array:

mana_create_txq() {
    apc->tx_qp = kzalloc_objs(struct mana_tx_qp, apc->num_queues);
    if (!apc->tx_qp)
        return -ENOMEM;

    for (i = 0; i < apc->num_queues; i++) {
        apc->tx_qp[i].tx_object = INVALID_MANA_HANDLE;
    ...
}

If mana_create_txq() fails during the loop, the unvisited elements
(where j > i) will still have their zero-initialized values, meaning
tx_qp[j].tx_object == 0.

Since mana_destroy_txq() iterates over all apc->num_queues, and 0 is not
INVALID_MANA_HANDLE, the condition apc->tx_qp[i].tx_object !=
INVALID_MANA_HANDLE will be true for these unvisited queues.

Could this lead to erroneously calling mana_destroy_wq_obj() with handle 0,
potentially destroying an active WQ object used by another queue or port?

>  
>  		mana_deinit_cq(apc, &apc->tx_qp[i].tx_cq);
>  

[Severity: High]
This isn't a bug introduced by this patch, but is there a resource leak in
mana_create_vport() if mana_create_txq() fails?

mana_create_vport() {
    ...
    err = mana_pf_register_hw_vport(apc);
    if (err)
        return err;

    err = mana_cfg_vport(apc, gd->pdid, gd->doorbell);
    if (err)
        return err;

    return mana_create_txq(apc, net);
}

If mana_create_txq() fails, it returns the error directly to
mana_alloc_queues() without jumping to a destroy_vport label. This bypasses
mana_destroy_vport() and skips cleaning up the hardware vport registrations.

Because the interface fails to open, apc->port_is_up remains false. Future
teardown attempts will skip mana_dealloc_queues(), leaving vport_use_count
permanently incremented.

Would this prevent the interface from ever being brought up again, as
mana_cfg_vport() enforces apc->vport_use_count == 0, requiring a driver
reload to recover?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608101345.2267320-1-gargaditya@linux.microsoft.com?part=2