memstick: core: fix device_register() error handling

If device_register() fails then call put_device(). See comment to device_register.

Signed-off-by Vasiliy Kulikov <[email protected]>

---
compile tested.

 drivers/memstick/core/memstick.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/memstick/core/memstick.c +0 -0

index c00fe82..4303b7e 100644

Quoted 15 lines · On Sun, 19 Sep 2010 16:54:49 +0400 Vasiliy Kulikov <[email protected]> wrote:

If device_register() fails then call put_device(). See comment to device_register.

Signed-off-by Vasiliy Kulikov <[email protected]>

---
compile tested.

 drivers/memstick/core/memstick.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/memstick/core/memstick.c +0 -0

index c00fe82..4303b7e 100644

drivers/memstick/core/memstick.c static void memstick_check(struct work_struct *work) · line 471

Quoted 7 lines

drivers/memstick/core/memstick.c +1 -0

@@ -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()!

 int device_register(struct device *dev)
 {
+       int retval;
+
       device_initialize(dev);
-       return device_add(dev);
+       retval = device_add(dev);
+       if (retval)
+               put_device(dev);
+       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...

Quoted 12 lines · Greg KH

int device_register(struct device *dev)
{

drivers/base/core.c +6 -1

+       int retval;
+
       device_initialize(dev);
-       return device_add(dev);
+       retval = device_add(dev);
+       if (retval)
+               put_device(dev);
+       return retval;
 }

Quoted 4 lines

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

That looks right, besides that there might be callers already doing this. Which needs to be checked.

I never liked this pretty useless "convenience API", which just wraps two simple functions and the first one can never fail anyway.

We better remove that device_register() stuff entirely in the long run, it's not doing any good. At the kobject level we killed the same stuff already long ago.

Kay

Quoted 25 lines · Kay Sievers

Quoted 12 lines · Greg KH

int device_register(struct device *dev)
{

drivers/base/core.c +6 -1

+       int retval;
+
       device_initialize(dev);
-       return device_add(dev);
+       retval = device_add(dev);
+       if (retval)
+               put_device(dev);
+       return retval;
 }

Quoted 4 lines

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

That looks right, besides that there might be callers already doing this. Which needs to be checked.

I never liked this pretty useless "convenience API", which just wraps two simple functions and the first one can never fail anyway.

Quoted 4 lines

We better remove that device_register() stuff entirely in the long run, it's not doing any good. At the kobject level we killed the same stuff already long ago.

That would be fine, and ping me when you do it, I'll help with my driver. But don't forget to let us have a way to embed a device inside a bigger structure.

For meanwhile Please check the patch James sent to add_device that cleans up the allocation of the kobj.name member. (And the comment made there)

Quoted 26 lines · Boaz Harrosh

Quoted 25 lines · Kay Sievers

Quoted 12 lines · Greg KH

int device_register(struct device *dev)
{

drivers/base/core.c +6 -1

+       int retval;
+
       device_initialize(dev);
-       return device_add(dev);
+       retval = device_add(dev);
+       if (retval)
+               put_device(dev);
+       return retval;
 }

Quoted 4 lines

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

That looks right, besides that there might be callers already doing this. Which needs to be checked.

I never liked this pretty useless "convenience API", which just wraps two simple functions and the first one can never fail anyway.

Quoted 8 lines

Quoted 4 lines

We better remove that device_register() stuff entirely in the long run, it's not doing any good. At the kobject level we killed the same stuff already long ago.

That would be fine, and ping me when you do it, I'll help with my driver. But don't forget to let us have a way to embed a device inside a bigger structure.

That's what you should be doing anyway, so it shouldn't be a problem.

Quoted 2 lines

For meanwhile Please check the patch James sent to add_device that cleans up the allocation of the kobj.name member. (And the comment made there)

See my other email why that isn't a good idea.

Quoted 38 lines · Greg KH

Quoted 26 lines · Boaz Harrosh

Quoted 25 lines · Kay Sievers

Quoted 12 lines · Greg KH

int device_register(struct device *dev)
{

drivers/base/core.c +6 -1

+       int retval;
+
       device_initialize(dev);
-       return device_add(dev);
+       retval = device_add(dev);
+       if (retval)
+               put_device(dev);
+       return retval;
 }

Quoted 4 lines

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

That looks right, besides that there might be callers already doing this. Which needs to be checked.

I never liked this pretty useless "convenience API", which just wraps two simple functions and the first one can never fail anyway.

Quoted 8 lines

Quoted 4 lines

We better remove that device_register() stuff entirely in the long run, it's not doing any good. At the kobject level we killed the same stuff already long ago.

That would be fine, and ping me when you do it, I'll help with my driver. But don't forget to let us have a way to embed a device inside a bigger structure.

That's what you should be doing anyway, so it shouldn't be a problem.

Quoted 4 lines

Quoted 2 lines

For meanwhile Please check the patch James sent to add_device that cleans up the allocation of the kobj.name member. (And the comment made there)

See my other email why that isn't a good idea.

What other email? ... neither you nor Kay replied after either Boaz's patch or the corrected one were posted.

Quoted 2 lines

What other email? ... neither you nor Kay replied after either Boaz's patch or the corrected one were posted.

See my other email a minute ago in response to this thread.

Quoted 11 lines · Greg KH

 int device_register(struct device *dev)
 {
+       int retval;
+
       device_initialize(dev);
-       return device_add(dev);
+       retval = device_add(dev);
+       if (retval)
+               put_device(dev);
+       return retval;
 }

Quoted 4 lines

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

That looks right, besides that there might be callers already doing this. Which needs to be checked.

I never liked this pretty useless "convenience API", which just wraps two simple functions and the first one can never fail anyway.

We better remove that device_register() stuff entirely in the long run, it's not doing any good. At the kobject level we killed the same stuff already long ago.

Quoted 19 lines · Kay Sievers

Quoted 16 lines · Greg KH

 int device_register(struct device *dev)
 {
+       int retval;
+
       device_initialize(dev);
-       return device_add(dev);
+       retval = device_add(dev);
+       if (retval)
+               put_device(dev);
+       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...

Quoted 2 lines

That looks right, besides that there might be callers already doing this. Which needs to be checked.

Yes, it would be. I'll go through the tree.

Quoted 29 lines · Andrew Morton

Quoted 15 lines · On Sun, 19 Sep 2010 16:54:49 +0400 Vasiliy Kulikov <[email protected]> wrote:

If device_register() fails then call put_device(). See comment to device_register.

Signed-off-by Vasiliy Kulikov <[email protected]>

---
compile tested.

 drivers/memstick/core/memstick.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/memstick/core/memstick.c +0 -0

index c00fe82..4303b7e 100644

drivers/memstick/core/memstick.c static void memstick_check(struct work_struct *work) · line 471

Quoted 7 lines

drivers/memstick/core/memstick.c +1 -0

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

Quoted 1 line

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?

drivers/base/core.c +0 -3

index d1b2c9a..4ba8599 100644
@@ -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.

drivers/base/core.c +1 -0

+	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...

thanks,

greg k-h

Quoted 68 lines · Greg KH

Quoted 29 lines · Andrew Morton

Quoted 15 lines · On Sun, 19 Sep 2010 16:54:49 +0400 Vasiliy Kulikov <[email protected]> wrote:

If device_register() fails then call put_device(). See comment to device_register.

Signed-off-by Vasiliy Kulikov <[email protected]>

---
compile tested.

 drivers/memstick/core/memstick.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/memstick/core/memstick.c +0 -0

index c00fe82..4303b7e 100644

drivers/memstick/core/memstick.c static void memstick_check(struct work_struct *work) · line 471

Quoted 7 lines

drivers/memstick/core/memstick.c +1 -0

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

Quoted 1 line

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?

drivers/base/core.c +2 -3

index d1b2c9a..4ba8599 100644
@@ -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;
+

drivers/base/core.c name_error: · line 1095

Quoted 5 lines

drivers/base/core.c +3 -1

 	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?

Quoted 7 lines

drivers/base/core.c +1 -0

+	return retval;
 }
 
 /**

Quoted 5 lines

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.