net: mana: Fix probe/remove error path bugs

Move INIT_WORK(link_change_work) to right after the mana_context allocation, before any error path that could reach mana_remove().

Previously, if mana_create_eq() or mana_query_device_cfg() failed, mana_probe() would jump to the error path which calls mana_remove(). mana_remove() unconditionally calls disable_work_sync(link_change_work), but the work struct had not been initialized yet. This can trigger CONFIG_DEBUG_OBJECTS_WORK enabled.

Fixes 54133f9b4b53 ("net: mana: Support HW link state events")

Signed-off-by Erni Sri Satya Vennela <[email protected]>

---
Changes in v2:
* Apply the patch in net instead of net-next.

Move INIT_DELAYED_WORK(gf_stats_work) to before mana_create_eq(), while keeping schedule_delayed_work() at its original location.

Previously, if any function between mana_create_eq() and the INIT_DELAYED_WORK call failed, mana_probe() would call mana_remove() which unconditionally calls cancel_delayed_work_sync(gf_stats_work) in __flush_work() or debug object warnings with CONFIG_DEBUG_OBJECTS_WORK enabled.

Fixes be4f1d67ec56 ("net: mana: Add standard counter rx_missed_errors")

Signed-off-by Erni Sri Satya Vennela <[email protected]>

drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0

