Skip to contentSkip to conversation

ixgbe: fix memory leaks in ixgbe_recovery_probe()

PATCH Superseded Kohei Enju 4 participants 5 messages Dec 6, 2025 – Dec 11, 2025

Subsystems (3)
Networking Network drivers Intel Ethernet

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Kohei Enju

author
#0 lore raw

PATCH 1/1 ixgbe: fix memory leaks in ixgbe_recovery_probe()

ixgbe_recovery_probe() does not free the following resources in its error path, unlike ixgbe_probe():
- adapter->io_addr
- adapter->jump_tables[0]
- adapter->mac_table
- adapter->rss_key
- adapter->af_xdp_zc_qps

The leaked MMIO region can be observed in /proc/vmallocinfo, and the remaining leaks are reported by kmemleak.

Free these allocations and unmap the MMIO region on failure to avoid the leaks.

Fixes 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery mode")
Signed-off-by Kohei Enju <[email protected]>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +5 -0
index 4af3b3e71ff1..1bfec3fffae0 100644
@@ -11508,6 +11508,11 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)
 	mutex_destroy(&adapter->hw.aci.lock);
 	ixgbe_release_hw_control(adapter);
 clean_up_probe:
+	iounmap(adapter->io_addr);
+	kfree(adapter->jump_tables[0]);
+	kfree(adapter->mac_table);
+	kfree(adapter->rss_key);
+	bitmap_free(adapter->af_xdp_zc_qps);
 	disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter->state);
 	free_netdev(netdev);
 	devlink_free(adapter->devlink);
Signature · 1 line
2.52.0
reviewer
#1 lore raw
Quoted 8 lines · Kohei Enju

ixgbe_recovery_probe() does not free the following resources in its error path, unlike ixgbe_probe():
- adapter->io_addr
- adapter->jump_tables[0]
- adapter->mac_table
- adapter->rss_key
- adapter->af_xdp_zc_qps

Quoted 8 lines

The leaked MMIO region can be observed in /proc/vmallocinfo, and the remaining leaks are reported by kmemleak.

Free these allocations and unmap the MMIO region on failure to avoid the leaks.

Fixes 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery mode")
Signed-off-by Kohei Enju <[email protected]>

Hi,

It seems that ixgbe_recovery_probe() is only called from ixgbe_probe(). And that ixgbe_probe() already has an unwind ladder for these resources. So I would suggest using that rather than replicating it
in ixgbe_recovery_probe. That is, have ixgbe_probe() unwind when ixgbe_recovery_probe returns an error.

Also, maybe I'm wrong, but it seems that hw->aci.lock is initialised more than once if ixgbe_recovery_probe() is called.

...

#2 lore raw
Quoted 18 lines · Simon Horman
Quoted 8 lines · Kohei Enju

ixgbe_recovery_probe() does not free the following resources in its error path, unlike ixgbe_probe():
- adapter->io_addr
- adapter->jump_tables[0]
- adapter->mac_table
- adapter->rss_key
- adapter->af_xdp_zc_qps

Quoted 8 lines

The leaked MMIO region can be observed in /proc/vmallocinfo, and the remaining leaks are reported by kmemleak.

Free these allocations and unmap the MMIO region on failure to avoid the leaks.

Fixes 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery mode")
Signed-off-by Kohei Enju <[email protected]>
Quoted 7 lines

Hi,

It seems that ixgbe_recovery_probe() is only called from ixgbe_probe(). And that ixgbe_probe() already has an unwind ladder for these resources. So I would suggest using that rather than replicating it
in ixgbe_recovery_probe. That is, have ixgbe_probe() unwind when ixgbe_recovery_probe returns an error.

Right. If resources are allocated by ixgbe_probe() they should be freed in ixgbe_probe() and not in ixgbe_recovery_probe() which is a smaller function called by ixgbe_probe() to enter recovery mode where only devlink flash update is enabled.

It looks like most of these resources are allocated by probe and then ixgbe_recovery_probe() is called, which should instead let regular probe do cleanup for stuff it didn't setup itself.

Quoted 4 lines

Also, maybe I'm wrong, but it seems that hw->aci.lock is initialised more than once if ixgbe_recovery_probe() is called.

