Skip to contentSkip to conversation

net: mana: Fix NULL dereferences during teardown after attach failure

PATCH Applied Dipayaan Roy 4 participants 8 messages May 25 – May 30

Subsystems (6)
lore
Share & tools
lore
#0 lore raw

v3 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 v3:
- Patch 1: Fixed commit message and fixes tag.
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
#1 lore raw

PATCH v3 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.

Fixes ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
---
 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
index 9afc786b297a..0582803907a8 100644
@@ -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
#2 lore raw

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

Thank 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] 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.

Quoted 5 lines

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?

Quoted 4 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0
index 9afc786b297a8..9e7e4bf526bf6 100644

[ ... ]

Quoted 13 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +2 -6
@@ -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++) {

drivers/net/ethernet/microsoft/mana/mana_en.c static void mana_destroy_vport(struct mana_port_context *apc) · line 2876

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +7 -0
+			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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#3 lore raw

PATCH v3 2/2 net: mana: Skip redundant detach on already-detached port

In reply to Dipayaan Roy

When 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.

Fixes 3b194343c250 ("net: mana: Implement ndo_tx_timeout and serialize queue resets per port.")
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/net/ethernet/microsoft/mana/mana_en.c +6 -0
index 0582803907a8..1e1ad2795c3c 100644
@@ -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
maintainer
#4 lore raw

Re: [PATCH net v3 2/2] net: mana: Skip redundant detach on already-detached port

Quoted 25 lines · Dipayaan Roy

When 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.

Fixes 3b194343c250 ("net: mana: Implement ndo_tx_timeout and serialize queue resets per port.")
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0
index 0582803907a8..1e1ad2795c3c 100644
@@ -3350,6 +3350,12 @@ int mana_detach(struct net_device *ndev, bool from_close)
 

drivers/net/ethernet/microsoft/mana/mana_en.c int mana_detach(struct net_device *ndev, bool from_close) · line 3360

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +6 -0
 	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;

sashiko(gemini) notes the above can lead to different race:

---
Can this early return cause state machine corruption by bypassing the updates
to apc->port_st_save?
Consider this sequence:
1. queue_reset_work runs, mana_detach() succeeds (apc->port_st_save = true, apc->port_is_up = false), but mana_attach() fails.
2. The admin brings the interface down (ip link set dev eth0 down), skipping mana_close() since apc->port_is_up is false.
3. The admin changes the MTU, triggering mana_change_mtu() which calls mana_detach() followed by mana_attach().
4. mana_detach() hits this new early return, preserving apc->port_st_save == true.
When mana_attach() runs, it sees apc->port_st_save == true and allocates queues, setting apc->vport_use_count = 1 and apc->port_is_up = true, even though the interface is administratively down.
If the admin then brings the interface up, mana_open() will unconditionally call mana_alloc_queues(). That function calls mana_cfg_vport(), which will return -EBUSY because apc->vport_use_count is already 1.
This leaves mana_open() failing and the interface down. Since the interface is already down, trying to bring it down again is a no-op, meaning mana_close() is never called to clean up the orphaned queues. Does this sequence permanently brick the port until the driver is reloaded?
---

I think you need to be more restrictive in the early return check.

/P

#5 lore raw

Re: [PATCH net v3 2/2] net: mana: Skip redundant detach on already-detached port

Quoted 64 lines · Paolo Abeni
Quoted 25 lines · Dipayaan Roy

When 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.

Fixes 3b194343c250 ("net: mana: Implement ndo_tx_timeout and serialize queue resets per port.")
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0
index 0582803907a8..1e1ad2795c3c 100644
@@ -3350,6 +3350,12 @@ int mana_detach(struct net_device *ndev, bool from_close)
 

drivers/net/ethernet/microsoft/mana/mana_en.c int mana_detach(struct net_device *ndev, bool from_close) · line 3360

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +6 -0
 	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;

sashiko(gemini) notes the above can lead to different race:

---
Can this early return cause state machine corruption by bypassing the updates
to apc->port_st_save?
Consider this sequence:
1. queue_reset_work runs, mana_detach() succeeds (apc->port_st_save = true, apc->port_is_up = false), but mana_attach() fails.
2. The admin brings the interface down (ip link set dev eth0 down), skipping mana_close() since apc->port_is_up is false.
3. The admin changes the MTU, triggering mana_change_mtu() which calls mana_detach() followed by mana_attach().
4. mana_detach() hits this new early return, preserving apc->port_st_save == true.
When mana_attach() runs, it sees apc->port_st_save == true and allocates queues, setting apc->vport_use_count = 1 and apc->port_is_up = true, even though the interface is administratively down.
If the admin then brings the interface up, mana_open() will unconditionally call mana_alloc_queues(). That function calls mana_cfg_vport(), which will return -EBUSY because apc->vport_use_count is already 1.
This leaves mana_open() failing and the interface down. Since the interface is already down, trying to bring it down again is a no-op, meaning mana_close() is never called to clean up the orphaned queues. Does this sequence permanently brick the port until the driver is reloaded?
---

Quoted 4 lines

I think you need to be more restrictive in the early return check.

/P

Hi Paolo,

Thank you for the comments,
I think the scenario pointed out by sashiko does not seems valid, as it mentioned in step 2 and 3 admin changing MTU after bringing interface down. This is becasue netif_set_mtu_ext() in dev.c checks netif_device_present and returns -ENODEV before calling ndo_change_mtu. So mana_change_mtu() is never reachable when the device is in the !present state.

https://elixir.bootlin.com/linux/v7.0/source/net/core/dev.c#L9906

Please let me know if the above check is good enough?

Regards
Dipayaan Roy

#6 lore raw

Re: [PATCH net v3 2/2] net: mana: Skip redundant detach on already-detached port

In reply to Dipayaan Roy
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] State machine corruption in mana_detach leads to permanently bricked port. - [Medium] The commit…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] State machine corruption in mana_detach leads to permanently bricked port.
- [Medium] The commit message falsely claims the patch fixes a NULL pointer dereference, indicating a misdiagnosis of the original bug. --

