2010-07-09 10:49:52

by Axel Lin

Subject: [PATCH] edd: fix possible memory leak in edd_init() error path

The error may happen at any iteration of the for loop,
this patch properly unregisters already registed edd_devices in error path.

Signed-off-by: Axel Lin <[email protected]>
---
drivers/firmware/edd.c | 23 ++++++++++++++++-------
1 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
index 110e24e..b75082c 100644
--- a/drivers/firmware/edd.c
+++ b/drivers/firmware/edd.c
@@ -744,7 +744,7 @@ static inline int edd_num_devices(void)
static int __init
edd_init(void)
{
- unsigned int i;
+ int i;
int rc=0;
struct edd_device *edev;

@@ -760,21 +760,30 @@ edd_init(void)
if (!edd_kset)
return -ENOMEM;

- for (i = 0; i < edd_num_devices() && !rc; i++) {
+ for (i = 0; i < edd_num_devices(); i++) {
edev = kzalloc(sizeof (*edev), GFP_KERNEL);
- if (!edev)
- return -ENOMEM;
+ if (!edev) {
+ rc = -ENOMEM;
+ goto out;
+ }

rc = edd_device_register(edev, i);
if (rc) {
kfree(edev);
- break;
+ goto out;
}
edd_devices[i] = edev;
}

- if (rc)
- kset_unregister(edd_kset);
+ return 0;
+
+out:
+ while (--i >= 0) {
+ edev = edd_devices[i];
+ if (edev)
+ edd_device_unregister(edev);
+ }
+ kset_unregister(edd_kset);
return rc;
}

--
1.5.4.3

2010-07-11 09:01:35

by Pekka Enberg

Subject: Re: [PATCH] edd: fix possible memory leak in edd_init() error path

Axel Lin wrote:
> The error may happen at any iteration of the for loop,
> this patch properly unregisters already registed edd_devices in error path.
>
> Signed-off-by: Axel Lin <[email protected]>

Looks good to me.

Reviewed-by: Pekka Enberg <[email protected]>

> ---
> drivers/firmware/edd.c | 23 ++++++++++++++++-------
> 1 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
> index 110e24e..b75082c 100644
> --- a/drivers/firmware/edd.c
> +++ b/drivers/firmware/edd.c
> @@ -744,7 +744,7 @@ static inline int edd_num_devices(void)
> static int __init
> edd_init(void)
> {
> - unsigned int i;
> + int i;
> int rc=0;
> struct edd_device *edev;
>
> @@ -760,21 +760,30 @@ edd_init(void)
> if (!edd_kset)
> return -ENOMEM;
>
> - for (i = 0; i < edd_num_devices() && !rc; i++) {
> + for (i = 0; i < edd_num_devices(); i++) {
> edev = kzalloc(sizeof (*edev), GFP_KERNEL);
> - if (!edev)
> - return -ENOMEM;
> + if (!edev) {
> + rc = -ENOMEM;
> + goto out;
> + }
>
> rc = edd_device_register(edev, i);
> if (rc) {
> kfree(edev);
> - break;
> + goto out;
> }
> edd_devices[i] = edev;
> }
>
> - if (rc)
> - kset_unregister(edd_kset);
> + return 0;
> +
> +out:
> + while (--i >= 0) {
> + edev = edd_devices[i];
> + if (edev)
> + edd_device_unregister(edev);
> + }
> + kset_unregister(edd_kset);
> return rc;
> }
>

2010-07-12 19:49:38

by Andrew Morton

Subject: Re: [PATCH] edd: fix possible memory leak in edd_init() error path

On Fri, 09 Jul 2010 18:50:07 +0800
Axel Lin <[email protected]> wrote:

> The error may happen at any iteration of the for loop,
> this patch properly unregisters already registed edd_devices in error path.
>
> Signed-off-by: Axel Lin <[email protected]>
> ---
> drivers/firmware/edd.c | 23 ++++++++++++++++-------
> 1 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
> index 110e24e..b75082c 100644
> --- a/drivers/firmware/edd.c
> +++ b/drivers/firmware/edd.c
> @@ -744,7 +744,7 @@ static inline int edd_num_devices(void)
> static int __init
> edd_init(void)
> {
> - unsigned int i;
> + int i;
> int rc=0;
> struct edd_device *edev;
>
> @@ -760,21 +760,30 @@ edd_init(void)
> if (!edd_kset)
> return -ENOMEM;
>
> - for (i = 0; i < edd_num_devices() && !rc; i++) {
> + for (i = 0; i < edd_num_devices(); i++) {
> edev = kzalloc(sizeof (*edev), GFP_KERNEL);
> - if (!edev)
> - return -ENOMEM;
> + if (!edev) {
> + rc = -ENOMEM;
> + goto out;
> + }
>
> rc = edd_device_register(edev, i);
> if (rc) {
> kfree(edev);
> - break;
> + goto out;
> }
> edd_devices[i] = edev;
> }
>
> - if (rc)
> - kset_unregister(edd_kset);
> + return 0;
> +
> +out:
> + while (--i >= 0) {
> + edev = edd_devices[i];
> + if (edev)

This test is unneeded?

> + edd_device_unregister(edev);
> + }
> + kset_unregister(edd_kset);
> return rc;
> }

2010-07-13 01:18:20

by Axel Lin

Subject: Re: [PATCH] edd: fix possible memory leak in edd_init() error path

2010/7/13 Andrew Morton <[email protected]>:
> On Fri, 09 Jul 2010 18:50:07 +0800
> Axel Lin <[email protected]> wrote:
>
>> The error may happen at any iteration of the for loop,
>> this patch properly unregisters already registed edd_devices in error path.
>>
>> Signed-off-by: Axel Lin <[email protected]>
>> ---
>> ?drivers/firmware/edd.c | ? 23 ++++++++++++++++-------
>> ?1 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
>> index 110e24e..b75082c 100644
>> --- a/drivers/firmware/edd.c
>> +++ b/drivers/firmware/edd.c
>> @@ -744,7 +744,7 @@ static inline int edd_num_devices(void)
>> ?static int __init
>> ?edd_init(void)
>> ?{
>> - ? ? unsigned int i;
>> + ? ? int i;
>> ? ? ? int rc=0;
>> ? ? ? struct edd_device *edev;
>>
>> @@ -760,21 +760,30 @@ edd_init(void)
>> ? ? ? if (!edd_kset)
>> ? ? ? ? ? ? ? return -ENOMEM;
>>
>> - ? ? for (i = 0; i < edd_num_devices() && !rc; i++) {
>> + ? ? for (i = 0; i < edd_num_devices(); i++) {
>> ? ? ? ? ? ? ? edev = kzalloc(sizeof (*edev), GFP_KERNEL);
>> - ? ? ? ? ? ? if (!edev)
>> - ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
>> + ? ? ? ? ? ? if (!edev) {
>> + ? ? ? ? ? ? ? ? ? ? rc = -ENOMEM;
>> + ? ? ? ? ? ? ? ? ? ? goto out;
>> + ? ? ? ? ? ? }
>>
>> ? ? ? ? ? ? ? rc = edd_device_register(edev, i);
>> ? ? ? ? ? ? ? if (rc) {
>> ? ? ? ? ? ? ? ? ? ? ? kfree(edev);
>> - ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? ? ? ? ? goto out;
>> ? ? ? ? ? ? ? }
>> ? ? ? ? ? ? ? edd_devices[i] = edev;
>> ? ? ? }
>>
>> - ? ? if (rc)
>> - ? ? ? ? ? ? kset_unregister(edd_kset);
>> + ? ? return 0;
>> +
>> +out:
>> + ? ? while (--i >= 0) {
>> + ? ? ? ? ? ? edev = edd_devices[i];
>> + ? ? ? ? ? ? if (edev)
>
> This test is unneeded?

right. a revised version is on the way.

Regards,
Axel

>
>> + ? ? ? ? ? ? ? ? ? ? edd_device_unregister(edev);
>> + ? ? }
>> + ? ? kset_unregister(edd_kset);
>> ? ? ? return rc;
>> ?}
>
>

2010-07-13 01:29:40

by Andrew Morton

Subject: Re: [PATCH] edd: fix possible memory leak in edd_init() error path

On Tue, 13 Jul 2010 09:17:58 +0800 Axel Lin <[email protected]> wrote:

> >> +
> >> +out:
> >> + __ __ while (--i >= 0) {
> >> + __ __ __ __ __ __ edev = edd_devices[i];
> >> + __ __ __ __ __ __ if (edev)
> >
> > This test is unneeded?
>
> right. a revised version is on the way.

I already did the below, which you were cc'ed on?

--- a/drivers/firmware/edd.c~edd-fix-possible-memory-leak-in-edd_init-error-path-fix
+++ a/drivers/firmware/edd.c
@@ -778,11 +778,8 @@ edd_init(void)
return 0;

out:
- while (--i >= 0) {
- edev = edd_devices[i];
- if (edev)
- edd_device_unregister(edev);
- }
+ while (--i >= 0)
+ edd_device_unregister(edd_devices[i]);
kset_unregister(edd_kset);
return rc;
}
_