* [PATCH] drm/amdgpu: fix memory leak in amdgpu_fru_get_product_info()
@ 2023-09-22 5:27 Yang Wang
2023-09-22 17:21 ` Alex Deucher
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Yang Wang @ 2023-09-22 5:27 UTC (permalink / raw)
To: amd-gfx; +Cc: Luben Tuikov, Yang Wang
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>
---
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;
}
/**
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: fix memory leak in amdgpu_fru_get_product_info()
2023-09-22 5:27 [PATCH] drm/amdgpu: fix memory leak in amdgpu_fru_get_product_info() Yang Wang
@ 2023-09-22 17:21 ` Alex Deucher
2023-09-22 20:58 ` Luben Tuikov
2023-09-22 21:09 ` Luben Tuikov
2 siblings, 0 replies; 5+ messages in thread
From: Alex Deucher @ 2023-09-22 17:21 UTC (permalink / raw)
To: Yang Wang; +Cc: Luben Tuikov, amd-gfx
On Fri, Sep 22, 2023 at 2:06 AM Yang Wang <kevinyang.wang@amd.com> 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>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> ---
> 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;
> }
>
> /**
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: fix memory leak in amdgpu_fru_get_product_info()
2023-09-22 5:27 [PATCH] drm/amdgpu: fix memory leak in amdgpu_fru_get_product_info() Yang Wang
2023-09-22 17:21 ` Alex Deucher
@ 2023-09-22 20:58 ` Luben Tuikov
2023-09-22 21:12 ` Luben Tuikov
2023-09-22 21:09 ` Luben Tuikov
2 siblings, 1 reply; 5+ messages in thread
From: Luben Tuikov @ 2023-09-22 20:58 UTC (permalink / raw)
To: Yang Wang, amd-gfx
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>
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: fix memory leak in amdgpu_fru_get_product_info()
2023-09-22 5:27 [PATCH] drm/amdgpu: fix memory leak in amdgpu_fru_get_product_info() Yang Wang
2023-09-22 17:21 ` Alex Deucher
2023-09-22 20:58 ` Luben Tuikov
@ 2023-09-22 21:09 ` Luben Tuikov
2 siblings, 0 replies; 5+ messages in thread
From: Luben Tuikov @ 2023-09-22 21:09 UTC (permalink / raw)
To: Yang Wang, amd-gfx
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>
> ---
> 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;
Actually the memory leak is ONLY here.
I wonder if you could've simply added a
kfree(pia);
> + goto Out;
> }
before the goto Out; which would result in a one-line change.
Yeah, please do that instead.
So, don't add "ret" and what not. Just add the one liner "kfree(pia);" before
the "goto Out;" and make it a SINGLE-LINE change to fix this memory leak.
You don't need so many (formulaic) changes of adding "ret" and what not.
Just a one-liner change, please.
>
> /* 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: fix memory leak in amdgpu_fru_get_product_info()
2023-09-22 20:58 ` Luben Tuikov
@ 2023-09-22 21:12 ` Luben Tuikov
0 siblings, 0 replies; 5+ messages in thread
From: Luben Tuikov @ 2023-09-22 21:12 UTC (permalink / raw)
To: Yang Wang, amd-gfx, Deucher, Alexander
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-09-22 21:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-22 5:27 [PATCH] drm/amdgpu: fix memory leak in amdgpu_fru_get_product_info() Yang Wang
2023-09-22 17:21 ` Alex Deucher
2023-09-22 20:58 ` Luben Tuikov
2023-09-22 21:12 ` Luben Tuikov
2023-09-22 21:09 ` Luben Tuikov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox