Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Boaz Harrosh]

On 09/22/2010 12:49 AM, Greg KH wrote:
> On Tue, Sep 21, 2010 at 03:20:31PM -0700, Andrew Morton wrote:
>> On Sun, 19 Sep 2010 16:54:49 +0400
>> Vasiliy Kulikov <segooon@xxxxxxxxx> wrote:
>>
>>> If device_register() fails then call put_device().
>>> See comment to device_register.
>>>
>>> Signed-off-by: Vasiliy Kulikov <segooon@xxxxxxxxx>
>>> ---
>>>  compile tested.
>>>
>>>  drivers/memstick/core/memstick.c |    1 +
>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>>> index c00fe82..4303b7e 100644
>>> --- a/drivers/memstick/core/memstick.c
>>> +++ b/drivers/memstick/core/memstick.c
>>> @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work)
>>>  		if (!host->card) {
>>>  			host->card = card;
>>>  			if (device_register(&card->dev)) {
>>> +				put_device(&card->dev);
>>>  				kfree(host->card);
>>>  				host->card = NULL;
>>>  			}
>>
>> A failed device_register() takes a bogus ref on the not-registered
>> device?  It's no surprise that people are getting this wrong.  
>>
>> The principle of least surprise says: fix device_register()!
> 
> One might think that, but it's a bit more difficult.
> 
> How does device_register know it should destroy the device if it fails?
> 
> Here's how it works:
>  - device_register is just a wrapper around device_initialize() and
>    device_add()
>      - device_initialize() can't do anything wrong, so it's safe, BUT,
>        at this point in time, the reference for the device is
>        incremented, so any caller must now drop the reference and
>        properly free stuff.
>      - device_add() does a lot.
> 
> Hm, I guess, because we "know" in device_register() that we must drop
> something if device_add() fails, then I guess it's not being consistant
> with it's own calls...
> 
> So, something as simple as this?
> 
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d1b2c9a..4ba8599 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1084,14 +1084,16 @@ name_error:
>   * have a clearly defined need to use and refcount the device
>   * before it is added to the hierarchy.
>   *
> - * NOTE: _Never_ directly free @dev after calling this function, even
> - * if it returned an error! Always use put_device() to give up the
> - * reference initialized in this function instead.
>   */
>  int device_register(struct device *dev)
>  {
> +	int retval;
> +
>  	device_initialize(dev);
> -	return device_add(dev);
> +	retval = device_add(dev);
> +	if (retval)
> +		put_device(dev);

This is all theoretically sound. But it will crash my device driver
and a dozen more like mine.

Because the passed *dev is an embedded structure inside a bigger
driver-specific structure. (Hence the call to device_register).

The kref inside the *dev is used to also govern the lifetime of
the outer struct. With a registered release and the use of a
container_of.

Now up until now, and was fine before the set_name changes.
If device_register failed the build-up function would just
deallocate the outer struct together with the inner device.
If you call put_device(dev) here then the release function
will be called. Which is a double free. And also the release
function is coded to expect a fully setup device, though
calling device_unregister, and all the other resources contained
in the outer struct.

So in short it would work but not with current code, and not
with out a major restructuring.

What about the patch James sent to just free the name string
and be done with it?

> +	return retval;
>  }
>  
>  /**
>      	
> 
> 
> Kay, what am I missing here, why can't we just do this?  Hm, the
> side-affect might be that if device_register() fails, NO ONE had better
> touch that device again, as it might have just been freed from the
> system.  I wonder if that will cause problems...
> 

Exactly. Lets please let us recap the scope here. device_register
is specifically for Embedded devices where the life time rules are
"delicate" to say the least. All we have is the name string memory
leak. Can we fix that and not force a major Kernel re-write.

> thanks,
> 
> greg k-h

Thanks
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/