linux-kernel - Re: [PATCH 2/3] thermal: core: Reorder 'thermal_zone_device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 08 Aug 2017 21:05:41 +0800
From:   Zhang Rui <rui.zhang@...el.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        edubezval@...il.com
Cc:     linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 2/3] thermal: core: Reorder
 'thermal_zone_device_register()' error handling code

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@...adoo.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);
> > >   }
>