Its initialized in ixgbe_sw_init, which is called before the ixgbe_recovery_probe, so yes that does look like a double initialization.

Quoted 1 line

...

author
#3 lore raw
Quoted 29 lines · Jacob Keller
Quoted 18 lines · Simon Horman
Quoted 8 lines · Kohei Enju

ixgbe_recovery_probe() does not free the following resources in its error path, unlike ixgbe_probe():
- adapter->io_addr
- adapter->jump_tables[0]
- adapter->mac_table
- adapter->rss_key
- adapter->af_xdp_zc_qps

Quoted 8 lines

The leaked MMIO region can be observed in /proc/vmallocinfo, and the remaining leaks are reported by kmemleak.

Free these allocations and unmap the MMIO region on failure to avoid the leaks.

Fixes 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery mode")
Signed-off-by Kohei Enju <[email protected]>
Quoted 7 lines

Hi,

It seems that ixgbe_recovery_probe() is only called from ixgbe_probe(). And that ixgbe_probe() already has an unwind ladder for these resources. So I would suggest using that rather than replicating it
in ixgbe_recovery_probe. That is, have ixgbe_probe() unwind when ixgbe_recovery_probe returns an error.

Quoted 8 lines

Right. If resources are allocated by ixgbe_probe() they should be freed in ixgbe_probe() and not in ixgbe_recovery_probe() which is a smaller function called by ixgbe_probe() to enter recovery mode where only devlink flash update is enabled.

It looks like most of these resources are allocated by probe and then ixgbe_recovery_probe() is called, which should instead let regular probe do cleanup for stuff it didn't setup itself.

That makes sense. I'll revise the patch and work on v2.

Quoted 8 lines
Quoted 5 lines

Also, maybe I'm wrong, but it seems that hw->aci.lock is initialised more than once if ixgbe_recovery_probe() is called.

Its initialized in ixgbe_sw_init, which is called before the ixgbe_recovery_probe, so yes that does look like a double initialization.

Good catch, I overlooked that. I'll address that as well.

Thank you for taking a look, Simon and Jacob.

Reviewed-by on v1 1/1
#4 lore raw
In reply to Kohei Enju
Quoted 60 lines

-----Original Message-----
From: Intel-wired-lan <[email protected]> On Behalf Of Kohei Enju
Sent: Saturday, December 6, 2025 4:51 PM
To: [email protected]; [email protected]
Cc: Nguyen, Anthony L <[email protected]>; Kitszel, Przemyslaw <[email protected]>; Andrew Lunn <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>; Jagielski, Jedrzej <[email protected]>; Mateusz Polchlopek <[email protected]>; Wegrzyn, Stefan <[email protected]>; [email protected]; Kohei Enju <[email protected]>
Subject: [Intel-wired-lan] [PATCH iwl-net v1] ixgbe: fix memory leaks in ixgbe_recovery_probe()

ixgbe_recovery_probe() does not free the following resources in its error path, unlike ixgbe_probe():
- adapter->io_addr
- adapter->jump_tables[0]
- adapter->mac_table
- adapter->rss_key
- adapter->af_xdp_zc_qps

The leaked MMIO region can be observed in /proc/vmallocinfo, and the remaining leaks are reported by kmemleak.

Free these allocations and unmap the MMIO region on failure to avoid the leaks.

Fixes 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery

mode")

Signed-off-by Kohei Enju <[email protected]>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +5 -0
index 4af3b3e71ff1..1bfec3fffae0 100644
@@ -11508,6 +11508,11 @@ static int ixgbe_recovery_probe(struct
 ixgbe_adapter *adapter)
 	mutex_destroy(&adapter->hw.aci.lock);
 	ixgbe_release_hw_control(adapter);
 clean_up_probe:
+	iounmap(adapter->io_addr);
+	kfree(adapter->jump_tables[0]);
+	kfree(adapter->mac_table);
+	kfree(adapter->rss_key);
+	bitmap_free(adapter->af_xdp_zc_qps);
 	disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter-
 >state);
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0 -1
 	free_netdev(netdev);
 	devlink_free(adapter->devlink);
--
Quoted 1 line

2.52.0

Reviewed-by Aleksandr Loktionov <[email protected]>