[PATCH] ipack: add missing put_device() after device_register() failed

[PATCH] ipack: add missing put_device() after device_register() failed

[Samuel Iglesias Gonsalvez]

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@igalia.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);
 
 	return ret;
 }
-- 
1.7.10.4

Re: [PATCH] ipack: add missing put_device() after device_register() failed

[Dmitry Torokhov]

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@igalia.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. 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().

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.

Thanks.

-- 
Dmitry

Re: [PATCH] ipack: add missing put_device() after device_register() failed

[Samuel Iglesias Gonsálvez]

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@igalia.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

[PATCH 1/3] ipack: avoid double free on device->id

[Samuel Iglesias Gonsalvez]

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 drivers/ipack/ipack.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ipack/ipack.c b/drivers/ipack/ipack.c
index 7ec6b20..599d4ff 100644
--- a/drivers/ipack/ipack.c
+++ b/drivers/ipack/ipack.c
@@ -24,6 +24,7 @@ static void ipack_device_release(struct device *dev)
 {
 	struct ipack_device *device = to_ipack_dev(dev);
 	kfree(device->id);
+	device->id = NULL;
 	device->release(device);
 }
 
-- 
1.7.10.4

[PATCH 2/3] ipack: add ipack_get_device() ipack_put_device()

[Samuel Iglesias Gonsalvez]

Prepare everything for later use.

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 drivers/ipack/ipack.c |   12 ++++++++++++
 include/linux/ipack.h |    3 +++
 2 files changed, 15 insertions(+)

diff --git a/drivers/ipack/ipack.c b/drivers/ipack/ipack.c
index 599d4ff..bdac7f6 100644
--- a/drivers/ipack/ipack.c
+++ b/drivers/ipack/ipack.c
@@ -462,6 +462,18 @@ void ipack_device_unregister(struct ipack_device *dev)
 }
 EXPORT_SYMBOL_GPL(ipack_device_unregister);
 
