Re: [PATCH net v3 1/2] net: mana: Add NULL guards in teardown path to prevent panic on attach failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] mana_add_rx_queues() writes to a freed apc->rxqs pointer on success path after an error.
- [Medium] mana_destroy_txq() attempts to destroy uninitialized WQ objects.
--

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

net: mana: Add NULL guards in teardown path to prevent panic on attach failure

This patch adds NULL guards for apc->rxqs and apc->tx_qp in multiple teardown
paths, such as mana_fence_rqs(), mana_destroy_vport(), and
mana_dealloc_queues(), to prevent NULL pointer dereference panics when queue
allocation fails.

> When queue allocation fails partway through, the error cleanup frees
> and NULLs apc->tx_qp and apc->rxqs. Multiple teardown paths such as
> mana_remove(), mana_change_mtu() recovery, and internal error handling
> in mana_alloc_queues() can subsequently call into functions that
> dereference these pointers without NULL checks:

[Severity: High]
This is a pre-existing issue, but could a subsequent interface bring-up
cause a panic due to the NULL apc->rxqs pointer?

If mana_attach() fails (e.g., during mana_change_mtu() recovery), the error
path calls mana_cleanup_port_context(apc), which frees apc->rxqs and sets it
to NULL, leaving the interface in a down state.

If the interface is subsequently brought back up, mana_open() is invoked.
mana_open() calls mana_alloc_queues() directly, skipping
mana_init_port_context() where apc->rxqs is normally allocated.

When mana_alloc_queues() reaches mana_add_rx_queues(), it executes:

mana_add_rx_queues()
    apc->rxqs[i] = rxq;

Would this write to the freed apc->rxqs pointer and cause a kernel panic?

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 9afc786b297a8..9e7e4bf526bf6 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -2858,13 +2861,16 @@ static void mana_destroy_vport(struct mana_port_context *apc)
>  	struct mana_rxq *rxq;
>  	u32 rxq_idx;
>  
> -	for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
> -		rxq = apc->rxqs[rxq_idx];
> -		if (!rxq)
> -			continue;
> +	if (apc->rxqs) {
>  
> -		mana_destroy_rxq(apc, rxq, true);
> -		apc->rxqs[rxq_idx] = NULL;
> +		for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
> +			rxq = apc->rxqs[rxq_idx];
> +			if (!rxq)
> +				continue;
> +
> +			mana_destroy_rxq(apc, rxq, true);
> +			apc->rxqs[rxq_idx] = NULL;
> +		}
>  	}
>  
>  	mana_destroy_txq(apc);

[Severity: Medium]
This is a pre-existing issue, but does mana_destroy_txq() send invalid
teardown requests to the hardware if mana_create_txq() failed partway
through?

Unlike mana_destroy_rxq(), which explicitly checks if rxq->rxobj is valid
(rxq->rxobj != INVALID_MANA_HANDLE) before destroying it, mana_destroy_txq()
unconditionally loops over all num_queues and calls:

mana_destroy_txq()
    mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i].tx_object);

For queues that were not initialized, the tx_object handle is 0 because the
memory was zeroed by allocation, rather than being set to
INVALID_MANA_HANDLE.

Does this result in blindly sending a MANA_DESTROY_WQ_OBJ hardware request
with wq_obj_handle = 0 for the uninitialized queues?

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