[PATCH 0/3] thermal: core: Fix some error handling code in 'thermal_zone_device_register()'

[PATCH 0/3] thermal: core: Fix some error handling code in 'thermal_zone_device_register()'

[Christophe JAILLET]

These 3 patches are all related to error handling in
'thermal_zone_device_register()'. I've splitted them in 3 because each
fixes or cleans something different. They could also be merged together
because they are all related to the same few lines of code.

The 1st one fixes in memory leak.
The 2nd reorders code in the error handling path to have it more logical.
The 3rd simplifies the code.

Not 100% sure, but the 2nd one could also avoid an OOPS because we
try to unregister something that has never been registered.

Christophe JAILLET (3):
  thermal: core: Fix a memory leak in 'thermal_zone_device_register()'
    error handling path
  thermal: core: Reorder 'thermal_zone_device_register()' error handling
    code
  thermal: core: Avoid code duplication in
    'thermal_zone_device_register()'

 drivers/thermal/thermal_core.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

-- 
2.11.0

[PATCH 1/3] thermal: core: Fix a memory leak in 'thermal_zone_device_register()' error handling path

[Christophe JAILLET]

'tz' is freed in some error handling paths but not in the main one.
So free it also here to avoid a memory leak.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/thermal/thermal_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 5a51c740e372..9743f3e65eb0 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1296,6 +1296,7 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 unregister:
 	ida_simple_remove(&thermal_tz_ida, tz->id);
 	device_unregister(&tz->device);
+	kfree(tz);
 	return ERR_PTR(result);
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_register);
-- 
2.11.0

[PATCH 2/3] thermal: core: Reorder 'thermal_zone_device_register()' error handling code

[Christophe JAILLET]

Reorder code in the error handling path in order to match the way resources
have been allocated.

With this new order, we can avoid a call to 'device_unregister()' if
'thermal_zone_create_device_groups'()' fails. At this point,
'device_register()' has not been called yet.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/thermal/thermal_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 9743f3e65eb0..c58714800660 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1232,7 +1232,7 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 	/* Add nodes that are always present via .groups */
 	result = thermal_zone_create_device_groups(tz, mask);
 	if (result)
-		goto unregister;
+		goto remove_id;
 
 	/* A new thermal zone needs to be updated anyway. */
 	atomic_set(&tz->need_update, 1);
@@ -1294,8 +1294,9 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 	return tz;
 
 unregister:
-	ida_simple_remove(&thermal_tz_ida, tz->id);
 	device_unregister(&tz->device);
+remove_id:
+	ida_simple_remove(&thermal_tz_ida, tz->id);
 	kfree(tz);
 	return ERR_PTR(result);
 }
-- 
2.11.0

[PATCH 3/3] thermal: core: Avoid code duplication in 'thermal_zone_device_register()'

[Christophe JAILLET]

Jump in the error handling path in order to avoid code duplication if
some function fail.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/thermal/thermal_core.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index c58714800660..fe4b812eeb12 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1213,10 +1213,8 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 	ida_init(&tz->ida);
 	mutex_init(&tz->lock);
 	result = ida_simple_get(&thermal_tz_ida, 0, 0, GFP_KERNEL);
-	if (result < 0) {
-		kfree(tz);
-		return ERR_PTR(result);
-	}
+	if (result < 0)
+		goto free_tz;
 
 	tz->id = result;
 	strlcpy(tz->type, type, sizeof(tz->type));
@@ -1239,11 +1237,8 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 
 	dev_set_name(&tz->device, "thermal_zone%d", tz->id);
 	result = device_register(&tz->device);
-	if (result) {
-		ida_simple_remove(&thermal_tz_ida, tz->id);
-		kfree(tz);
-		return ERR_PTR(result);
-	}
+	if (result)
+		goto remove_id;
 
 	for (count = 0; count < trips; count++) {
 		if (tz->ops->get_trip_type(tz, count, &trip_type))
@@ -1297,6 +1292,7 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 	device_unregister(&tz->device);
 remove_id:
 	ida_simple_remove(&thermal_tz_ida, tz->id);
+free_tz:
 	kfree(tz);
 	return ERR_PTR(result);
 }
-- 
2.11.0

Re: [PATCH 1/3] thermal: core: Fix a memory leak in 'thermal_zone_device_register()' error handling path

