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 <[email protected]> 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/[email protected]?part=1
