linux-kernel - Re: [PATCH] ipack: add missing put_device() after 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <512DCB33.6030500@igalia.com>
Date:	Wed, 27 Feb 2013 10:00:35 +0100
From:	Samuel Iglesias Gonsálvez <siglesias@...lia.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jens Taprogge <jens.taprogge@...rogge.org>,
	industrypack-devel@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ipack: add missing put_device() after device_register()
 failed

Hello Dmitry,

First of all, thanks for your comments.

On 02/26/2013 11:28 PM, Dmitry Torokhov wrote:
> On Tue, Feb 26, 2013 at 10:03:15AM +0100, Samuel Iglesias Gonsalvez wrote:
>> put_device() must be called after device_register() fails,
>> since device_register() always initializes the refcount
>> on the device structure to one.
>>
>> dev->id is free'd inside of ipack_device_release function.
>> So, it's not needed to do it here.
>>
>> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@...lia.com>
>> ---
>>  drivers/ipack/ipack.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/ipack/ipack.c b/drivers/ipack/ipack.c
>> index 7ec6b20..3588ccf 100644
>> --- a/drivers/ipack/ipack.c
>> +++ b/drivers/ipack/ipack.c
>> @@ -449,7 +449,7 @@ int ipack_device_register(struct ipack_device *dev)
>>  
>>  	ret = device_register(&dev->dev);
>>  	if (ret < 0)
>> -		kfree(dev->id);
>> +		put_device(&dev->dev);
> 
> Now callers have no idea if they have to free (put_device) it in case of
> failure or it is already done for them, because a few lines earlier, if
> pack_device_read_id() fails, it simply returns error code.
> 
> IMO if a function did not allocate object it should not try to free it,
> callers should dispose of the device as they see fit.
> 
> What is missing, however, is dev->id = NULL assignment so that if caller
> does put_device() kit won't double-free the memory.

OK. I will write a patch.

> Also it would make
> sense to split device_register into device_init() and device_add() so
> that device_init() is very first thing you do and in case of all
> failures callers should use put_device().
> 

It makes sense. I will write a patch for it.

> Also tpci200.c need to learn to clean up after itself and I also not
> sure why it tries to provide its own release function.
>

tpci200 driver is the one who allocated the struct ipack_device and it
should free it. The path to release it is the following:

* The kernel will call the corresponding release() function of the
struct device. This callback is linked to ipack_release_device() in
ipack.c. In that function, there is a kfree(dev->id) as it was
previously allocated by ipack bus driver.

* Then, ipack_release_device() calls the carrier's release function to
indicate that the device is being released and the carrier driver needs
to take care of its own resources.

In summary, each driver allocate and free its own resources when
unregistering a device.

Thanks,

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