[Zhang Rui]

On Sun, 2017-07-16 at 08:59 +0200, Christophe JAILLET wrote:
> 'tz' is freed in some error handling paths but not in the main one.
> So free it also here to avoid a memory leak.
> 
After device registered, tz is freed in thermal_release().

thanks,
rui
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/thermal/thermal_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index 5a51c740e372..9743f3e65eb0 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1296,6 +1296,7 @@ thermal_zone_device_register(const char *type,
> int trips, int mask,
>  unregister:
>  	ida_simple_remove(&thermal_tz_ida, tz->id);
>  	device_unregister(&tz->device);
> +	kfree(tz);
>  	return ERR_PTR(result);
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_device_register);

Re: [PATCH 2/3] thermal: core: Reorder 'thermal_zone_device_register()' error handling code

[Zhang Rui]

On Sun, 2017-07-16 at 08:59 +0200, Christophe JAILLET wrote:
> Reorder code in the error handling path in order to match the way
> resources
> have been allocated.
> 
> With this new order, we can avoid a call to 'device_unregister()' if
> 'thermal_zone_create_device_groups'()' fails. At this point,
> 'device_register()' has not been called yet.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/thermal/thermal_core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index 9743f3e65eb0..c58714800660 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1232,7 +1232,7 @@ thermal_zone_device_register(const char *type,
> int trips, int mask,
>  	/* Add nodes that are always present via .groups */
>  	result = thermal_zone_create_device_groups(tz, mask);
>  	if (result)
> -		goto unregister;
> +		goto remove_id;
> 
I agree we should release ida and free tz, like you did in this patch.

But the problem is in the code below, where device_register() fails,
we should free the resources allocated in
thermal_zone_create_device_groups() explicitly.

thanks,
rui

>  	/* A new thermal zone needs to be updated anyway. */
>  	atomic_set(&tz->need_update, 1);
> @@ -1294,8 +1294,9 @@ thermal_zone_device_register(const char *type,
> int trips, int mask,
>  	return tz;
>  
>  unregister:
> -	ida_simple_remove(&thermal_tz_ida, tz->id);
>  	device_unregister(&tz->device);
> +remove_id:
> +	ida_simple_remove(&thermal_tz_ida, tz->id);
>  	kfree(tz);
>  	return ERR_PTR(result);
>  }

Re: [PATCH 2/3] thermal: core: Reorder 'thermal_zone_device_register()' error handling code

[Christophe JAILLET]

Le 08/08/2017 à 10:49, Zhang Rui a écrit :
> On Sun, 2017-07-16 at 08:59 +0200, Christophe JAILLET wrote:
>> Reorder code in the error handling path in order to match the way
>> resources
>> have been allocated.
>>
>> With this new order, we can avoid a call to 'device_unregister()' if
>> 'thermal_zone_create_device_groups'()' fails. At this point,
>> 'device_register()' has not been called yet.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   drivers/thermal/thermal_core.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_core.c
>> b/drivers/thermal/thermal_core.c
>> index 9743f3e65eb0..c58714800660 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -1232,7 +1232,7 @@ thermal_zone_device_register(const char *type,
>> int trips, int mask,
>>   	/* Add nodes that are always present via .groups */
>>   	result = thermal_zone_create_device_groups(tz, mask);
>>   	if (result)
>> -		goto unregister;
>> +		goto remove_id;
>>
> I agree we should release ida and free tz, like you did in this patch.
>
> But the problem is in the code below, where device_register() fails,
> we should free the resources allocated in
> thermal_zone_create_device_groups() explicitly.
>
> thanks,
> rui
Hi,

Thanks for the review.


I will propose a v2 patch serie with some new helper functions:
    void destroy_trip_attrs(struct thermal_zone_device *tz)
    void thermal_zone_destroy_device_groups(struct thermal_zone_device *tz)

'thermal_zone_destroy_device_groups()' will then be called in the error 
handling path of 'thermal_zone_device_register()', when 
'device_register()' fails.


Would you like me to keep the same patch granularity:
    - (new patch in the serie) - Add some new helper functions to free 
resources
    - add kfree(tz) in the actual error handling path    (despite your 
comment on patch 1/3, I still think it is needed in thie error handling 
path)
    - reorder error handling code
    - avoid code duplication

Or the 3 last ones can be merged together under a generic "Fix resources 
release in error paths in thermal_zone_device_register()" ?

CJ

>>   	/* A new thermal zone needs to be updated anyway. */
>>   	atomic_set(&tz->need_update, 1);
>> @@ -1294,8 +1294,9 @@ thermal_zone_device_register(const char *type,
>> int trips, int mask,
>>   	return tz;
>>   
>>   unregister:
>> -	ida_simple_remove(&thermal_tz_ida, tz->id);
>>   	device_unregister(&tz->device);
>> +remove_id:
>> +	ida_simple_remove(&thermal_tz_ida, tz->id);
>>   	kfree(tz);
>>   	return ERR_PTR(result);
>>   }

Re: [PATCH 2/3] thermal: core: Reorder 'thermal_zone_device_register()' error handling code

[Zhang Rui]

On Tue, 2017-08-08 at 14:31 +0200, Christophe JAILLET wrote:
> Le 08/08/2017 à 10:49, Zhang Rui a écrit :
> > 
> > On Sun, 2017-07-16 at 08:59 +0200, Christophe JAILLET wrote:
> > > 
> > > Reorder code in the error handling path in order to match the way
> > > resources
> > > have been allocated.
> > > 
> > > With this new order, we can avoid a call to 'device_unregister()'
> > > if
> > > 'thermal_zone_create_device_groups'()' fails. At this point,
> > > 'device_register()' has not been called yet.
> > > 
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > >   drivers/thermal/thermal_core.c | 5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/thermal/thermal_core.c
> > > b/drivers/thermal/thermal_core.c
> > > index 9743f3e65eb0..c58714800660 100644
> > > --- a/drivers/thermal/thermal_core.c
> > > +++ b/drivers/thermal/thermal_core.c
> > > @@ -1232,7 +1232,7 @@ thermal_zone_device_register(const char
> > > *type,
> > > int trips, int mask,
> > >   	/* Add nodes that are always present via .groups */
> > >   	result = thermal_zone_create_device_groups(tz, mask);
> > >   	if (result)
> > > -		goto unregister;
> > > +		goto remove_id;
> > > 
> > I agree we should release ida and free tz, like you did in this
> > patch.
> > 
> > But the problem is in the code below, where device_register()
> > fails,
> > we should free the resources allocated in
> > thermal_zone_create_device_groups() explicitly.
> > 
> > thanks,
> > rui
> Hi,
> 
> Thanks for the review.
> 
> 
> I will propose a v2 patch serie with some new helper functions:
>     void destroy_trip_attrs(struct thermal_zone_device *tz)
>     void thermal_zone_destroy_device_groups(struct
> thermal_zone_device *tz)
> 
> 'thermal_zone_destroy_device_groups()' will then be called in the
> error 
> handling path of 'thermal_zone_device_register()', when 
> 'device_register()' fails.
> 
> 
> Would you like me to keep the same patch granularity:
>     - (new patch in the serie) - Add some new helper functions to
> free 
> resources

agreed.

>     - add kfree(tz) in the actual error handling path    (despite
> your 
> comment on patch 1/3, I still think it is needed in thie error
> handling 
> path)
>     - reorder error handling code
>     - avoid code duplication

we don't need to invoke kfree(tz) after device unregistered.
so if you want to share error handling code, there should be two error
handling paths, one before device registered, which needs kfree(tz),
and one after device registered.

thanks,
rui

> 
> Or the 3 last ones can be merged together under a generic "Fix
> resources 
> release in error paths in thermal_zone_device_register()" ?
> 
> CJ
> 
> > 
> > > 
> > >   	/* A new thermal zone needs to be updated anyway. */
> > >   	atomic_set(&tz->need_update, 1);
> > > @@ -1294,8 +1294,9 @@ thermal_zone_device_register(const char
> > > *type,
> > > int trips, int mask,
> > >   	return tz;
> > >   
> > >   unregister:
> > > -	ida_simple_remove(&thermal_tz_ida, tz->id);
> > >   	device_unregister(&tz->device);
> > > +remove_id:
> > > +	ida_simple_remove(&thermal_tz_ida, tz->id);
> > >   	kfree(tz);
> > >   	return ERR_PTR(result);
> > >   }
>