+void ipack_get_device(struct ipack_device *dev)
+{
+	get_device(&dev->dev);
+}
+EXPORT_SYMBOL_GPL(ipack_get_device);
+
+void ipack_put_device(struct ipack_device *dev)
+{
+	put_device(&dev->dev);
+}
+EXPORT_SYMBOL_GPL(ipack_put_device);
+
 static int __init ipack_init(void)
 {
 	ida_init(&ipack_ida);
diff --git a/include/linux/ipack.h b/include/linux/ipack.h
index fea12cb..def91fd 100644
--- a/include/linux/ipack.h
+++ b/include/linux/ipack.h
@@ -221,6 +221,9 @@ void ipack_driver_unregister(struct ipack_driver *edrv);
 int ipack_device_register(struct ipack_device *dev);
 void ipack_device_unregister(struct ipack_device *dev);
 
+void ipack_get_device(struct ipack_device *dev);
+void ipack_put_device(struct ipack_device *dev);
+
 /**
  * DEFINE_IPACK_DEVICE_TABLE - macro used to describe a IndustryPack table
  * @_table: device table name
-- 
1.7.10.4

[PATCH 3/3] ipack: split ipack_device_register() in several functions

[Samuel Iglesias Gonsalvez]

One function is ipack_device_init(). If it fails, the caller should execute
ipack_put_device().

The second function is ipack_device_add that only adds the device. If
it fails, the caller should execute ipack_put_device().

Then the device is removed with refcount = 0, as device_register() kernel
documentation says.

ipack_device_del() is added to remove the device.

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 drivers/ipack/carriers/tpci200.c |   14 +++++++++++++-
 drivers/ipack/ipack.c            |   24 +++++++++++++----------
 include/linux/ipack.h            |   39 ++++++++++++++++++++++++++++----------
 3 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/drivers/ipack/carriers/tpci200.c b/drivers/ipack/carriers/tpci200.c
index 0246b1f..c276fde 100644
--- a/drivers/ipack/carriers/tpci200.c
+++ b/drivers/ipack/carriers/tpci200.c
@@ -480,6 +480,7 @@ static void tpci200_release_device(struct ipack_device *dev)
 
 static int tpci200_create_device(struct tpci200_board *tpci200, int i)
 {
+	int ret;
 	enum ipack_space space;
 	struct ipack_device *dev =
 		kzalloc(sizeof(struct ipack_device), GFP_KERNEL);
@@ -495,7 +496,18 @@ static int tpci200_create_device(struct tpci200_board *tpci200, int i)
 			+ tpci200_space_interval[space] * i;
 		dev->region[space].size = tpci200_space_size[space];
 	}
-	return ipack_device_register(dev);
+
+	ret = ipack_device_init(dev);
+	if (ret < 0) {
+		ipack_put_device(dev);
+		return ret;
+	}
+
+	ret = ipack_device_add(dev);
+	if (ret < 0)
+		ipack_put_device(dev);
+
+	return ret;
 }
 
 static int tpci200_pci_probe(struct pci_dev *pdev,
diff --git a/drivers/ipack/ipack.c b/drivers/ipack/ipack.c
index bdac7f6..a26f371 100644
--- a/drivers/ipack/ipack.c
+++ b/drivers/ipack/ipack.c
@@ -228,7 +228,7 @@ static int ipack_unregister_bus_member(struct device *dev, void *data)
 	struct ipack_bus_device *bus = data;
 
 	if (idev->bus == bus)
-		ipack_device_unregister(idev);
+		ipack_device_del(idev);
 
 	return 1;
 }
@@ -420,7 +420,7 @@ out:
 	return ret;
 }
 
-int ipack_device_register(struct ipack_device *dev)
+int ipack_device_init(struct ipack_device *dev)
 {
 	int ret;
 
@@ -429,6 +429,7 @@ int ipack_device_register(struct ipack_device *dev)
 	dev->dev.parent = dev->bus->parent;
 	dev_set_name(&dev->dev,
 		     "ipack-dev.%u.%u", dev->bus->bus_nr, dev->slot);
+	device_initialize(&dev->dev);
 
 	if (dev->bus->ops->set_clockrate(dev, 8))
 		dev_warn(&dev->dev, "failed to switch to 8 MHz operation for reading of device ID.\n");
@@ -448,19 +449,22 @@ int ipack_device_register(struct ipack_device *dev)
 			dev_err(&dev->dev, "failed to switch to 32 MHz operation.\n");
 	}
 
-	ret = device_register(&dev->dev);
-	if (ret < 0)
-		kfree(dev->id);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ipack_device_init);
 
-	return ret;
+int ipack_device_add(struct ipack_device *dev)
+{
+	return device_add(&dev->dev);
 }
-EXPORT_SYMBOL_GPL(ipack_device_register);
+EXPORT_SYMBOL_GPL(ipack_device_add);
 
-void ipack_device_unregister(struct ipack_device *dev)
+void ipack_device_del(struct ipack_device *dev)
 {
-	device_unregister(&dev->dev);
+	device_del(&dev->dev);
+	ipack_put_device(dev);
 }
-EXPORT_SYMBOL_GPL(ipack_device_unregister);
+EXPORT_SYMBOL_GPL(ipack_device_del);
 
 void ipack_get_device(struct ipack_device *dev)
 {
diff --git a/include/linux/ipack.h b/include/linux/ipack.h
index def91fd..1888e06 100644
--- a/include/linux/ipack.h
+++ b/include/linux/ipack.h
@@ -207,19 +207,38 @@ int ipack_driver_register(struct ipack_driver *edrv, struct module *owner,
 void ipack_driver_unregister(struct ipack_driver *edrv);
 
 /**
- *	ipack_device_register -- register an IPack device with the kernel
- *	@dev: the new device to register.
+ *	ipack_device_init -- initialize an IPack device
+ * @dev: the new device to initialize.
  *
- *	Register a new IPack device ("module" in IndustryPack jargon). The call
- *	is done by the carrier driver.  The carrier should populate the fields
- *	bus and slot as well as the region array of @dev prior to calling this
- *	function.  The rest of the fields will be allocated and populated
- *	during registration.
+ * Initialize a new IPack device ("module" in IndustryPack jargon). The call
+ * is done by the carrier driver.  The carrier should populate the fields
+ * bus and slot as well as the region array of @dev prior to calling this
+ * function.  The rest of the fields will be allocated and populated
+ * during initalization.
  *
- *	Return zero on success or error code on failure.
+ * Return zero on success or error code on failure.
+ *
+ * NOTE: _Never_ directly free @dev after calling this function, even
+ * if it returned an error! Always use ipack_put_device() to give up the
+ * reference initialized in this function instead.
+ */
+int ipack_device_init(struct ipack_device *dev);
+
+/**
+ *	ipack_device_add -- Add an IPack device
+ * @dev: the new device to add.
+ *
+ * Add a new IPack device. The call is done by the carrier driver
+ * after calling ipack_device_init().
+ *
+ * Return zero on success or error code on failure.
+ *
+ * NOTE: _Never_ directly free @dev after calling this function, even
+ * if it returned an error! Always use ipack_put_device() to give up the
+ * reference initialized in this function instead.
  */
-int ipack_device_register(struct ipack_device *dev);
-void ipack_device_unregister(struct ipack_device *dev);
+int ipack_device_add(struct ipack_device *dev);
+void ipack_device_del(struct ipack_device *dev);
 
 void ipack_get_device(struct ipack_device *dev);
 void ipack_put_device(struct ipack_device *dev);
-- 
1.7.10.4

Re: [PATCH 1/3] ipack: avoid double free on device->id

[Greg Kroah-Hartman]

On Fri, Mar 08, 2013 at 09:21:45AM +0100, Samuel Iglesias Gonsalvez wrote:
> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
> ---
>  drivers/ipack/ipack.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/ipack/ipack.c b/drivers/ipack/ipack.c
> index 7ec6b20..599d4ff 100644
> --- a/drivers/ipack/ipack.c
> +++ b/drivers/ipack/ipack.c
> @@ -24,6 +24,7 @@ static void ipack_device_release(struct device *dev)
>  {
>  	struct ipack_device *device = to_ipack_dev(dev);
>  	kfree(device->id);
> +	device->id = NULL;

How does that keep anything from being freed twice?

>  	device->release(device);

device should now be gone after this call, right?  What am I missing?

thanks,

greg k-h

Re: [PATCH 1/3] ipack: avoid double free on device->id

[Samuel Iglesias Gonsálvez]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 03/08/2013 06:47 PM, Greg Kroah-Hartman wrote:
> On Fri, Mar 08, 2013 at 09:21:45AM +0100, Samuel Iglesias
> Gonsalvez wrote:
>> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com> 
>> --- drivers/ipack/ipack.c |    1 + 1 file changed, 1 
>> insertion(+)
>> 
>> diff --git a/drivers/ipack/ipack.c b/drivers/ipack/ipack.c index 
>> 7ec6b20..599d4ff 100644 --- a/drivers/ipack/ipack.c +++ 
>> b/drivers/ipack/ipack.c @@ -24,6 +24,7 @@ static void 
>> ipack_device_release(struct device *dev) { struct ipack_device 
>> *device = to_ipack_dev(dev); kfree(device->id); +	device->id = 
>> NULL;
> 
> How does that keep anything from being freed twice?
> 
>> device->release(device);
> 
> device should now be gone after this call, right?  What am I 
> missing?
> 

Yes, you are right. It's not possible to have it freed twice once it's
in ipack_device_release().

You can skip this patch. If you want, I can resend the others accordingly.

Sam

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with undefined - http://www.enigmail.net/

iQIcBAEBCAAGBQJROim2AAoJEH/0ujLxfcNDY+sQAMfrjjs05FJ9wmQvqEFKg18I
QVddG8BbHKDzdmSTkDlagijTE/Q7eOOFV7zOKz6QWJjO14ktPjr56cFwEmIl0KPj
LcDO4+4bIsOfhO64ZJ7CHUFyMcq3olrZNBhQLJZU1anOcDKIimzomGGPXv5PPFe6
BXnRNeXEnS81r3LXQtlJM7h8th3objKWW2R5No40E9sHIKWEgYR7JmgCqhk0fQNv
RKA9FjyBFpF8RjPoi+xSdPYhFPLpOv99ZbYK1wU4goD4ADw9Npujw/66CjFtgJGx
BH3MquLaCYvCmJK6+obQJzWKl1TonQ153hh6uM1b7h+5RJ4yhUSaSPRLLIWGAxRH
3XlbtjMO6bwEDAEg2hOq9whmoVxK70YvjKOCkqo8crwqVsLccMnv4OzoE4NNrm99
8cdUScK8cTi5fg2HuIEJMVTPxpfLk46Ab3OTO9hoVURyRG84TnJ0SLK0CPzy6J1z
ORgZWh7PlEsgLeqsqjKv7q/5tcLLPEGZcV7TekWPBfNVR5aneemHrEIaSIuVYXxw
a7INgvaAWNhqGhag86KwVH0ICbz2xuJu6wtrlFhyMZWhdPN5aEcHwXusjUBhrdrm
DXoEDBFvJShBE8PxNPWYIBcrphetaSQzl0vNsO/SqacGlppsAqtvvU6hkkAuhGeO
a8dpR7nIR8tuz4NPnFHu
=NypR
-----END PGP SIGNATURE-----

Re: [PATCH 1/3] ipack: avoid double free on device->id

[Greg Kroah-Hartman]

On Fri, Mar 08, 2013 at 07:11:02PM +0100, Samuel Iglesias Gonsálvez wrote:
> On 03/08/2013 06:47 PM, Greg Kroah-Hartman wrote:
> > On Fri, Mar 08, 2013 at 09:21:45AM +0100, Samuel Iglesias
> > Gonsalvez wrote:
> >> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com> 
> >> --- drivers/ipack/ipack.c |    1 + 1 file changed, 1 
> >> insertion(+)
> >> 
> >> diff --git a/drivers/ipack/ipack.c b/drivers/ipack/ipack.c index 
> >> 7ec6b20..599d4ff 100644 --- a/drivers/ipack/ipack.c +++ 
> >> b/drivers/ipack/ipack.c @@ -24,6 +24,7 @@ static void 
> >> ipack_device_release(struct device *dev) { struct ipack_device 
> >> *device = to_ipack_dev(dev); kfree(device->id); +	device->id = 
> >> NULL;
> > 
> > How does that keep anything from being freed twice?
> > 
> >> device->release(device);
> > 
> > device should now be gone after this call, right?  What am I 
> > missing?
> > 
> 
> Yes, you are right. It's not possible to have it freed twice once it's
> in ipack_device_release().
> 
> You can skip this patch. If you want, I can resend the others accordingly.

If the others require this one to be applied, in order for them to apply
properly, yes, it would be great to resend.  If not, I'll just skip the
first one.

thanks,

greg k-h

Re: [PATCH 1/3] ipack: avoid double free on device->id

[Samuel Iglesias Gonsálvez]

[-- Attachment #1: Type: text/plain, Size: 304 bytes --]

On Fri, Mar 08, 2013 at 11:36:24AM -0800, Greg Kroah-Hartman wrote:
[...]
> If the others require this one to be applied, in order for them to apply
> properly, yes, it would be great to resend.  If not, I'll just skip the
> first one.
>

They don't require the first one. You can skip it.

Thanks,

Sam

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] ipack: avoid double free on device->id

[Samuel Iglesias Gonsálvez]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 03/08/2013 06:47 PM, Greg Kroah-Hartman wrote:


Yes, you are right. It's not possible to have it freed twice once it's
in ipack_device_release().

You can skip this patch. If you want, I can resend the others accordingly.

Sam

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with undefined - http://www.enigmail.net/

iQIcBAEBCAAGBQJROim2AAoJEH/0ujLxfcND8ksQAIKZJlIONv1h6FWQEzcOrptA
ReEB0p+uDQfRzjpbqzeZcXa3vVK6sc2nIUTggdQcnfNcse38Jn4JCKGllAQfZE51
71ANDmhUpmALKD1o0A8Cya85IfnaVQBhHcmA2b3/wh5dgXjcvgjHWZDjduEC3+I7
sIikZ+OYAZa69dfXYN+NgVYCTj5Gk+eA9oWdnV17d18bviqVf1/PdHHGMceMke9p
jW/UY4mxPHGkJdtZ8RV0avm2VAwY0/HOtzjI8JXZ7Nve9HIYhEcMyBrpHxOQI6U3
CBPY5kqlOa2MdGTiKQct7Daeeqm1guSPPJDRRI3y0O+qgHOn4a8eTVHQ21WMHJoP
Yq1RterlGzRGGzv8s9k76600gXqbaQreEN96fXxISDtk57N/ffIn0cuZwbaeTLpa
AfvV0PeuBpwJ5SezQryNx2O708tk8FGiCys3yAFcak82Am4xBsgIrEo1UYIYXonC
rGbmuG36yxsktMdklz5+ys7R4pDW8Un0iCOMmO3etrYDepy6n9vfjXmYWc8MK2D6
WaVj01qadSj8iHE1cFZms6LnQ+VETc9256QkkFMWaNApW9+BhnGIRj8DErbgOfKK
k1BfmNUrPyuFIlxPxy6kkPwPGfLUOP9yRhGmzrJ18DWJJtdELan5RQIrMlH21LA2
ajvtF3Pt5eDhDy9gM6A8
=LI+5
-----END PGP SIGNATURE-----