ixgbe: fix issues around ixgbe_recovery_probe()
Subsystems (3)
v2 cover ixgbe: fix issues around ixgbe_recovery_probe()
ixgbe_recovery_probe() and codes around this function have two bugs:
1. resource freeing up is not complete, resulting in memory leaks
2. mutex lock (hw->aci.lock) is initialized twice
Fix these issues.
Changes:
v2:
- let ixgbe_probe() to clean up, instead of ixgbe_recovery_probe()
- don't initialize aci lock twice
v1: https://lore.kernel.org/intel-wired-lan/[email protected]/
Kohei Enju (2):
ixgbe: fix memory leaks in the ixgbe_recovery_probe() path
ixgbe: don't initialize aci lock in ixgbe_recovery_probe()
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-)
Signature · 1 line
2.52.0
PATCH v2 1/2 ixgbe: fix memory leaks in the ixgbe_recovery_probe() path
When ixgbe_recovery_probe() is invoked and this function fails, allocated resources in advance are not completely freed, because ixgbe_probe() returns ixgbe_recovery_probe() directly and ixgbe_recovery_probe() only frees partial resources, resulting in memory leaks including:
- 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.
Don't return ixgbe_recovery_probe() directly, and instead let ixgbe_probe() to clean up resources on failures.
--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +8 -12
@@ -11468,14 +11468,12 @@ static void ixgbe_set_fw_version(struct ixgbe_adapter *adapter)*/ static int ixgbe_recovery_probe (struct ixgbe_adapter *adapter) { -struct net_device *netdev = adapter->netdev;struct pci_dev *pdev = adapter->pdev;struct ixgbe_hw *hw = &adapter->hw; -bool disable_dev;int err = -EIO ;if (hw->mac.type != ixgbe_mac_e610) - goto clean_up_probe; +return err;ixgbe_get_hw_control (adapter);mutex_init (&hw->aci.lock);
@@ -11507,13 +11505,6 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter) shutdown_aci:mutex_destroy (&adapter->hw.aci.lock);ixgbe_release_hw_control (adapter); -clean_up_probe: - disable_dev = !test_and_set_bit (__IXGBE_DISABLED, &adapter->state); -free_netdev (netdev); -devlink_free (adapter->devlink); -pci_release_mem_regions (pdev); -if (disable_dev) -pci_disable_device (pdev);return err; }
@@ -11655,8 +11646,13 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)if (err) goto err_sw_init; -if (ixgbe_check_fw_error (adapter)) -return ixgbe_recovery_probe (adapter); +if (ixgbe_check_fw_error (adapter)) { + err =ixgbe_recovery_probe (adapter); +if (err) + goto err_sw_init; + +return 0 ; + }if (adapter->hw.mac.type == ixgbe_mac_e610) { err =ixgbe_get_caps (&adapter->hw);
Signature · 1 line
2.52.0
RE: [Intel-wired-lan] [PATCH iwl-net v2 1/2] ixgbe: fix memory leaks in the ixgbe_recovery_probe() path
Quoted 52 lines
-----Original Message-----
From: Intel-wired-lan <[email protected]> On Behalf Of Kohei Enju
Sent: Thursday, December 11, 2025 10:16 AM
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]>; Wegrzyn, Stefan <[email protected]>; Simon Horman <[email protected]>; Keller, Jacob E <[email protected]>; [email protected]; Kohei Enju <[email protected]>
Subject: [Intel-wired-lan] [PATCH iwl-net v2 1/2] ixgbe: fix memory leaks in theixgbe_recovery_probe()pathWhen
ixgbe_recovery_probe()is invoked and this function fails, allocated resources in advance are not completely freed, becauseixgbe_probe()returnsixgbe_recovery_probe()directly andixgbe_recovery_probe()only frees partial resources, resulting in memory leaks including:
-adapter->io_addr
- adapter->jump_tables[0]
-adapter->mac_table
-adapter->rss_key
-adapter->af_xdp_zc_qpsThe leaked MMIO region can be observed in
/proc/vmallocinfo, and the remaining leaks are reported by kmemleak.Don't return
ixgbe_recovery_probe()directly, and instead letixgbe_probe()to clean up resources on failures.Fixes 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recoverymode")
Signed-off-by Kohei Enju <[email protected]>--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 20 ++++++++----------(quoted diff) +0
-1- 1 file changed, 8 insertions(+), 12 deletions(-)drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-0@@ -11468,14 +11468,12 @@ static void ixgbe_set_fw_version(structixgbe_adapter *adapter) */
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static void ixgbe_set_fw_version(struct · line 11476
Quoted 10 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +1
-3static int ixgbe_recovery_probe (struct ixgbe_adapter *adapter) { -struct net_device *netdev = adapter->netdev;struct pci_dev *pdev = adapter->pdev;struct ixgbe_hw *hw = &adapter->hw; -bool disable_dev;int err = -EIO ;if (hw->mac.type != ixgbe_mac_e610) - goto clean_up_probe; +return err;
You've removed the clean_up_probe: label and its cleanup code (free_netdev, devlink_free, pci_release_mem_regions, pci_disable_device) without providing a proof that ixgbe_probe()'s error path correctly handles all these resources. I'm afraid this patch may trade one leak for another, or cause double-free issues.
Quoted 20 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-7ixgbe_get_hw_control (adapter);mutex_init (&hw->aci.lock);@@ -11507,13 +11505,6 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter) shutdown_aci: mutex_destroy(&adapter->hw.aci.lock);ixgbe_release_hw_control (adapter); -clean_up_probe: - disable_dev = !test_and_set_bit (__IXGBE_DISABLED, &adapter- >state); -free_netdev (netdev); -devlink_free (adapter->devlink); -pci_release_mem_regions (pdev); -if (disable_dev) -pci_disable_device (pdev);return err;}
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_probe(struct pci_dev *pdev, · line 11653
Quoted 10 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +4
-2@@ -11655,8 +11646,13 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (err) goto err_sw_init; - if (ixgbe_check_fw_error(adapter)) - return ixgbe_recovery_probe(adapter); + if (ixgbe_check_fw_error(adapter)) { + err =ixgbe_recovery_probe (adapter); +if (err) + goto err_sw_init;
The early return 0; on successful ixgbe_recovery_probe() bypasses the remainder of ixgbe_probe().
The commit message doesn't explain what subsequent initialization steps (if any) are intentionally skipped in recovery mode, or whether this is correct behavior.
Quoted 8 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +3
-0+ +return 0 ; + }if (adapter->hw.mac.type == ixgbe_mac_e610) {err =
ixgbe_get_caps(&adapter->hw);
--
2.52.0
Re: RE: [Intel-wired-lan] [PATCH iwl-net v2 1/2] ixgbe: fix memory leaks in the ixgbe_recovery_probe() path
Quoted 15 lines · Aleksandr
Quoted 15 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-2@@ -11468,14 +11468,12 @@ static void ixgbe_set_fw_version(structixgbe_adapter *adapter) */ static int ixgbe_recovery_probe (struct ixgbe_adapter *adapter) { -struct net_device *netdev = adapter->netdev;struct pci_dev *pdev = adapter->pdev;struct ixgbe_hw *hw = &adapter->hw; -bool disable_dev;int err = -EIO ;
Quoted 5 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static void ixgbe_set_fw_version(struct ixgbe_adapter *adapter) · line 11476
Quoted 3 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +1
-1if (hw->mac.type != ixgbe_mac_e610) - goto clean_up_probe; +return err;You've removed the
clean_up_probe: label and its cleanup code (free_netdev,devlink_free,pci_release_mem_regions,pci_disable_device) without providing a proof that ixgbe_probe()'s error path correctly handles all these resources. I'm afraid this patch may trade one leak for another, or cause double-free issues.
Hi Alex, thank you for taking a look.
First, ixgbe_recovery_probe() is a static function and is only called from ixgbe_probe(), so potential affected scope is limited to the ixgbe_probe() function (at least for now). Also I don't think ixgbe_recovery_probe() is a common util function which would be used in other functions than ixgbe_probe().
Also I changed ixgbe_probe() to cleanup those resources when ixgbe_recovery_probe() fails to keep consistency, just because those resources are allocated by ixgbe_probe(), not by ixgbe_recovery_probe().
Considering the conversation I had in v1 patch [1], I think this change would be acceptable, and rather preferable to reduce the possibility of future regression.
[1] https://lore.kernel.org/all/[email protected]/
Quoted 5 lines
Quoted 5 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-0@@ -11655,8 +11646,13 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (err) goto err_sw_init;
Quoted 8 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent) · line 11652
Quoted 6 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +4
-2-if (ixgbe_check_fw_error (adapter)) -return ixgbe_recovery_probe (adapter); +if (ixgbe_check_fw_error (adapter)) { + err =ixgbe_recovery_probe (adapter); +if (err) + goto err_sw_init;The early return 0; on successful
ixgbe_recovery_probe()bypasses the remainder ofixgbe_probe().
The commit message doesn't explain what subsequent initialization steps (if any) are intentionally skipped in recovery mode, or whether this is correct behavior.
On successful path, ixgbe_probe() just returns ixgbe_recovery_probe() and don't execute following codes, so there's no change of behavior.
PATCH v2 2/2 ixgbe: don't initialize aci lock in ixgbe_recovery_probe()
In reply to Kohei Enjuhw->aci.lock is already initialized in ixgbe_sw_init(), so ixgbe_recovery_probe() doesn't need to initialize the lock. This function is also not responsible for destroying the lock on failures.
Additionally, change the name of label in accordance with this change.
--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +2 -4
@@ -11476,10 +11476,9 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)return err;ixgbe_get_hw_control (adapter); -mutex_init (&hw->aci.lock); err =ixgbe_get_flash_data (&adapter->hw);if (err) - goto shutdown_aci; + goto err_release_hw_control;timer_setup (&adapter->service_timer, ixgbe_service_timer,0 );INIT_WORK (&adapter->service_task, ixgbe_recovery_service_task);
@@ -11502,8 +11501,7 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)devl_unlock (adapter->devlink);return 0 ; -shutdown_aci: -mutex_destroy (&adapter->hw.aci.lock); +err_release_hw_control:ixgbe_release_hw_control (adapter);return err; }
Signature · 1 line
2.52.0
RE: [Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't initialize aci lock in ixgbe_recovery_probe()
Quoted 19 lines
-----Original Message-----
From: Intel-wired-lan <[email protected]> On Behalf Of Kohei Enju
Sent: Thursday, December 11, 2025 10:16 AM
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]>; Wegrzyn, Stefan <[email protected]>; Simon Horman <[email protected]>; Keller, Jacob E <[email protected]>; [email protected]; Kohei Enju <[email protected]>
Subject: [Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't initialize aci lock inixgbe_recovery_probe()
Quoted 2 lines
hw->aci.lockis already initialized inixgbe_sw_init(), soixgbe_recovery_probe()doesn't need to initialize the lock. This
You claim that ixgbe_sw_init() initializes hw->aci.lock but don't provide evidence(s). Can you?
Quoted 45 lines
function is also not responsible for destroying the lock on failures.
Additionally, change the name of label in accordance with this change.
Fixes 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recoverymode")
Reported-by Simon Horman <[email protected]>@horms.kernel.org/
Signed-off-by Kohei Enju <[email protected]>--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +1
-2@@ -11476,10 +11476,9 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)return err;ixgbe_get_hw_control (adapter); -mutex_init (&hw->aci.lock); err =ixgbe_get_flash_data (&adapter->hw);if (err) - goto shutdown_aci; + goto err_release_hw_control;timer_setup (&adapter->service_timer, ixgbe_service_timer,0 );drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +1
-2INIT_WORK (&adapter->service_task, ixgbe_recovery_service_task);@@ -11502,8 +11501,7 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter) devl_unlock(adapter->devlink);return 0 ; -shutdown_aci: -mutex_destroy (&adapter->hw.aci.lock); +err_release_hw_control:ixgbe_release_hw_control (adapter);return err;}
--
2.52.0
Re: RE: [Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't initialize aci lock in ixgbe_recovery_probe()
Quoted 7 lines · Aleksandr
Quoted 5 lines
Subject: [Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't initialize aci lock in
ixgbe_recovery_probe()
hw->aci.lockis already initialized inixgbe_sw_init(), soixgbe_recovery_probe()doesn't need to initialize the lock. ThisYou claim that
ixgbe_sw_init()initializeshw->aci.lockbut don't provide evidence(s). Can you?
Hi Alex, thank you for reviewing!
Yeah, I claim that because currently ixgbe_recovery_probe() is only called from ixgbe_probe(), and this is called after ixgbe_sw_init(). Also I don't expect ixgbe_recovery_probe() would be called from other contexts in the future.
We confirmed the that double initialization would occur in the context[1], but are there any recommended solutions we can adopt?
I understand that double initialization doesn't always introduce realistic issue because it would be problematic only when reinialization is done while the lock is held, but it's a fact that actually unnecessary initialization is done in ixgbe_recovery_probe().
I believe this change would be right, but maybe we should ask Jedrzej for the intention of mutex_init() in ixgbe_recovery_probe(), and possibility that ixgbe_recovery_probe() would be called from any other contexts.