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

Boaz Harrosh
2010-09-20T16:31:48Z

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