Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Boaz Harrosh <bharrosh@panasas.com>]

On 09/20/2010 05:55 PM, James Bottomley wrote:
> On Mon, 2010-09-20 at 17:42 +0200, Boaz Harrosh wrote:
>> I think I have a compromise. If it is indeed the dev_set_name() leak
>> then we can just deallocate the name on the error return path. Therefore
>> any drivers that have the device embedded and rely on it been freed without
>> calling _put will be fine as before. And these calling _put will be fine
>> as well.
>>
>> See below
>> Boaz
>> ---
>> git diff --stat -p -M drivers/base/core.c
>>  drivers/base/core.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index d1b2c9a..054fac2 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -1068,6 +1068,7 @@ done:
>>  	if (parent)
>>  		put_device(parent);
>>  name_error:
>> +	kfree(dev->kobj.name);
>>  	kfree(dev->p);
>>  	dev->p = NULL;
>>  	goto done;
> 
> That's not really good enough ... it will result in a double free
> because the final put (if there is one) will free the name again.
> 
> This would work, but it feels a bit wrong to be poking at the kobject
> from within the driver core ... perhaps a kobject_free_name() call might
> be better?
> 

It would not be the first place to directly access kobj. members but sure
that will work as well.

> James
> 
> ---
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d1b2c9a..88ffaca 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1067,6 +1067,8 @@ done:
>  	cleanup_device_parent(dev);
>  	if (parent)
>  		put_device(parent);
> +	kfree(dev->kobj.name);
> +	dev->kobj.name = NULL;

Rrr thanks, the NULL. Therefore the lack of a signed-off ;-)

I take it that you agree with me that for some code it would be nice
if we add support for not mandating a device_put after a device_register
for the embedded types? For sake of already written code?

>  name_error:
>  	kfree(dev->p);
>  	dev->p = NULL;
> 
> 

Thanks
Boaz