Re: [PATCH] drm/amdgpu: fix memory leak in amdgpu_fru_get_product_info()

[Luben Tuikov <luben.tuikov@amd.com>]

On 2023-09-22 16:58, Luben Tuikov wrote:
> On 2023-09-22 01:27, Yang Wang wrote:
>> fix a memory leak that occurs when csum is 0,
>> the origin function will return directly and forgets to free 'pia' resource.
>>
>> Fixes: 0dbf2c562625 ("drm/amdgpu: Interpret IPMI data for product information (v2)")
>>
>> CC: Luben Tuikov <luben.tuikov@amd.com>
>> Signed-off-by: Yang Wang <kevinyang.wang@amd.com>
> 
> Ah, yes, we should free "pia". Good catch!
> 
> Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>

Retracted--see my follow-up email of making this a one-liner change, instead
of adding a whole bunch of changes which are unnecessary to fix this memory leak.

Regards,
Luben

> 
> Regards,
> Luben
> 
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>> index 401651f28ba2..50b6eb447726 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>> @@ -111,7 +111,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>>  {
>>  	unsigned char buf[8], *pia;
>>  	u32 addr, fru_addr;
>> -	int size, len;
>> +	int size, len, ret = 0;
>>  	u8 csum;
>>  
>>  	if (!is_fru_eeprom_supported(adev, &fru_addr))
>> @@ -171,16 +171,17 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>>  	/* Read the whole PIA. */
>>  	len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, pia, size);
>>  	if (len != size) {
>> -		kfree(pia);
>>  		DRM_ERROR("Couldn't read the Product Info Area: %d", len);
>> -		return len < 0 ? len : -EIO;
>> +		ret = len < 0 ? len : -EIO;
>> +		goto Out;
>>  	}
> 
> 
>>  
>>  	for (csum = 0; size > 0; size--)
>>  		csum += pia[size - 1];
>>  	if (csum) {
>>  		DRM_ERROR("Bad Product Info Area checksum: 0x%02x", csum);
>> -		return -EIO;
>> +		ret = -EIO;
>> +		goto Out;
>>  	}
>>  
>>  	/* Now extract useful information from the PIA.
>> @@ -220,7 +221,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>>  	adev->serial[sizeof(adev->serial) - 1] = '\0';
>>  Out:
>>  	kfree(pia);
>> -	return 0;
>> +	return ret;
>>  }
>>  
>>  /**
> 

-- 
Regards,
Luben