netdev - Re: RE: [Intel-wired-lan] [PATCH iwl-net v2 1/2] ixgbe: fix memory leaks in the ixgbe_recovery

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251211150416.88637-1-enjuk@amazon.com>
Date: Fri, 12 Dec 2025 00:03:10 +0900
From: Kohei Enju <enjuk@...zon.com>
To: <aleksandr.loktionov@...el.com>
CC: <andrew+netdev@...n.ch>, <anthony.l.nguyen@...el.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <enjuk@...zon.com>,
	<horms@...nel.org>, <intel-wired-lan@...ts.osuosl.org>,
	<jacob.e.keller@...el.com>, <jedrzej.jagielski@...el.com>, <kohei@...uk.org>,
	<kuba@...nel.org>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<przemyslaw.kitszel@...el.com>, <stefan.wegrzyn@...el.com>
Subject: Re: RE: [Intel-wired-lan] [PATCH iwl-net v2 1/2] ixgbe: fix memory leaks in the ixgbe_recovery_probe() path

On Thu, 11 Dec 2025 10:13:09 +0000, Loktionov, Aleksandr wrote:

>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 4af3b3e71ff1..85023bb4e5a5 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -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;
>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/b5787c94-2ad0-4519-9cdb-5e82acfebe05@intel.com/

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

On successful path, ixgbe_probe() just returns ixgbe_recovery_probe()
and don't execute following codes, so there's no change of behavior.