Thermal:Fix memory leak if occur goto unregister

PATCH Superseded Yao Dongdong 3 participants 5 messages Oct 20, 2014 – Nov 4, 2014 Thermal

Share & tools

R, Durgadoss

2014-11-03 18:17 UTC #2 lore raw

Quoted 8 lines

-----Original Message-----
From: [email protected] [mailto:linux-pm- [email protected]] On Behalf Of Eduardo Valentin
Sent: Monday, October 20, 2014 6:04 PM
To: Yao Dongdong
Cc: Zhang, Rui; [email protected]; LKML
Subject: Re: [PATCH 1/2] Thermal:Fix memory leak if occur goto unregister

Quoted 9 lines

Hello,
Quoted 1 line · Yao Dongdong
Signed-off-by [email protected]
Acked-by Eduardo Valentin <[email protected]>
Rui, would you take care of this?

If I remember it right, this 'tz' is freed in the thermal_release() function, during device_unregister().

It is similar in all other functions in thermal_core.c

So, Yao, Did you really test this patch ?
And did not see any crashes ?

Thanks,
Durga

Quoted 22 lines

drivers/thermal/thermal_core.c struct thermal_zone_device
Quoted 10 lines
---
 drivers/thermal/thermal_core.c | 1 +
 1 file changed, 1 insertion(+)
drivers/thermal/thermal_core.c +0 -0
index 71b0ec0..5b7d466 100644
@@ -1574,6 +1574,7 @@ struct thermal_zone_device
*thermal_zone_device_register(const char *type,
Quoted 11 lines
drivers/thermal/thermal_core.c +1 -1
 unregister:
    release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
    device_unregister(&tz->device);
+   kfree(tz);
    return ERR_PTR(result);
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_register);
--
1.8.0.1

Eduardo Valentin

2014-11-03 22:59 UTC #3 lore raw

Hello,

Quoted 18 lines · Durgadoss

Quoted 8 lines
-----Original Message-----
From: [email protected] [mailto:linux-pm- [email protected]] On Behalf Of Eduardo Valentin
Sent: Monday, October 20, 2014 6:04 PM
To: Yao Dongdong
Cc: Zhang, Rui; [email protected]; LKML
Subject: Re: [PATCH 1/2] Thermal:Fix memory leak if occur goto unregister
Quoted 9 lines
Hello,
Quoted 1 line · Yao Dongdong
Signed-off-by [email protected]
Acked-by Eduardo Valentin <[email protected]>
Rui, would you take care of this?

Quoted 8 lines

If I remember it right, this 'tz' is freed in the thermal_release() function, during device_unregister().
It is similar in all other functions in thermal_core.c
So, Yao, Did you really test this patch ?
And did not see any crashes ?

Yao, reading the patch change carefully, now I see that Durga is correct here.

Quoted 18 lines

Thanks,
Durga
Quoted 15 lines
drivers/thermal/thermal_core.c struct thermal_zone_device
Quoted 10 lines
---
 drivers/thermal/thermal_core.c | 1 +
 1 file changed, 1 insertion(+)
drivers/thermal/thermal_core.c +0 -0
index 71b0ec0..5b7d466 100644
@@ -1574,6 +1574,7 @@ struct thermal_zone_device
*thermal_zone_device_register(const char *type,
Quoted 4 lines
drivers/thermal/thermal_core.c +1 -0
 unregister:
    release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
    device_unregister(&tz->device);
+   kfree(tz);

The device unregister is called above your kfree call, which will cause the thermal_release to be called. Did you test the code for double kfree calls? Your patch probably inserts a memory corruption.

I will revert your patch from my tree until you provide an answer about your testing.

Thanks,

Eduardo Valentin

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
drivers/thermal/thermal_core.c +0 -1
    return ERR_PTR(result);
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_register);
--
1.8.0.1

Yao Dongdong

author

2014-11-04 02:01 UTC #4 lore raw

Quoted 47 lines · Eduardo Valentin

Hello,
Quoted 16 lines · Durgadoss
Quoted 8 lines
-----Original Message-----
From: [email protected] [mailto:linux-pm- [email protected]] On Behalf Of Eduardo Valentin
Sent: Monday, October 20, 2014 6:04 PM
To: Yao Dongdong
Cc: Zhang, Rui; [email protected]; LKML
Subject: Re: [PATCH 1/2] Thermal:Fix memory leak if occur goto unregister
Quoted 8 lines
Hello,
Quoted 1 line · Yao Dongdong
Signed-off-by [email protected]
Acked-by Eduardo Valentin <[email protected]>
Rui, would you take care of this?
Quoted 8 lines
If I remember it right, this 'tz' is freed in the thermal_release() function, during device_unregister().
It is similar in all other functions in thermal_core.c
So, Yao, Did you really test this patch ?
And did not see any crashes ?
Yao, reading the patch change carefully, now I see that Durga is correct here.
Quoted 17 lines
Thanks,
Durga
Quoted 14 lines
drivers/thermal/thermal_core.c struct thermal_zone_device
Quoted 9 lines
---
 drivers/thermal/thermal_core.c | 1 +
 1 file changed, 1 insertion(+)
drivers/thermal/thermal_core.c +0 -0
index 71b0ec0..5b7d466 100644
@@ -1574,6 +1574,7 @@ struct thermal_zone_device
*thermal_zone_device_register(const char *type,
Quoted 4 lines
drivers/thermal/thermal_core.c +1 -0
 unregister:
    release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
    device_unregister(&tz->device);
+   kfree(tz);

Quoted 6 lines

The device unregister is called above your kfree call, which will cause the thermal_release to be called. Did you test the code for double kfree calls? Your patch probably inserts a memory corruption.
I will revert your patch from my tree until you provide an answer about your testing.

Yes, i have checked it and you are right, the patch will double kfree tz.

Thansks Durga for correcting me.

Quoted 12 lines

Thanks,
Eduardo Valentin
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/thermal/thermal_core.c +0 -1
    return ERR_PTR(result);
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_register);
--
1.8.0.1