index 9d095a6fb56c7..4d42cbfb3f15b 100644
@@ -3634,10 +3634,9 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
 	if (!resuming) {

From: 'Simon Horman' <[email protected]>

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev
---
net: mana: Don't overwrite port probe error with add_adev result

This commit ensures that if mana_probe_port() or mana_attach() fails, the error is not overwritten by the result of add_adev(). By preserving the error, it triggers mana_remove() cleanup instead of leaving ports partially initialized.

Quoted 6 lines

drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0

index 9d095a6fb56c7..4d42cbfb3f15b 100644
@@ -3634,10 +3634,9 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
 	if (!resuming) {

drivers/net/ethernet/microsoft/mana/mana_en.c int mana_probe(struct gdma_dev *gd, bool resuming) · line 3640

Quoted 10 lines

drivers/net/ethernet/microsoft/mana/mana_en.c +3 -4

 		for (i = 0; i < ac->num_ports; i++) {
 			err = mana_probe_port(ac, i, &ac->ports[i]);
-			/* we log the port for which the probe failed and stop
-			 * probes for subsequent ports.
-			 * Note that we keep running ports, for which the probes
-			 * were successful, unless add_adev fails too
+			/* Log the port for which the probe failed, stop probing
+			 * subsequent ports, and skip add_adev.
+			 * Already-probed ports remain functional.
 			 */

Are these comments accurate regarding ports remaining functional?

Since this patch preserves the error code, execution falls through to the end of mana_probe() where it checks if err is set and calls mana_remove(). mana_remove() will iterate over the successfully allocated ports, completely detach them, unregister their net_devices, and destroy event queues.

It appears no ports remain functional because the device is completely torn down.

[ ... ]

drivers/net/ethernet/microsoft/mana/mana_en.c int mana_probe(struct gdma_dev *gd, bool resuming)

Quoted 15 lines

drivers/net/ethernet/microsoft/mana/mana_en.c +3 -4

@@ -3651,10 +3650,9 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
 			enable_work(&apc->queue_reset_work);
 			err = mana_attach(ac->ports[i]);
 			rtnl_unlock();
-			/* we log the port for which the attach failed and stop
-			 * attach for subsequent ports
-			 * Note that we keep running ports, for which the attach
-			 * were successful, unless add_adev fails too
+			/* Log the port for which the attach failed, stop
+			 * attaching subsequent ports, and skip add_adev.
+			 * Already-attached ports remain functional.
 			 */
 			if (err) {
 				dev_err(dev, "Attach Failed for port %d\n", i);

@@ -3663,7 +3661,8 @@ int mana_probe(struct gdma_dev *gd, bool resuming)

Does propagating this error during PM resume introduce a NULL pointer dereference?

If PM resume fails here (resuming == true), the preserved error code causes mana_probe() to call mana_remove(), which tears down the device and sets gd->gdma_context = NULL.

However, a failed resume callback does not automatically unbind the driver. When the device is eventually unbound later, mana_remove() is invoked again:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_remove() {
    struct gdma_context *gc = gd->gdma_context;
    struct device *dev = gc->dev;
    ...
}

In the second invocation, gd->gdma_context is NULL, meaning gc is NULL, which could lead to a kernel panic when dereferencing gc->dev.

Quoted 37 lines · Simon Horman

From: 'Simon Horman' <[email protected]>

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev
---
net: mana: Don't overwrite port probe error with add_adev result

This commit ensures that if mana_probe_port() or mana_attach() fails, the error is not overwritten by the result of add_adev(). By preserving the error, it triggers mana_remove() cleanup instead of leaving ports partially initialized.

Quoted 6 lines

drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0

index 9d095a6fb56c7..4d42cbfb3f15b 100644
@@ -3634,10 +3634,9 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
 	if (!resuming) {

drivers/net/ethernet/microsoft/mana/mana_en.c int mana_probe(struct gdma_dev *gd, bool resuming) · line 3640

Quoted 10 lines

drivers/net/ethernet/microsoft/mana/mana_en.c +3 -4

 		for (i = 0; i < ac->num_ports; i++) {
 			err = mana_probe_port(ac, i, &ac->ports[i]);
-			/* we log the port for which the probe failed and stop
-			 * probes for subsequent ports.
-			 * Note that we keep running ports, for which the probes
-			 * were successful, unless add_adev fails too
+			/* Log the port for which the probe failed, stop probing
+			 * subsequent ports, and skip add_adev.
+			 * Already-probed ports remain functional.
 			 */

Are these comments accurate regarding ports remaining functional?

Since this patch preserves the error code, execution falls through to the end of mana_probe() where it checks if err is set and calls mana_remove(). mana_remove() will iterate over the successfully allocated ports, completely detach them, unregister their net_devices, and destroy event queues.

Quoted 3 lines

It appears no ports remain functional because the device is completely torn down.

I will update the patch with the updated comment message.

Quoted 34 lines

[ ... ]

drivers/net/ethernet/microsoft/mana/mana_en.c int mana_probe(struct gdma_dev *gd, bool resuming)

Quoted 15 lines

drivers/net/ethernet/microsoft/mana/mana_en.c +3 -4

@@ -3651,10 +3650,9 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
 			enable_work(&apc->queue_reset_work);
 			err = mana_attach(ac->ports[i]);
 			rtnl_unlock();
-			/* we log the port for which the attach failed and stop
-			 * attach for subsequent ports
-			 * Note that we keep running ports, for which the attach
-			 * were successful, unless add_adev fails too
+			/* Log the port for which the attach failed, stop
+			 * attaching subsequent ports, and skip add_adev.
+			 * Already-attached ports remain functional.
 			 */
 			if (err) {
 				dev_err(dev, "Attach Failed for port %d\n", i);

@@ -3663,7 +3661,8 @@ int mana_probe(struct gdma_dev *gd, bool resuming)

Does propagating this error during PM resume introduce a NULL pointer dereference?

If PM resume fails here (resuming == true), the preserved error code causes mana_probe() to call mana_remove(), which tears down the device and sets gd->gdma_context = NULL.

However, a failed resume callback does not automatically unbind the driver. When the device is eventually unbound later, mana_remove() is invoked again:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_remove() {
    struct gdma_context *gc = gd->gdma_context;
    struct device *dev = gc->dev;
    ...
}

Quoted 2 lines

In the second invocation, gd->gdma_context is NULL, meaning gc is NULL, which could lead to a kernel panic when dereferencing gc->dev.

Thankyou for pointing it out, Simon.
Since this is a pre-existing bug, I will create a different patch for this change and make it as part of this patchset.

Fixes ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: mana: Fix EQ leak in mana_remove on NULL port

In mana_remove(), when a NULL port is encountered in the port iteration loop, 'goto out' skips the mana_destroy_eq(ac) call, leaking the event queues allocated earlier by mana_create_eq(). This can happen when mana_probe_port() fails for port 0, leaving ac->ports[0] as NULL. On driver unload or error cleanup, mana_remove() hits the NULL entry and jumps past mana_destroy_eq(). Change 'goto out' to 'break' so the for-loop exits normally and mana_destroy_eq() is always reached.

Quoted 1 line

Fixes ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")

Does this Fixes tag point to the correct commit? Looking at the commit history, the original driver commit ca9c54d2d6a5 did not have mana_destroy_eq() in mana_remove() at all, so the 'goto out' pattern could not have caused an EQ leak at that point.

The bug appears to have been introduced by commit 1e2d0824a9c3 ("net: mana:
Add support for EQ sharing"), which added mana_destroy_eq(ac) to mana_remove() and placed it after the 'out:' label, making it skippable by the 'goto out' on NULL port.

Should the Fixes tag be:

Fixes 1e2d0824a9c3 ("net: mana: Add support for EQ sharing")