net: mana: Fix NULL dereferences during teardown after attach failure
Subsystems (6)
v2 cover net: mana: Fix NULL dereferences during teardown after attach failure
When mana_attach() fails (e.g. during queue allocation), the error cleanup frees apc->tx_qp and apc->rxqs and sets them to NULL. Multiple subsequent teardown paths can then dereference these NULL pointers, causing kernel panics.
Patch 1 adds NULL guards in the low-level teardown functions (mana_fence_rqs, mana_destroy_vport, mana_dealloc_queues) so they are safe to call regardless of queue initialization state. This covers all
callers: mana_remove(), mana_change_mtu() recovery, and internal error paths in mana_alloc_queues().
Patch 2 adds an early exit in mana_detach() for already-detached ports, making it safe for non-close callers. This allows the queue reset handler to safely retry mana_attach() without redundant teardown.
Changes in v2:
- Patch 2: moved netif_device_present() check into mana_detach() as an early exit instead of using goto in the work handler
Dipayaan Roy (2):
net: mana: Add NULL guards in teardown path to prevent panic on attach
failure
net: mana: Skip redundant detach on already-detached port
drivers/net/ethernet/microsoft/mana/mana_en.c | 76 ++++++++++++------- 1 file changed, 47 insertions(+), 29 deletions(-)
Signature · 1 line
2.43.0
PATCH v2 1/2 net: mana: Add NULL guards in teardown path to prevent panic on attach failure
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:
- mana_chn_setxdp() dereferences apc->rxqs[0], causing a NULL pointer dereference panic (CR2: 0000000000000000 at mana_chn_setxdp+0x26).
- mana_destroy_vport() iterates apc->rxqs without a NULL check.
- mana_fence_rqs() iterates apc->rxqs without a NULL check.
- mana_dealloc_queues() iterates apc->tx_qp without a NULL check.
Add NULL guards for apc->rxqs in mana_fence_rqs(), mana_destroy_vport(), and before the mana_chn_setxdp() call. Add a NULL guard for apc->tx_qp in mana_dealloc_queues() to skip TX queue draining when TX queues were never allocated or already freed.
--- drivers/net/ethernet/microsoft/mana/mana_en.c | 70 +++++++++++-------- 1 file changed, 41 insertions(+), 29 deletions(-)
drivers/net/ethernet/microsoft/mana/mana_en.c +41 -29
@@ -1727,6 +1727,9 @@ static void mana_fence_rqs(struct mana_port_context *apc)struct mana_rxq *rxq;int err; +if (!apc->rxqs) +return ; +for (rxq_idx =0 ; rxq_idx < apc->num_queues; rxq_idx++) { rxq = apc->rxqs[rxq_idx]; err =mana_fence_rq (apc, rxq);
@@ -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);
@@ -3269,7 +3275,8 @@ static int mana_dealloc_queues(struct net_device *ndev)if (apc->port_is_up)return -EINVAL ; -mana_chn_setxdp (apc,NULL ); +if (apc->rxqs) +mana_chn_setxdp (apc,NULL );if (gd->gdma_context->is_pf && !apc->ac->bm_hostmode)mana_pf_deregister_filter (apc);
@@ -3287,33 +3294,38 @@ static int mana_dealloc_queues(struct net_device *ndev)* number of queues. */ -for (i =0 ; i < apc->num_queues; i++) { - txq = &apc->tx_qp[i].txq; - tsleep =1000 ; -while (atomic_read (&txq->pending_sends) >0 && -time_before (jiffies, timeout)) { -usleep_range (tsleep, tsleep +1000 ); - tsleep <<=1 ; - } -if (atomic_read (&txq->pending_sends)) { - err =pcie_flr (to_pci_dev (gd->gdma_context->dev)); -if (err) { -netdev_err (ndev,"flr failed %d with %d pkts pending in txq %u\n" , - err,atomic_read (&txq->pending_sends), - txq->gdma_txq_id); +if (apc->tx_qp) { +for (i =0 ; i < apc->num_queues; i++) { + txq = &apc->tx_qp[i].txq; + tsleep =1000 ; +while (atomic_read (&txq->pending_sends) >0 && +time_before (jiffies, timeout)) { +usleep_range (tsleep, tsleep +1000 ); + tsleep <<=1 ; + } +if (atomic_read (&txq->pending_sends)) { + err = +pcie_flr (to_pci_dev (gd->gdma_context->dev)); +if (err) { +netdev_err (ndev,"flr failed %d with %d pkts pending in txq %u\n" , + err, +atomic_read (&txq->pending_sends), + txq->gdma_txq_id); + } +break ; } -break ; } - } -for (i =0 ; i < apc->num_queues; i++) { - txq = &apc->tx_qp[i].txq; -while ((skb =skb_dequeue (&txq->pending_skbs))) { -mana_unmap_skb (skb, apc); -dev_kfree_skb_any (skb); +for (i =0 ; i < apc->num_queues; i++) { + txq = &apc->tx_qp[i].txq; +while ((skb =skb_dequeue (&txq->pending_skbs))) { +mana_unmap_skb (skb, apc); +dev_kfree_skb_any (skb); + } +atomic_set (&txq->pending_sends,0 ); } -atomic_set (&txq->pending_sends,0 ); } +/* We're 100% sure the queues can no longer be woken up, because * we're sure now mana_poll_tx_cq() can't be running. */
Signature · 1 line
2.43.0
Re: [PATCH net v2 1/2] net: mana: Add NULL guards in teardown path to prevent panic on attach failure
Quoted 12 lines · Dipayaan Roy
When queue allocation fails partway through, the error cleanup frees and NULLs
apc->tx_qpandapc->rxqs. Multiple teardown paths such asmana_remove(),mana_change_mtu()recovery, and internal error handling inmana_alloc_queues()can subsequently call into functions that dereference these pointers without NULL checks:-
mana_chn_setxdp()dereferences apc->rxqs[0], causing a NULL pointer dereference panic (CR2: 0000000000000000 atmana_chn_setxdp+0x26).
-mana_destroy_vport()iteratesapc->rxqswithout a NULL check.
-mana_fence_rqs()iteratesapc->rxqswithout a NULL check.
-mana_dealloc_queues()iteratesapc->tx_qpwithout a NULL check.
Quoted 8 lines
Add NULL guards for
apc->rxqsinmana_fence_rqs(),mana_destroy_vport(), and before themana_chn_setxdp()call. Add a NULL guard forapc->tx_qpinmana_dealloc_queues()to skip TX queue draining when TX queues were never allocated or already freed.Fixes ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")Signed-off-by Dipayaan Roy <[email protected]>
Hi,
I will send a v3 to fix the fixes tag issue highlighted in:
https://netdev-ctrl.bots.linux.dev/logs/build/1099669/14590738/verify_fixes/summary
Quoted 115 lines
--- drivers/net/ethernet/microsoft/mana/mana_en.c | 70 +++++++++++-------- 1 file changed, 41 insertions(+), 29 deletions(-)drivers/net/ethernet/microsoft/mana/mana_en.c +41
-29@@ -1727,6 +1727,9 @@ static void mana_fence_rqs(struct mana_port_context *apc)struct mana_rxq *rxq;int err; +if (!apc->rxqs) +return ; +for (rxq_idx =0 ; rxq_idx < apc->num_queues; rxq_idx++) { rxq = apc->rxqs[rxq_idx]; err =mana_fence_rq (apc, rxq);@@ -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);@@ -3269,7 +3275,8 @@ static int mana_dealloc_queues(struct net_device *ndev)if (apc->port_is_up)return -EINVAL ; -mana_chn_setxdp (apc,NULL ); +if (apc->rxqs) +mana_chn_setxdp (apc,NULL );if (gd->gdma_context->is_pf && !apc->ac->bm_hostmode)mana_pf_deregister_filter (apc);@@ -3287,33 +3294,38 @@ static int mana_dealloc_queues(struct net_device *ndev)* number of queues. */ -for (i =0 ; i < apc->num_queues; i++) { - txq = &apc->tx_qp[i].txq; - tsleep =1000 ; -while (atomic_read (&txq->pending_sends) >0 && -time_before (jiffies, timeout)) { -usleep_range (tsleep, tsleep +1000 ); - tsleep <<=1 ; - } -if (atomic_read (&txq->pending_sends)) { - err =pcie_flr (to_pci_dev (gd->gdma_context->dev)); -if (err) { -netdev_err (ndev,"flr failed %d with %d pkts pending in txq %u\n" , - err,atomic_read (&txq->pending_sends), - txq->gdma_txq_id); +if (apc->tx_qp) { +for (i =0 ; i < apc->num_queues; i++) { + txq = &apc->tx_qp[i].txq; + tsleep =1000 ; +while (atomic_read (&txq->pending_sends) >0 && +time_before (jiffies, timeout)) { +usleep_range (tsleep, tsleep +1000 ); + tsleep <<=1 ; + } +if (atomic_read (&txq->pending_sends)) { + err = +pcie_flr (to_pci_dev (gd->gdma_context->dev)); +if (err) { +netdev_err (ndev,"flr failed %d with %d pkts pending in txq %u\n" , + err, +atomic_read (&txq->pending_sends), + txq->gdma_txq_id); + } +break ; } -break ; } - } -for (i =0 ; i < apc->num_queues; i++) { - txq = &apc->tx_qp[i].txq; -while ((skb =skb_dequeue (&txq->pending_skbs))) { -mana_unmap_skb (skb, apc); -dev_kfree_skb_any (skb); +for (i =0 ; i < apc->num_queues; i++) { + txq = &apc->tx_qp[i].txq; +while ((skb =skb_dequeue (&txq->pending_skbs))) { +mana_unmap_skb (skb, apc); +dev_kfree_skb_any (skb); + } +atomic_set (&txq->pending_sends,0 ); } -atomic_set (&txq->pending_sends,0 ); } +/* We're 100% sure the queues can no longer be woken up, because * we're sure now mana_poll_tx_cq() can't be running. */ Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH net v2 1/2] net: mana: Add NULL guards in teardown path to prevent panic on attach failure
In reply to Dipayaan RoyThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] State Machine Violation / NULL Pointer Dereference on interface open after a failed MTU change.
- [High] Uninitialized Resource Teardown / Invalid Hardware Request during TX queue creation failure. --
commit effecc83d343cb89cadf93984f0a62ac828d0166
Author: Dipayaan Roy <[email protected]>
net: mana: Add NULL guards in teardown path to prevent panic on attach failure
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.
drivers/net/ethernet/microsoft/mana/mana_en.c static void mana_fence_rqs(struct mana_port_context *apc) · line 1732
Quoted 11 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +3
-0@@ -1727,6 +1727,9 @@ static void mana_fence_rqs(struct mana_port_context *apc)struct mana_rxq *rxq;int err; +if (!apc->rxqs) +return ; +
[Severity: High]
This isn't a bug introduced by this patch, but if mana_change_mtu() fails, does it leave the device in a state where a subsequent mana_open() will crash?
When mana_change_mtu() calls mana_detach(ndev, false), the device is detached and mana_cleanup_port_context() is explicitly called, freeing apc->rxqs and setting it to NULL.
It then calls mana_attach(). If mana_attach() fails internally (e.g., inside its call to mana_alloc_queues()), its error path calls mana_cleanup_port_context() again, which frees apc->rxqs and sets it to NULL.
mana_change_mtu() then returns the error.
The interface is now in the DOWN state (apc->port_is_up == false) with apc->rxqs == NULL.
If the user subsequently attempts to bring the interface up (ip link set up), mana_open() is executed. mana_open() calls mana_alloc_queues() but does not call mana_init_port() to reallocate apc->rxqs.
Inside mana_alloc_queues(), mana_add_rx_queues() unconditionally dereferences apc->rxqs:
apc->rxqs[i] = rxq;
Could this result in a kernel panic?
[ ... ]
Quoted 17 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +0
-13@@ -3287,33 +3294,38 @@ static int mana_dealloc_queues(struct net_device *ndev)* number of queues. */ -for (i =0 ; i < apc->num_queues; i++) { - txq = &apc->tx_qp[i].txq; - tsleep =1000 ; -while (atomic_read (&txq->pending_sends) >0 && -time_before (jiffies, timeout)) { -usleep_range (tsleep, tsleep +1000 ); - tsleep <<=1 ; - } -if (atomic_read (&txq->pending_sends)) { - err =pcie_flr (to_pci_dev (gd->gdma_context->dev)); -if (err) { - netdev_err(ndev, "flr failed %d with %d pkts pending in txq %u\n", - err, atomic_read(&txq->pending_sends),
drivers/net/ethernet/microsoft/mana/mana_en.c static int mana_dealloc_queues(struct net_device *ndev) · line 3305
Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +9
-1- txq->gdma_txq_id); +if (apc->tx_qp) { +for (i =0 ; i < apc->num_queues; i++) { + txq = &apc->tx_qp[i].txq; + tsleep =1000 ; +while (atomic_read (&txq->pending_sends) >0 && +time_before (jiffies, timeout)) { +usleep_range (tsleep, tsleep +1000 ); + tsleep <<=1 ; + }
[Severity: High]
This is a pre-existing issue, but does mana_destroy_txq() send invalid handles to the hardware when TX queue creation fails?
In mana_create_txq(), the apc->tx_qp array is allocated via kzalloc_objs(), meaning all elements (including tx_object) are initialized to 0.
The function then loops over num_queues. For the current queue i, it sets apc->tx_qp[i].tx_object = INVALID_MANA_HANDLE; before attempting to create the hardware object via mana_create_wq_obj().
If creation fails, it jumps to the out label and calls mana_destroy_txq(apc). mana_destroy_txq() unconditionally loops over all num_queues and calls:
mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i].tx_object);
Unlike mana_destroy_rxq(), which checks if (rxq->rxobj != INVALID_MANA_HANDLE), mana_destroy_txq() passes the handle directly to mana_destroy_wq_obj().
For the failed queue, the handle is INVALID_MANA_HANDLE. For all unreached queues (j > i), the handle is 0.
Could this cause mana_destroy_wq_obj() to send invalid handles (including 0, which might be a valid handle for an unrelated object) to the hardware?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v2 2/2 net: mana: Skip redundant detach on already-detached port
In reply to Dipayaan RoyWhen mana_per_port_queue_reset_work_handler() runs after a previous detach succeeded but attach failed, the port is left in a detached state with apc->tx_qp and apc->rxqs already freed. Calling mana_detach() again unconditionally leads to NULL pointer dereferences during queue teardown.
Add an early exit in mana_detach() when the port is already in detached state (!netif_device_present) for non-close callers, making it safe to call idempotently. This allows the queue reset handler and other recovery paths to simply retry mana_attach() without redundant teardown.
--- drivers/net/ethernet/microsoft/mana/mana_en.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/net/ethernet/microsoft/mana/mana_en.c +6 -0
@@ -3350,6 +3350,12 @@ int mana_detach(struct net_device *ndev, bool from_close)ASSERT_RTNL (); +/* If already detached (indicates detach succeeded but attach failed +* previously). Now skip mana detach and just retry mana_attach. +*/ +if (!from_close && !netif_device_present (ndev)) +return 0 ; + apc->port_st_save = apc->port_is_up; apc->port_is_up = false;
Signature · 1 line
2.43.0