commit 9693e5b39c52406711e627ad84a6eb7471551a72
Author: Dipayaan Roy <[email protected]>

net: mana: Skip redundant detach on already-detached port

This commit adds an early exit to mana_detach() to prevent redundant teardown when a port is already detached due to a previous failed attach.

Quoted 5 lines

When 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.

[Severity: Medium]
Is this description of the crash accurate? Looking at the unpatched code, if mana_attach() fails, apc->port_is_up is left as false. If mana_detach() is called again, apc->port_st_save is set to apc->port_is_up (which is false).

Because mana_dealloc_queues() is guarded by if (apc->port_st_save), queue teardown would be entirely skipped. Could the actual issue with the old code be that apc->port_st_save was incorrectly cleared, preventing future mana_attach() retries from allocating queues and recovering the port, rather than a NULL pointer dereference?

Quoted 5 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0
index 9e7e4bf526bf6..c9b1df1ed1098 100644

drivers/net/ethernet/microsoft/mana/mana_en.c int mana_detach(struct net_device *ndev, bool from_close) · line 3358

Quoted 9 lines
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;
+

[Severity: High]
Can this early return cause state machine corruption by bypassing the updates to apc->port_st_save?

Consider this sequence:
1. queue_reset_work runs, mana_detach() succeeds (apc->port_st_save = true, apc->port_is_up = false), but mana_attach() fails.
2. The admin brings the interface down (ip link set dev eth0 down), skipping mana_close() since apc->port_is_up is false.
3. The admin changes the MTU, triggering mana_change_mtu() which calls mana_detach() followed by mana_attach().
4. mana_detach() hits this new early return, preserving apc->port_st_save == true.

When mana_attach() runs, it sees apc->port_st_save == true and allocates queues, setting apc->vport_use_count = 1 and apc->port_is_up = true, even though the interface is administratively down.

If the admin then brings the interface up, mana_open() will unconditionally call mana_alloc_queues(). That function calls mana_cfg_vport(), which will return -EBUSY because apc->vport_use_count is already 1.

This leaves mana_open() failing and the interface down. Since the interface is already down, trying to bring it down again is a no-op, meaning mana_close() is never called to clean up the orphaned queues.

Does this sequence permanently brick the port until the driver is reloaded?

Quoted 2 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0
 	apc->port_st_save = apc->port_is_up;
 	apc->port_is_up = false;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
#7 lore raw
In reply to Dipayaan Roy
This series was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 5 lines · On Mon, 25 May 2026 01:08:23 -0700 you wrote:

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.

Quoted 7 lines

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().

[...]

Here is the summary with links:
- [net,v3,1/2] net: mana: Add NULL guards in teardown path to prevent panic on attach failure

    https://git.kernel.org/netdev/net/c/17bfe0a8c014
  - [net,v3,2/2] net: mana: Skip redundant detach on already-detached port
    https://git.kernel.org/netdev/net/c/5b05aa36ee24

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html