edd: fix possible memory leak in edd_init() error path
PATCH 1/1 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(-)
drivers/firmware/edd.c +16 -7
@@ -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; }
Signature · 1 line
1.5.4.3
Quoted 4 lines · Axel Lin
The error may happen at any iteration of the for loop,
this patch properly unregisters already registededd_devicesin error path.Signed-off-by Axel Lin <[email protected]>
Looks good to me.
Reviewed-by Pekka Enberg <[email protected]>
Quoted 54 lines
--- drivers/firmware/edd.c | 23 ++++++++++++++++------- 1 files changed, 16 insertions(+), 7 deletions(-)drivers/firmware/edd.c +16
-7@@ -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; }
Quoted 42 lines · 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 registededd_devicesin error path.Signed-off-by Axel Lin <[email protected]>--- drivers/firmware/edd.c | 23 ++++++++++++++++------- 1 files changed, 16 insertions(+), 7 deletions(-)drivers/firmware/edd.c +7
-5@@ -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;
drivers/firmware/edd.c edd_init(void) · line 783
Quoted 10 lines
drivers/firmware/edd.c +6
-2} -if (rc) -kset_unregister (edd_kset); +return 0 ; + +out: +while (--i >=0 ) { + edev = edd_devices[i]; +if (edev)
This test is unneeded?
Quoted 5 lines
drivers/firmware/edd.c +3
-0+edd_device_unregister (edev); + } +kset_unregister (edd_kset);return rc; }
2010/7/13 Andrew Morton <[email protected]>:
Quoted 47 lines
Quoted 44 lines · 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 registededd_devicesin error path.Signed-off-by Axel Lin <[email protected]>---
drivers/firmware/edd.c | 23 ++++++++++++++++-------1 files changed, 16 insertions(+), 7 deletions(-)drivers/firmware/edd.c +0
-0@@ -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;
structedd_device*edev;drivers/firmware/edd.c +6
-4@@ -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; }
Quoted 10 lines
drivers/firmware/edd.c edd_init(void) · line 783
Quoted 8 lines
drivers/firmware/edd.c +6
-2- 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
Quoted 8 lines
Quoted 6 lines
drivers/firmware/edd.c +3
-0+ edd_device_unregister(edev); + } + kset_unregister(edd_kset); return rc; }
Quoted 9 lines · On Tue, 13 Jul 2010 09:17:58 +0800 Axel Lin <[email protected]> wrote:
Quoted 7 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ +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?
drivers/firmware/edd.c renamed from drivers/firmware/edd.c~edd-fix-possible-memory-leak-in-edd_init-error-path-fix +2 -5
@@ -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; }
_