From: Pekka Enberg on
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 <axel.lin(a)gmail.com>

Looks good to me.

Reviewed-by: Pekka Enberg <penberg(a)cs.helsinki.fi>

> ---
> 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;
> }
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on
On Fri, 09 Jul 2010 18:50:07 +0800
Axel Lin <axel.lin(a)gmail.com> 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 <axel.lin(a)gmail.com>
> ---
> 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;
> }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Axel Lin on
2010/7/13 Andrew Morton <akpm(a)linux-foundation.org>:
> On Fri, 09 Jul 2010 18:50:07 +0800
> Axel Lin <axel.lin(a)gmail.com> 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 <axel.lin(a)gmail.com>
>> ---
>> �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;
>> �}
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on
On Tue, 13 Jul 2010 09:17:58 +0800 Axel Lin <axel.lin(a)gmail.com> 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;
}
_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/