[PATCH 0/4] treewide: add missing put_device calls

[PATCH 0/4] treewide: add missing put_device calls

[Levente Kurusa]

Hi,

This is just the beginning of patchset-set that aims to fix possible
problems caused by not calling put_device() if device_register() fails.

The root cause for the need to call put_device() is that the underlying
kobject still has a reference count of 1. Thus, device.release() will not
be called and the device will just sit there waiting for a put_device().
Adding the put_device() also removes the need for the call to kfree() as most
release functions already call kfree() on the container of the device.

While these have not been experienced, they are potential issues and thus
they need to be fixed. Also, they are a few more files that have the same
kind of issue, those will be fixed if these are accepted.

(Sorry for the noise, I messed up my SMTP server so it didn't reach LKML)

The patchset consists of the following patches:

net: phy: call put_device on device_register() failure
eisa: call put_device if device_register fails
backlight: lcd: call put_device if device_register fails
w1: call put_device if device_register fails


diffstat as follows:

 drivers/eisa/eisa-bus.c       |    4 +++-
 drivers/net/phy/mdio_bus.c    |    1 +
 drivers/video/backlight/lcd.c |    2 +-
 drivers/w1/w1_int.c           |    5 ++---
 4 files changed, 7 insertions(+), 5 deletions(-)

--
Regards,
Levente Kurusa

[PATCH 1/4] net: phy: call put_device on device_register() failure

[Levente Kurusa]

It is required to call put_device() if device_register() fails,
so that we give up the last reference to the device. Calling put_device
allows for mdiobus_release to be executed, kfreeing the bus.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Levente Kurusa <levex@linux.com>
---
 drivers/net/phy/mdio_bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 5617876..b071af7 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -154,6 +154,7 @@ int mdiobus_register(struct mii_bus *bus)
 	err = device_register(&bus->dev);
 	if (err) {
 		pr_err("mii_bus %s failed to register\n", bus->id);
+		put_device(&bus->dev);
 		return -EINVAL;
 	}
 
-- 
1.8.3.1

[PATCH 2/4] eisa: call put_device if device_register fails

[Levente Kurusa]

We need to give up the last reference to edev->dev, so
we need to call put_device().

Cc: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Levente Kurusa <levex@linux.com>
---
 drivers/eisa/eisa-bus.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/eisa/eisa-bus.c b/drivers/eisa/eisa-bus.c
index 272a3ec..8842cde 100644
--- a/drivers/eisa/eisa-bus.c
+++ b/drivers/eisa/eisa-bus.c
@@ -232,8 +232,10 @@ static int __init eisa_init_device(struct eisa_root_device *root,
 static int __init eisa_register_device(struct eisa_device *edev)
 {
 	int rc = device_register(&edev->dev);
-	if (rc)
+	if (rc) {
+		put_device(&edev->dev);
 		return rc;
+	}
 
 	rc = device_create_file(&edev->dev, &dev_attr_signature);
 	if (rc)
-- 
1.8.3.1

[PATCH 3/4] backlight: lcd: call put_device if device_register fails

[Levente Kurusa]

Currently we kfree the container of the device which failed to register.
This is wrong as the last reference is not given up with a put_device
call. Also, now that we have put_device() callen, we no longer need
the kfree as the new_ld->dev.release function will take care of kfreeing
the associated memory.

Signed-off-by: Levente Kurusa <levex@linux.com>
---
 drivers/video/backlight/lcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/backlight/lcd.c b/drivers/video/backlight/lcd.c
index 93cf15e..7de847d 100644
--- a/drivers/video/backlight/lcd.c
+++ b/drivers/video/backlight/lcd.c
@@ -228,7 +228,7 @@ struct lcd_device *lcd_device_register(const char *name, struct device *parent,
 
 	rc = device_register(&new_ld->dev);
 	if (rc) {
-		kfree(new_ld);
+		put_device(&new_ld->dev);
 		return ERR_PTR(rc);
 	}
 
-- 
1.8.3.1

[PATCH 3/4] backlight: lcd: call put_device if device_register fails

[Levente Kurusa]

Currently we kfree the container of the device which failed to register.
This is wrong as the last reference is not given up with a put_device
call. Also, now that we have put_device() callen, we no longer need
the kfree as the new_ld->dev.release function will take care of kfreeing
the associated memory.

Signed-off-by: Levente Kurusa <levex@linux.com>
---
 drivers/video/backlight/lcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/backlight/lcd.c b/drivers/video/backlight/lcd.c
index 93cf15e..7de847d 100644
--- a/drivers/video/backlight/lcd.c
+++ b/drivers/video/backlight/lcd.c
@@ -228,7 +228,7 @@ struct lcd_device *lcd_device_register(const char *name, struct device *parent,
 
 	rc = device_register(&new_ld->dev);
 	if (rc) {
-		kfree(new_ld);
+		put_device(&new_ld->dev);
 		return ERR_PTR(rc);
 	}
 
-- 
1.8.3.1

[PATCH 4/4] w1: call put_device if device_register fails

[Levente Kurusa]

Currently, memsetting and kfreeing the device is bad behaviour.
The device will have a reference count of 1 and hence can cause trouble
because it has kfree'd. Proper way to handle a failed device_register
is to call put_device right after it fails.

Signed-off-by: Levente Kurusa <levex@linux.com>
---
 drivers/w1/w1_int.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index 5a98649..0b9b59e 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -90,9 +90,8 @@ static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
 	err = device_register(&dev->dev);
 	if (err) {
 		printk(KERN_ERR "Failed to register master device. err=%d\n", err);
-		memset(dev, 0, sizeof(struct w1_master));
-		kfree(dev);
-		dev = NULL;
+		put_device(&dev->dev);
+		return NULL;
 	}
 
 	return dev;
-- 
1.8.3.1

Re: [PATCH 0/4] treewide: add missing put_device calls

[Bjorn Helgaas]

[+cc Greg]

On Fri, Dec 13, 2013 at 12:22 PM, Levente Kurusa <levex@linux.com> wrote:
> Hi,
>
> This is just the beginning of patchset-set that aims to fix possible
> problems caused by not calling put_device() if device_register() fails.
>
> The root cause for the need to call put_device() is that the underlying
> kobject still has a reference count of 1. Thus, device.release() will not
> be called and the device will just sit there waiting for a put_device().
> Adding the put_device() also removes the need for the call to kfree() as most
> release functions already call kfree() on the container of the device.
>
> While these have not been experienced, they are potential issues and thus
> they need to be fixed. Also, they are a few more files that have the same
> kind of issue, those will be fixed if these are accepted.

Thanks for doing this.  This is the sort of mistake that just gets
copied everywhere, so fixing the examples in the tree will help
prevent the problem from spreading more.

I don't know if there's really value in having device_register()
return an error but rely on the caller to do the put_device().  Are
there cases where the caller still needs the struct device even if
device_register() fails?  E.g., could we do something like this
instead (I know some callers would also require corresponding changes
to avoid double puts):

--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1139,8 +1139,13 @@ EXPORT_SYMBOL_GPL(device_add);
  */
 int device_register(struct device *dev)
 {
+       int ret;
+
        device_initialize(dev);
-       return device_add(dev);
+       ret = device_add(dev);
+       if (ret)
+               put_device(dev);
+       return ret;
 }
 EXPORT_SYMBOL_GPL(device_register);

Re: [PATCH 4/4] w1: call put_device if device_register fails

[Evgeniy Polyakov]

Hi

13.12.2013, 23:23, "Levente Kurusa" <levex@linux.com>:
> Currently, memsetting and kfreeing the device is bad behaviour.
> The device will have a reference count of 1 and hence can cause trouble
> because it has kfree'd. Proper way to handle a failed device_register
> is to call put_device right after it fails.

Looks good to me, thank you
Greg, please pull it into your treee

> Signed-off-by: Levente Kurusa <levex@linux.com>

Acked-by: Evgeniy Polyakov <zbr@ioremap.net>

Re: [PATCH 0/4] treewide: add missing put_device calls

[Greg Kroah-Hartman]

On Fri, Dec 13, 2013 at 01:42:05PM -0700, Bjorn Helgaas wrote:
> [+cc Greg]
> 
> On Fri, Dec 13, 2013 at 12:22 PM, Levente Kurusa <levex@linux.com> wrote:
> > Hi,
> >
> > This is just the beginning of patchset-set that aims to fix possible
> > problems caused by not calling put_device() if device_register() fails.
> >
> > The root cause for the need to call put_device() is that the underlying
> > kobject still has a reference count of 1. Thus, device.release() will not
> > be called and the device will just sit there waiting for a put_device().
> > Adding the put_device() also removes the need for the call to kfree() as most
> > release functions already call kfree() on the container of the device.
> >
> > While these have not been experienced, they are potential issues and thus
> > they need to be fixed. Also, they are a few more files that have the same
> > kind of issue, those will be fixed if these are accepted.
> 
> Thanks for doing this.  This is the sort of mistake that just gets
> copied everywhere, so fixing the examples in the tree will help
> prevent the problem from spreading more.
> 
> I don't know if there's really value in having device_register()
> return an error but rely on the caller to do the put_device().  Are
> there cases where the caller still needs the struct device even if
> device_register() fails?  E.g., could we do something like this
> instead (I know some callers would also require corresponding changes
> to avoid double puts):

Yeah, that might make more sense, but I was trying to not have the
driver core suddenly free memory if something you pass to it goes wrong.
That's a pretty "odd" thing for an api call to do in the kernel, usually
the caller is always responsible for cleaning up for errors happening.

And there's going to be a ton of changes to get this fixed, as you
really need to do it all in one patch, which makes for a bad "flag-day"
of the api.

thanks,

greg k-h

Re: [PATCH 0/4] treewide: add missing put_device calls

[Levente Kurusa]

On 12/14/2013 06:24 PM, Greg Kroah-Hartman wrote:
> On Fri, Dec 13, 2013 at 01:42:05PM -0700, Bjorn Helgaas wrote:
>> [+cc Greg]
>>
>> On Fri, Dec 13, 2013 at 12:22 PM, Levente Kurusa <levex@linux.com> wrote:
>>> Hi,
>>>
>>> This is just the beginning of patchset-set that aims to fix possible
>>> problems caused by not calling put_device() if device_register() fails.
>>>
>>> The root cause for the need to call put_device() is that the underlying
>>> kobject still has a reference count of 1. Thus, device.release() will not
>>> be called and the device will just sit there waiting for a put_device().
>>> Adding the put_device() also removes the need for the call to kfree() as most
>>> release functions already call kfree() on the container of the device.
>>>
>>> While these have not been experienced, they are potential issues and thus
>>> they need to be fixed. Also, they are a few more files that have the same
>>> kind of issue, those will be fixed if these are accepted.
>>
>> Thanks for doing this.  This is the sort of mistake that just gets
>> copied everywhere, so fixing the examples in the tree will help
>> prevent the problem from spreading more.
>>
>> I don't know if there's really value in having device_register()
>> return an error but rely on the caller to do the put_device().  Are
>> there cases where the caller still needs the struct device even if
>> device_register() fails?  E.g., could we do something like this
>> instead (I know some callers would also require corresponding changes
>> to avoid double puts):

There are cases where it is needed. There are quite a few files which
when device_register() fails, the driver print an error messages. IIRC,
there are also a few where the device is also unregistered from the
specific subsystem's core.

-- 
Regards,
Levente Kurusa

Re: [PATCH 0/4] treewide: add missing put_device calls

[Greg Kroah-Hartman]

On Sun, Dec 15, 2013 at 08:55:27AM +0100, Levente Kurusa wrote:
> On 12/14/2013 06:24 PM, Greg Kroah-Hartman wrote:
> > On Fri, Dec 13, 2013 at 01:42:05PM -0700, Bjorn Helgaas wrote:
> >> [+cc Greg]
> >>
> >> On Fri, Dec 13, 2013 at 12:22 PM, Levente Kurusa <levex@linux.com> wrote:
> >>> Hi,
> >>>
> >>> This is just the beginning of patchset-set that aims to fix possible
> >>> problems caused by not calling put_device() if device_register() fails.
> >>>
> >>> The root cause for the need to call put_device() is that the underlying
> >>> kobject still has a reference count of 1. Thus, device.release() will not
> >>> be called and the device will just sit there waiting for a put_device().
> >>> Adding the put_device() also removes the need for the call to kfree() as most
> >>> release functions already call kfree() on the container of the device.
> >>>
> >>> While these have not been experienced, they are potential issues and thus
> >>> they need to be fixed. Also, they are a few more files that have the same
> >>> kind of issue, those will be fixed if these are accepted.
> >>
> >> Thanks for doing this.  This is the sort of mistake that just gets
> >> copied everywhere, so fixing the examples in the tree will help
> >> prevent the problem from spreading more.
> >>
> >> I don't know if there's really value in having device_register()
> >> return an error but rely on the caller to do the put_device().  Are
> >> there cases where the caller still needs the struct device even if
> >> device_register() fails?  E.g., could we do something like this
> >> instead (I know some callers would also require corresponding changes
> >> to avoid double puts):
> 
> There are cases where it is needed. There are quite a few files which
> when device_register() fails, the driver print an error messages.

That shouldn't be needed, and can be removed.

> IIRC, there are also a few where the device is also unregistered from
> the specific subsystem's core.

Do you have a specific example of this?  This should happen in the
release function of the device already, not in some other code.

thanks,

greg k-h

Re: [PATCH 0/4] treewide: add missing put_device calls

[Levente Kurusa]

On 12/15/2013 06:03 PM, Greg Kroah-Hartman wrote:
> On Sun, Dec 15, 2013 at 08:55:27AM +0100, Levente Kurusa wrote:
>> On 12/14/2013 06:24 PM, Greg Kroah-Hartman wrote:
>>> On Fri, Dec 13, 2013 at 01:42:05PM -0700, Bjorn Helgaas wrote:
>>>> [+cc Greg]
>>>>
>>>> On Fri, Dec 13, 2013 at 12:22 PM, Levente Kurusa <levex@linux.com> wrote:
>>>>> Hi,
>>>>>
>>>>> This is just the beginning of patchset-set that aims to fix possible
>>>>> problems caused by not calling put_device() if device_register() fails.
>>>>>
>>>>> The root cause for the need to call put_device() is that the underlying
>>>>> kobject still has a reference count of 1. Thus, device.release() will not
>>>>> be called and the device will just sit there waiting for a put_device().
>>>>> Adding the put_device() also removes the need for the call to kfree() as most
>>>>> release functions already call kfree() on the container of the device.
>>>>>
>>>>> While these have not been experienced, they are potential issues and thus
>>>>> they need to be fixed. Also, they are a few more files that have the same
>>>>> kind of issue, those will be fixed if these are accepted.
>>>>
>>>> Thanks for doing this.  This is the sort of mistake that just gets
>>>> copied everywhere, so fixing the examples in the tree will help
>>>> prevent the problem from spreading more.
>>>>
>>>> I don't know if there's really value in having device_register()
>>>> return an error but rely on the caller to do the put_device().  Are
>>>> there cases where the caller still needs the struct device even if
>>>> device_register() fails?  E.g., could we do something like this
>>>> instead (I know some callers would also require corresponding changes
>>>> to avoid double puts):
>>
>> There are cases where it is needed. There are quite a few files which
>> when device_register() fails, the driver print an error messages.
> 
> That shouldn't be needed, and can be removed.
Yes, we could put a pr_warn() when device_register() fails.


> 
>> IIRC, there are also a few where the device is also unregistered from
>> the specific subsystem's core.
> 
> Do you have a specific example of this?  This should happen in the
> release function of the device already, not in some other code.
> 
Character drivers who register with device_register() call cdev_del() when device_register()
fails. cdev_del() in turn calls kobject_put on the kobject of the device. Of course, this could also
be replaced. Anyways, I have another set of these patches (approx 40) that I will post in a day or so.
With that most (if not all) should be fixed.


-- 
Regards,
Levente Kurusa

Re: [PATCH 0/4] treewide: add missing put_device calls

[Greg Kroah-Hartman]

On Mon, Dec 16, 2013 at 06:18:53PM +0100, Levente Kurusa wrote:
> >> IIRC, there are also a few where the device is also unregistered from
> >> the specific subsystem's core.
> > 
> > Do you have a specific example of this?  This should happen in the
> > release function of the device already, not in some other code.
> > 
> Character drivers who register with device_register() call cdev_del()
> when device_register() fails.

A cdev shouldn't be created until _after_ the device is successfully
registered, as it could be opened and accessed before the device is
registered.  That sounds like the drivers that do that should be fixed
(have an example of this somewhere?)

> cdev_del() in turn calls kobject_put on the kobject of the device.

Which device?  Don't get confused about the internal kobject for a cdev,
that's a totally different thing.

thanks,

greg k-h

Re: [PATCH 0/4] treewide: add missing put_device calls

[Levente Kurusa]

On 12/16/2013 06:58 PM, Greg Kroah-Hartman wrote:
> On Mon, Dec 16, 2013 at 06:18:53PM +0100, Levente Kurusa wrote:
>>>> IIRC, there are also a few where the device is also unregistered from
>>>> the specific subsystem's core.
>>>
>>> Do you have a specific example of this?  This should happen in the
>>> release function of the device already, not in some other code.
>>>
>> Character drivers who register with device_register() call cdev_del()
>> when device_register() fails.
> 
> A cdev shouldn't be created until _after_ the device is successfully
> registered, as it could be opened and accessed before the device is
> registered.  That sounds like the drivers that do that should be fixed
> (have an example of this somewhere?)

I did some research and it seems it has mostly been my false memory that made
me think that. The only example I could find that did something like this
was: drivers/scsi/osd/osd_uld.c at line 403.

When the device_register() fails it immediately jumps to a cdev_del();
> 
>> cdev_del() in turn calls kobject_put on the kobject of the device.
> 
> Which device?  Don't get confused about the internal kobject for a cdev,
> that's a totally different thing.
> 
Ah yes, now I see this. I thought that they shared a kobject, but now thinking
a bit better on this that wouldn't even make sense.


-- 
Regards,
Levente Kurusa

Re: [PATCH 0/4] treewide: add missing put_device calls

[Greg Kroah-Hartman]

On Mon, Dec 16, 2013 at 07:11:21PM +0100, Levente Kurusa wrote:
> On 12/16/2013 06:58 PM, Greg Kroah-Hartman wrote:
> > On Mon, Dec 16, 2013 at 06:18:53PM +0100, Levente Kurusa wrote:
> >>>> IIRC, there are also a few where the device is also unregistered from
> >>>> the specific subsystem's core.
> >>>
> >>> Do you have a specific example of this?  This should happen in the
> >>> release function of the device already, not in some other code.
> >>>
> >> Character drivers who register with device_register() call cdev_del()
> >> when device_register() fails.
> > 
> > A cdev shouldn't be created until _after_ the device is successfully
> > registered, as it could be opened and accessed before the device is
> > registered.  That sounds like the drivers that do that should be fixed
> > (have an example of this somewhere?)
> 
> I did some research and it seems it has mostly been my false memory that made
> me think that. The only example I could find that did something like this
> was: drivers/scsi/osd/osd_uld.c at line 403.
> 
> When the device_register() fails it immediately jumps to a cdev_del();

That should be easy to reorginize to not do this (i.e. do the cdev_add()
call after the device_register() call, if needed.

thanks,

greg k-h

Re: [PATCH 0/4] treewide: add missing put_device calls

[Levente Kurusa]

On 12/16/2013 07:18 PM, Greg Kroah-Hartman wrote:
> On Mon, Dec 16, 2013 at 07:11:21PM +0100, Levente Kurusa wrote:
>> On 12/16/2013 06:58 PM, Greg Kroah-Hartman wrote:
>>> On Mon, Dec 16, 2013 at 06:18:53PM +0100, Levente Kurusa wrote:
>>>>>> IIRC, there are also a few where the device is also unregistered from
>>>>>> the specific subsystem's core.
>>>>>
>>>>> Do you have a specific example of this?  This should happen in the
>>>>> release function of the device already, not in some other code.
>>>>>
>>>> Character drivers who register with device_register() call cdev_del()
>>>> when device_register() fails.
>>>
>>> A cdev shouldn't be created until _after_ the device is successfully
>>> registered, as it could be opened and accessed before the device is
>>> registered.  That sounds like the drivers that do that should be fixed
>>> (have an example of this somewhere?)
>>
>> I did some research and it seems it has mostly been my false memory that made
>> me think that. The only example I could find that did something like this
>> was: drivers/scsi/osd/osd_uld.c at line 403.
>>
>> When the device_register() fails it immediately jumps to a cdev_del();
> 
> That should be easy to reorginize to not do this (i.e. do the cdev_add()
> call after the device_register() call, if needed.
> 

So it seems that this should be fixed if I understand you correctly.
If you want I can add this to my TODO list, so I will make a patch in a few
days?

-- 
Regards,
Levente Kurusa

Re: [PATCH 4/4] w1: call put_device if device_register fails

[Greg KH]

On Sat, Dec 14, 2013 at 07:17:09PM +0400, Evgeniy Polyakov wrote:
> Hi
> 
> 13.12.2013, 23:23, "Levente Kurusa" <levex@linux.com>:
> > Currently, memsetting and kfreeing the device is bad behaviour.
> > The device will have a reference count of 1 and hence can cause trouble
> > because it has kfree'd. Proper way to handle a failed device_register
> > is to call put_device right after it fails.
> 
> Looks good to me, thank you
> Greg, please pull it into your treee
> 
> > Signed-off-by: Levente Kurusa <levex@linux.com>
> 
> Acked-by: Evgeniy Polyakov <zbr@ioremap.net>

I don't seem to have this patch anywhere, can someone please resend it
to me?

thanks,

greg k-h

Re: [PATCH 4/4] w1: call put_device if device_register fails

[Джамурахметов Рустафа]

Hi

19.12.2013, 03:47, "Greg KH" <gregkh@linuxfoundation.org>:

> I don't seem to have this patch anywhere, can someone please resend it
> to me?

Andrew Morton picked it up

http://www.spinics.net/lists/mm-commits/msg101101.html

Re: [PATCH 4/4] w1: call put_device if device_register fails

[Evgeniy Polyakov]

Hi

19.12.2013, 03:47, "Greg KH" <gregkh@linuxfoundation.org>:

> I don't seem to have this patch anywhere, can someone please resend it
> to me?

Andrew Morton picked it up

http://www.spinics.net/lists/mm-commits/msg101101.html

Re: [PATCH 1/4] net: phy: call put_device on device_register() failure

[Greg Kroah-Hartman]

On Fri, Dec 13, 2013 at 07:39:53PM +0100, Levente Kurusa wrote:
> It is required to call put_device() if device_register() fails,
> so that we give up the last reference to the device. Calling put_device
> allows for mdiobus_release to be executed, kfreeing the bus.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Levente Kurusa <levex@linux.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>