[PATCH 10/14] scsi: osd: fix device_register() error handling

[PATCH 10/14] scsi: osd: fix device_register() error handling

[Vasiliy Kulikov]

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

Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
 compile tested.

 drivers/scsi/osd/osd_uld.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
index cefb2c0..3e0edc2 100644
--- a/drivers/scsi/osd/osd_uld.c
+++ b/drivers/scsi/osd/osd_uld.c
@@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
 	error = device_register(&oud->class_dev);
 	if (error) {
 		OSD_ERR("device_register failed => %d\n", error);
-		goto err_put_cdev;
+		goto err_put_device;
 	}
 
 	get_device(&oud->class_dev);
@@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
 	OSD_INFO("osd_probe %s\n", disk->disk_name);
 	return 0;
 
+err_put_device:
+	put_device(&oud->class_dev);
 err_put_cdev:
 	cdev_del(&oud->cdev);
 err_put_disk:
-- 
1.7.0.4

[PATCH 10/14] scsi: osd: fix device_register() error handling

[Vasiliy Kulikov]

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

Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
 compile tested.

 drivers/scsi/osd/osd_uld.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
index cefb2c0..3e0edc2 100644
--- a/drivers/scsi/osd/osd_uld.c
+++ b/drivers/scsi/osd/osd_uld.c
@@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
 	error = device_register(&oud->class_dev);
 	if (error) {
 		OSD_ERR("device_register failed => %d\n", error);
-		goto err_put_cdev;
+		goto err_put_device;
 	}
 
 	get_device(&oud->class_dev);
@@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
 	OSD_INFO("osd_probe %s\n", disk->disk_name);
 	return 0;
 
+err_put_device:
+	put_device(&oud->class_dev);
 err_put_cdev:
 	cdev_del(&oud->cdev);
 err_put_disk:
-- 
1.7.0.4

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Dan Carpenter]

On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> If device_register() fails then call put_device().
> See comment to device_register.
> 
> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> ---
>  compile tested.
> 
>  drivers/scsi/osd/osd_uld.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> index cefb2c0..3e0edc2 100644
> --- a/drivers/scsi/osd/osd_uld.c
> +++ b/drivers/scsi/osd/osd_uld.c
> @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
>  	error = device_register(&oud->class_dev);
>  	if (error) {
>  		OSD_ERR("device_register failed => %d\n", error);
> -		goto err_put_cdev;
> +		goto err_put_device;
>  	}
>  
>  	get_device(&oud->class_dev);
> @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
>  	OSD_INFO("osd_probe %s\n", disk->disk_name);
>  	return 0;
>  

Hm...  So if device_register() fails then we should always call
device_put()?  It seems like a lot of existing code does that but I
hadn't realized until now that that is how it works.

Why can't the device_put() just be added inside the device_register() so
the unwinding works automatically?

Also if someone add some more stuff to the end of this function, will
the device_unregister() followed by a device_put() cause problems if we
unwind like this?

+err_free_something:
+	kfree(foo);
+	device_unregister(&oud->class_dev);
> +err_put_device:
> +	put_device(&oud->class_dev);
>  err_put_cdev:
>  	cdev_del(&oud->cdev);
>  err_put_disk:

If that's the case then the put_device() should be called infront of the
goto.

regards,
dan carpenter

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Dan Carpenter]

On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> If device_register() fails then call put_device().
> See comment to device_register.
> 
> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> ---
>  compile tested.
> 
>  drivers/scsi/osd/osd_uld.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> index cefb2c0..3e0edc2 100644
> --- a/drivers/scsi/osd/osd_uld.c
> +++ b/drivers/scsi/osd/osd_uld.c
> @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
>  	error = device_register(&oud->class_dev);
>  	if (error) {
>  		OSD_ERR("device_register failed => %d\n", error);
> -		goto err_put_cdev;
> +		goto err_put_device;
>  	}
>  
>  	get_device(&oud->class_dev);
> @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
>  	OSD_INFO("osd_probe %s\n", disk->disk_name);
>  	return 0;
>  

Hm...  So if device_register() fails then we should always call
device_put()?  It seems like a lot of existing code does that but I
hadn't realized until now that that is how it works.

Why can't the device_put() just be added inside the device_register() so
the unwinding works automatically?

Also if someone add some more stuff to the end of this function, will
the device_unregister() followed by a device_put() cause problems if we
unwind like this?

+err_free_something:
+	kfree(foo);
+	device_unregister(&oud->class_dev);
> +err_put_device:
> +	put_device(&oud->class_dev);
>  err_put_cdev:
>  	cdev_del(&oud->cdev);
>  err_put_disk:

If that's the case then the put_device() should be called infront of the
goto.

regards,
dan carpenter

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Vasiliy Kulikov]

On Sun, Sep 19, 2010 at 16:26 +0200, Dan Carpenter wrote:
> On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> > If device_register() fails then call put_device().
> > See comment to device_register.
> > 
> > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > ---
...
> 
> Hm...  So if device_register() fails then we should always call
> device_put()?  It seems like a lot of existing code does that but I
> hadn't realized until now that that is how it works.

Yes, almost ALL code using device_register() is buggy :-(

> Why can't the device_put() just be added inside the device_register() so
> the unwinding works automatically?

Because some code already calls device_put().  Also it is documented like
not putting the device.  However, I'm in doubt why it is written this way.

> Also if someone add some more stuff to the end of this function, will
> the device_unregister() followed by a device_put() cause problems if we
> unwind like this?

Yes, device_register() gets one reference, you should put in in both cases -
when device_register() failed and when it succeeded, but only one time.
device_unregister() puts it, so it is "double putting".

> +err_free_something:
> +	kfree(foo);
> +	device_unregister(&oud->class_dev);
> > +err_put_device:
> > +	put_device(&oud->class_dev);
> >  err_put_cdev:
> >  	cdev_del(&oud->cdev);
> >  err_put_disk:
> 
> If that's the case then the put_device() should be called infront of the
> goto.

As it is the last call that may fail, it is redundant.  Or you mean for future,
if someone adds more code after device_register()?

 
Thanks,
-- 
Vasiliy

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Vasiliy Kulikov]

On Sun, Sep 19, 2010 at 16:26 +0200, Dan Carpenter wrote:
> On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> > If device_register() fails then call put_device().
> > See comment to device_register.
> > 
> > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > ---
...
> 
> Hm...  So if device_register() fails then we should always call
> device_put()?  It seems like a lot of existing code does that but I
> hadn't realized until now that that is how it works.

Yes, almost ALL code using device_register() is buggy :-(

> Why can't the device_put() just be added inside the device_register() so
> the unwinding works automatically?

Because some code already calls device_put().  Also it is documented like
not putting the device.  However, I'm in doubt why it is written this way.

> Also if someone add some more stuff to the end of this function, will
> the device_unregister() followed by a device_put() cause problems if we
> unwind like this?

Yes, device_register() gets one reference, you should put in in both cases -
when device_register() failed and when it succeeded, but only one time.
device_unregister() puts it, so it is "double putting".

> +err_free_something:
> +	kfree(foo);
> +	device_unregister(&oud->class_dev);
> > +err_put_device:
> > +	put_device(&oud->class_dev);
> >  err_put_cdev:
> >  	cdev_del(&oud->cdev);
> >  err_put_disk:
> 
> If that's the case then the put_device() should be called infront of the
> goto.

As it is the last call that may fail, it is redundant.  Or you mean for future,
if someone adds more code after device_register()?

 
Thanks,
-- 
Vasiliy

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Vasiliy Kulikov]

On Sun, Sep 19, 2010 at 16:26 +0200, Dan Carpenter wrote:
> On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> > If device_register() fails then call put_device().
> > See comment to device_register.
> > 
> > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > ---
...
> 
> Hm...  So if device_register() fails then we should always call
> device_put()?  It seems like a lot of existing code does that but I
> hadn't realized until now that that is how it works.

Yes, almost ALL code using device_register() is buggy :-(

> Why can't the device_put() just be added inside the device_register() so
> the unwinding works automatically?

Because some code already calls device_put().  Also it is documented like
not putting the device.  However, I'm in doubt why it is written this way.

> Also if someone add some more stuff to the end of this function, will
> the device_unregister() followed by a device_put() cause problems if we
> unwind like this?

Yes, device_register() gets one reference, you should put in in both cases -
when device_register() failed and when it succeeded, but only one time.
device_unregister() puts it, so it is "double putting".

> +err_free_something:
> +	kfree(foo);
> +	device_unregister(&oud->class_dev);
> > +err_put_device:
> > +	put_device(&oud->class_dev);
> >  err_put_cdev:
> >  	cdev_del(&oud->cdev);
> >  err_put_disk:
> 
> If that's the case then the put_device() should be called infront of the
> goto.

As it is the last call that may fail, it is redundant.  Or you mean for future,
if someone adds more code after device_register()?

 
Thanks,
-- 
Vasiliy

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Dan Carpenter]

On Sun, Sep 19, 2010 at 06:39:50PM +0400, Vasiliy Kulikov wrote:
> > 
> > If that's the case then the put_device() should be called infront of the
> > goto.
> 
> As it is the last call that may fail, it is redundant.  Or you mean for future,
> if someone adds more code after device_register()?
> 

Yes.  I mean in the future.

regards,
dan carpenter

>  
> Thanks,
> -- 
> Vasiliy

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Dan Carpenter]

On Sun, Sep 19, 2010 at 06:39:50PM +0400, Vasiliy Kulikov wrote:
> > 
> > If that's the case then the put_device() should be called infront of the
> > goto.
> 
> As it is the last call that may fail, it is redundant.  Or you mean for future,
> if someone adds more code after device_register()?
> 

Yes.  I mean in the future.

regards,
dan carpenter

>  
> Thanks,
> -- 
> Vasiliy

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[James Bottomley]

On Sun, 2010-09-19 at 16:26 +0200, Dan Carpenter wrote:
> On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> > If device_register() fails then call put_device().
> > See comment to device_register.
> > 
> > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > ---
> >  compile tested.
> > 
> >  drivers/scsi/osd/osd_uld.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> > index cefb2c0..3e0edc2 100644
> > --- a/drivers/scsi/osd/osd_uld.c
> > +++ b/drivers/scsi/osd/osd_uld.c
> > @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
> >  	error = device_register(&oud->class_dev);
> >  	if (error) {
> >  		OSD_ERR("device_register failed => %d\n", error);
> > -		goto err_put_cdev;
> > +		goto err_put_device;
> >  	}
> >  
> >  	get_device(&oud->class_dev);
> > @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
> >  	OSD_INFO("osd_probe %s\n", disk->disk_name);
> >  	return 0;
> >  
> 
> Hm...  So if device_register() fails then we should always call
> device_put()?  It seems like a lot of existing code does that but I
> hadn't realized until now that that is how it works.

Heh, it wasn't a bug when most of the code was written.  It became a bug
when dev_set_name() was added because now the storage allocated for the
name has to be freed with a put.  Previous to this, the advice was just
to free the device if device_register() failed.

> Why can't the device_put() just be added inside the device_register() so
> the unwinding works automatically?

Since Greg and Kay didn't actually alter any of the device_register()
failure paths, this does sound to be the better course of action ... of
course, every device_register() introduced after the dev_set_name()
change may call put_device() on the cleanup path ... someone needs to
check.

James

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[James Bottomley]

On Sun, 2010-09-19 at 16:26 +0200, Dan Carpenter wrote:
> On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> > If device_register() fails then call put_device().
> > See comment to device_register.
> > 
> > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > ---
> >  compile tested.
> > 
> >  drivers/scsi/osd/osd_uld.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> > index cefb2c0..3e0edc2 100644
> > --- a/drivers/scsi/osd/osd_uld.c
> > +++ b/drivers/scsi/osd/osd_uld.c
> > @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
> >  	error = device_register(&oud->class_dev);
> >  	if (error) {
> >  		OSD_ERR("device_register failed => %d\n", error);
> > -		goto err_put_cdev;
> > +		goto err_put_device;
> >  	}
> >  
> >  	get_device(&oud->class_dev);
> > @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
> >  	OSD_INFO("osd_probe %s\n", disk->disk_name);
> >  	return 0;
> >  
> 
> Hm...  So if device_register() fails then we should always call
> device_put()?  It seems like a lot of existing code does that but I
> hadn't realized until now that that is how it works.

Heh, it wasn't a bug when most of the code was written.  It became a bug
when dev_set_name() was added because now the storage allocated for the
name has to be freed with a put.  Previous to this, the advice was just
to free the device if device_register() failed.

> Why can't the device_put() just be added inside the device_register() so
> the unwinding works automatically?

Since Greg and Kay didn't actually alter any of the device_register()
failure paths, this does sound to be the better course of action ... of
course, every device_register() introduced after the dev_set_name()
change may call put_device() on the cleanup path ... someone needs to
check.

James

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Greg KH]

On Mon, Sep 20, 2010 at 06:58:17AM -0500, James Bottomley wrote:
> On Sun, 2010-09-19 at 16:26 +0200, Dan Carpenter wrote:
> > On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> > > If device_register() fails then call put_device().
> > > See comment to device_register.
> > > 
> > > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > > ---
> > >  compile tested.
> > > 
> > >  drivers/scsi/osd/osd_uld.c |    4 +++-
> > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> > > index cefb2c0..3e0edc2 100644
> > > --- a/drivers/scsi/osd/osd_uld.c
> > > +++ b/drivers/scsi/osd/osd_uld.c
> > > @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
> > >  	error = device_register(&oud->class_dev);
> > >  	if (error) {
> > >  		OSD_ERR("device_register failed => %d\n", error);
> > > -		goto err_put_cdev;
> > > +		goto err_put_device;
> > >  	}
> > >  
> > >  	get_device(&oud->class_dev);
> > > @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
> > >  	OSD_INFO("osd_probe %s\n", disk->disk_name);
> > >  	return 0;
> > >  
> > 
> > Hm...  So if device_register() fails then we should always call
> > device_put()?  It seems like a lot of existing code does that but I
> > hadn't realized until now that that is how it works.
> 
> Heh, it wasn't a bug when most of the code was written.  It became a bug
> when dev_set_name() was added because now the storage allocated for the
> name has to be freed with a put.  Previous to this, the advice was just
> to free the device if device_register() failed.
> 
> > Why can't the device_put() just be added inside the device_register() so
> > the unwinding works automatically?
> 
> Since Greg and Kay didn't actually alter any of the device_register()
> failure paths, this does sound to be the better course of action ... of
> course, every device_register() introduced after the dev_set_name()
> change may call put_device() on the cleanup path ... someone needs to
> check.

Yes, this patch series should not be needed at all.  If there's a
problem with the driver core here, it should be fixed, not forcing the
issue to all of the individual callers.

Vasiliy, can you please do that instead?

thanks,

greg k-h

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Greg KH]

On Mon, Sep 20, 2010 at 06:58:17AM -0500, James Bottomley wrote:
> On Sun, 2010-09-19 at 16:26 +0200, Dan Carpenter wrote:
> > On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> > > If device_register() fails then call put_device().
> > > See comment to device_register.
> > > 
> > > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > > ---
> > >  compile tested.
> > > 
> > >  drivers/scsi/osd/osd_uld.c |    4 +++-
> > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> > > index cefb2c0..3e0edc2 100644
> > > --- a/drivers/scsi/osd/osd_uld.c
> > > +++ b/drivers/scsi/osd/osd_uld.c
> > > @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
> > >  	error = device_register(&oud->class_dev);
> > >  	if (error) {
> > >  		OSD_ERR("device_register failed => %d\n", error);
> > > -		goto err_put_cdev;
> > > +		goto err_put_device;
> > >  	}
> > >  
> > >  	get_device(&oud->class_dev);
> > > @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
> > >  	OSD_INFO("osd_probe %s\n", disk->disk_name);
> > >  	return 0;
> > >  
> > 
> > Hm...  So if device_register() fails then we should always call
> > device_put()?  It seems like a lot of existing code does that but I
> > hadn't realized until now that that is how it works.
> 
> Heh, it wasn't a bug when most of the code was written.  It became a bug
> when dev_set_name() was added because now the storage allocated for the
> name has to be freed with a put.  Previous to this, the advice was just
> to free the device if device_register() failed.
> 
> > Why can't the device_put() just be added inside the device_register() so
> > the unwinding works automatically?
> 
> Since Greg and Kay didn't actually alter any of the device_register()
> failure paths, this does sound to be the better course of action ... of
> course, every device_register() introduced after the dev_set_name()
> change may call put_device() on the cleanup path ... someone needs to
> check.

Yes, this patch series should not be needed at all.  If there's a
problem with the driver core here, it should be fixed, not forcing the
issue to all of the individual callers.

Vasiliy, can you please do that instead?

thanks,

greg k-h

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Greg KH]

On Mon, Sep 20, 2010 at 08:10:29AM -0700, Greg KH wrote:
> On Mon, Sep 20, 2010 at 06:58:17AM -0500, James Bottomley wrote:
> > On Sun, 2010-09-19 at 16:26 +0200, Dan Carpenter wrote:
> > > On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> > > > If device_register() fails then call put_device().
> > > > See comment to device_register.
> > > > 
> > > > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > > > ---
> > > >  compile tested.
> > > > 
> > > >  drivers/scsi/osd/osd_uld.c |    4 +++-
> > > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> > > > index cefb2c0..3e0edc2 100644
> > > > --- a/drivers/scsi/osd/osd_uld.c
> > > > +++ b/drivers/scsi/osd/osd_uld.c
> > > > @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
> > > >  	error = device_register(&oud->class_dev);
> > > >  	if (error) {
> > > >  		OSD_ERR("device_register failed => %d\n", error);
> > > > -		goto err_put_cdev;
> > > > +		goto err_put_device;
> > > >  	}
> > > >  
> > > >  	get_device(&oud->class_dev);
> > > > @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
> > > >  	OSD_INFO("osd_probe %s\n", disk->disk_name);
> > > >  	return 0;
> > > >  
> > > 
> > > Hm...  So if device_register() fails then we should always call
> > > device_put()?  It seems like a lot of existing code does that but I
> > > hadn't realized until now that that is how it works.
> > 
> > Heh, it wasn't a bug when most of the code was written.  It became a bug
> > when dev_set_name() was added because now the storage allocated for the
> > name has to be freed with a put.  Previous to this, the advice was just
> > to free the device if device_register() failed.

That was a long time ago.  When the driver core changed, all callers
were audited from what I recall.

> > > Why can't the device_put() just be added inside the device_register() so
> > > the unwinding works automatically?
> > 
> > Since Greg and Kay didn't actually alter any of the device_register()
> > failure paths, this does sound to be the better course of action ... of
> > course, every device_register() introduced after the dev_set_name()
> > change may call put_device() on the cleanup path ... someone needs to
> > check.
> 
> Yes, this patch series should not be needed at all.  If there's a
> problem with the driver core here, it should be fixed, not forcing the
> issue to all of the individual callers.

Nope, I'm wrong, sorry, this is correct.  We can't just free the device
ourselves in the driver core because other parts that the device might
be embedded in need to be cleaned up before it can go away.

So this patch series is correct, thanks.

greg k-h

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Greg KH]

On Mon, Sep 20, 2010 at 08:10:29AM -0700, Greg KH wrote:
> On Mon, Sep 20, 2010 at 06:58:17AM -0500, James Bottomley wrote:
> > On Sun, 2010-09-19 at 16:26 +0200, Dan Carpenter wrote:
> > > On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> > > > If device_register() fails then call put_device().
> > > > See comment to device_register.
> > > > 
> > > > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > > > ---
> > > >  compile tested.
> > > > 
> > > >  drivers/scsi/osd/osd_uld.c |    4 +++-
> > > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> > > > index cefb2c0..3e0edc2 100644
> > > > --- a/drivers/scsi/osd/osd_uld.c
> > > > +++ b/drivers/scsi/osd/osd_uld.c
> > > > @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
> > > >  	error = device_register(&oud->class_dev);
> > > >  	if (error) {
> > > >  		OSD_ERR("device_register failed => %d\n", error);
> > > > -		goto err_put_cdev;
> > > > +		goto err_put_device;
> > > >  	}
> > > >  
> > > >  	get_device(&oud->class_dev);
> > > > @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
> > > >  	OSD_INFO("osd_probe %s\n", disk->disk_name);
> > > >  	return 0;
> > > >  
> > > 
> > > Hm...  So if device_register() fails then we should always call
> > > device_put()?  It seems like a lot of existing code does that but I
> > > hadn't realized until now that that is how it works.
> > 
> > Heh, it wasn't a bug when most of the code was written.  It became a bug
> > when dev_set_name() was added because now the storage allocated for the
> > name has to be freed with a put.  Previous to this, the advice was just
> > to free the device if device_register() failed.

That was a long time ago.  When the driver core changed, all callers
were audited from what I recall.

> > > Why can't the device_put() just be added inside the device_register() so
> > > the unwinding works automatically?
> > 
> > Since Greg and Kay didn't actually alter any of the device_register()
> > failure paths, this does sound to be the better course of action ... of
> > course, every device_register() introduced after the dev_set_name()
> > change may call put_device() on the cleanup path ... someone needs to
> > check.
> 
> Yes, this patch series should not be needed at all.  If there's a
> problem with the driver core here, it should be fixed, not forcing the
> issue to all of the individual callers.

Nope, I'm wrong, sorry, this is correct.  We can't just free the device
ourselves in the driver core because other parts that the device might
be embedded in need to be cleaned up before it can go away.

So this patch series is correct, thanks.

greg k-h

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[James Bottomley]

On Mon, 2010-09-20 at 08:13 -0700, Greg KH wrote:
> On Mon, Sep 20, 2010 at 08:10:29AM -0700, Greg KH wrote:
> > On Mon, Sep 20, 2010 at 06:58:17AM -0500, James Bottomley wrote:
> > > On Sun, 2010-09-19 at 16:26 +0200, Dan Carpenter wrote:
> > > > On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> > > > > If device_register() fails then call put_device().
> > > > > See comment to device_register.
> > > > > 
> > > > > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > > > > ---
> > > > >  compile tested.
> > > > > 
> > > > >  drivers/scsi/osd/osd_uld.c |    4 +++-
> > > > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> > > > > index cefb2c0..3e0edc2 100644
> > > > > --- a/drivers/scsi/osd/osd_uld.c
> > > > > +++ b/drivers/scsi/osd/osd_uld.c
> > > > > @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
> > > > >  	error = device_register(&oud->class_dev);
> > > > >  	if (error) {
> > > > >  		OSD_ERR("device_register failed => %d\n", error);
> > > > > -		goto err_put_cdev;
> > > > > +		goto err_put_device;
> > > > >  	}
> > > > >  
> > > > >  	get_device(&oud->class_dev);
> > > > > @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
> > > > >  	OSD_INFO("osd_probe %s\n", disk->disk_name);
> > > > >  	return 0;
> > > > >  
> > > > 
> > > > Hm...  So if device_register() fails then we should always call
> > > > device_put()?  It seems like a lot of existing code does that but I
> > > > hadn't realized until now that that is how it works.
> > > 
> > > Heh, it wasn't a bug when most of the code was written.  It became a bug
> > > when dev_set_name() was added because now the storage allocated for the
> > > name has to be freed with a put.  Previous to this, the advice was just
> > > to free the device if device_register() failed.
> 
> That was a long time ago.  When the driver core changed, all callers
> were audited from what I recall.
> 
> > > > Why can't the device_put() just be added inside the device_register() so
> > > > the unwinding works automatically?
> > > 
> > > Since Greg and Kay didn't actually alter any of the device_register()
> > > failure paths, this does sound to be the better course of action ... of
> > > course, every device_register() introduced after the dev_set_name()
> > > change may call put_device() on the cleanup path ... someone needs to
> > > check.
> > 
> > Yes, this patch series should not be needed at all.  If there's a
> > problem with the driver core here, it should be fixed, not forcing the
> > issue to all of the individual callers.
> 
> Nope, I'm wrong, sorry, this is correct.  We can't just free the device
> ourselves in the driver core because other parts that the device might
> be embedded in need to be cleaned up before it can go away.

We're not asking you to free it; that would be wrong.  We're discussing
doing a put on add fail.  This will free the name allocation and would
call the release method if one exists, but most of these devices that
use device_register() seem not to have one (being embedded).  The
ultimate free would be done either directly in the error path or
indirectly via release.

This would make the bug you and Kay introduced with the dev_set_name()
patch series go away silently.  As I said ... this change would require
verification since device_register() calls introduced after that patch
series may do the put.

The question is really which is more effort.  Every device_register() up
until the beginning of 2009 has been made buggy by the dev_set_name()
patch set.  The chances are at least a few uses added after would be
rendered wrong (although most look to use copy and paste from existing
uses).

James

> So this patch series is correct, thanks.
> 
> greg k-h

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[James Bottomley]

On Mon, 2010-09-20 at 08:13 -0700, Greg KH wrote:
> On Mon, Sep 20, 2010 at 08:10:29AM -0700, Greg KH wrote:
> > On Mon, Sep 20, 2010 at 06:58:17AM -0500, James Bottomley wrote:
> > > On Sun, 2010-09-19 at 16:26 +0200, Dan Carpenter wrote:
> > > > On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> > > > > If device_register() fails then call put_device().
> > > > > See comment to device_register.
> > > > > 
> > > > > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > > > > ---
> > > > >  compile tested.
> > > > > 
> > > > >  drivers/scsi/osd/osd_uld.c |    4 +++-
> > > > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> > > > > index cefb2c0..3e0edc2 100644
> > > > > --- a/drivers/scsi/osd/osd_uld.c
> > > > > +++ b/drivers/scsi/osd/osd_uld.c
> > > > > @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
> > > > >  	error = device_register(&oud->class_dev);
> > > > >  	if (error) {
> > > > >  		OSD_ERR("device_register failed => %d\n", error);
> > > > > -		goto err_put_cdev;
> > > > > +		goto err_put_device;
> > > > >  	}
> > > > >  
> > > > >  	get_device(&oud->class_dev);
> > > > > @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
> > > > >  	OSD_INFO("osd_probe %s\n", disk->disk_name);
> > > > >  	return 0;
> > > > >  
> > > > 
> > > > Hm...  So if device_register() fails then we should always call
> > > > device_put()?  It seems like a lot of existing code does that but I
> > > > hadn't realized until now that that is how it works.
> > > 
> > > Heh, it wasn't a bug when most of the code was written.  It became a bug
> > > when dev_set_name() was added because now the storage allocated for the
> > > name has to be freed with a put.  Previous to this, the advice was just
> > > to free the device if device_register() failed.
> 
> That was a long time ago.  When the driver core changed, all callers
> were audited from what I recall.
> 
> > > > Why can't the device_put() just be added inside the device_register() so
> > > > the unwinding works automatically?
> > > 
> > > Since Greg and Kay didn't actually alter any of the device_register()
> > > failure paths, this does sound to be the better course of action ... of
> > > course, every device_register() introduced after the dev_set_name()
> > > change may call put_device() on the cleanup path ... someone needs to
> > > check.
> > 
> > Yes, this patch series should not be needed at all.  If there's a
> > problem with the driver core here, it should be fixed, not forcing the
> > issue to all of the individual callers.
> 
> Nope, I'm wrong, sorry, this is correct.  We can't just free the device
> ourselves in the driver core because other parts that the device might
> be embedded in need to be cleaned up before it can go away.

We're not asking you to free it; that would be wrong.  We're discussing
doing a put on add fail.  This will free the name allocation and would
call the release method if one exists, but most of these devices that
use device_register() seem not to have one (being embedded).  The
ultimate free would be done either directly in the error path or
indirectly via release.

This would make the bug you and Kay introduced with the dev_set_name()
patch series go away silently.  As I said ... this change would require
verification since device_register() calls introduced after that patch
series may do the put.

The question is really which is more effort.  Every device_register() up
until the beginning of 2009 has been made buggy by the dev_set_name()
patch set.  The chances are at least a few uses added after would be
rendered wrong (although most look to use copy and paste from existing
uses).

James

> So this patch series is correct, thanks.
> 
> greg k-h

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Boaz Harrosh]

On 09/20/2010 05:21 PM, James Bottomley wrote:
> On Mon, 2010-09-20 at 08:13 -0700, Greg KH wrote:
>> On Mon, Sep 20, 2010 at 08:10:29AM -0700, Greg KH wrote:
>>> On Mon, Sep 20, 2010 at 06:58:17AM -0500, James Bottomley wrote:
>>>> On Sun, 2010-09-19 at 16:26 +0200, Dan Carpenter wrote:
>>>>> On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
>>>>>> If device_register() fails then call put_device().
>>>>>> See comment to device_register.
>>>>>>
>>>>>> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
>>>>>> ---
>>>>>>  compile tested.
>>>>>>
>>>>>>  drivers/scsi/osd/osd_uld.c |    4 +++-
>>>>>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
>>>>>> index cefb2c0..3e0edc2 100644
>>>>>> --- a/drivers/scsi/osd/osd_uld.c
>>>>>> +++ b/drivers/scsi/osd/osd_uld.c
>>>>>> @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
>>>>>>  	error = device_register(&oud->class_dev);
>>>>>>  	if (error) {
>>>>>>  		OSD_ERR("device_register failed => %d\n", error);
>>>>>> -		goto err_put_cdev;
>>>>>> +		goto err_put_device;
>>>>>>  	}
>>>>>>  
>>>>>>  	get_device(&oud->class_dev);
>>>>>> @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
>>>>>>  	OSD_INFO("osd_probe %s\n", disk->disk_name);
>>>>>>  	return 0;
>>>>>>  
>>>>>
>>>>> Hm...  So if device_register() fails then we should always call
>>>>> device_put()?  It seems like a lot of existing code does that but I
>>>>> hadn't realized until now that that is how it works.
>>>>
>>>> Heh, it wasn't a bug when most of the code was written.  It became a bug
>>>> when dev_set_name() was added because now the storage allocated for the
>>>> name has to be freed with a put.  Previous to this, the advice was just
>>>> to free the device if device_register() failed.
>>
>> That was a long time ago.  When the driver core changed, all callers
>> were audited from what I recall.
>>
>>>>> Why can't the device_put() just be added inside the device_register() so
>>>>> the unwinding works automatically?
>>>>
>>>> Since Greg and Kay didn't actually alter any of the device_register()
>>>> failure paths, this does sound to be the better course of action ... of
>>>> course, every device_register() introduced after the dev_set_name()
>>>> change may call put_device() on the cleanup path ... someone needs to
>>>> check.
>>>
>>> Yes, this patch series should not be needed at all.  If there's a
>>> problem with the driver core here, it should be fixed, not forcing the
>>> issue to all of the individual callers.
>>
>> Nope, I'm wrong, sorry, this is correct.  We can't just free the device
>> ourselves in the driver core because other parts that the device might
>> be embedded in need to be cleaned up before it can go away.
> 
> We're not asking you to free it; that would be wrong.  We're discussing
> doing a put on add fail.  This will free the name allocation and would
> call the release method if one exists, but most of these devices that
> use device_register() seem not to have one (being embedded).  The
> ultimate free would be done either directly in the error path or
> indirectly via release.
> 
> This would make the bug you and Kay introduced with the dev_set_name()
> patch series go away silently.  As I said ... this change would require
> verification since device_register() calls introduced after that patch
> series may do the put.
> 
> The question is really which is more effort.  Every device_register() up
> until the beginning of 2009 has been made buggy by the dev_set_name()
> patch set.  The chances are at least a few uses added after would be
> rendered wrong (although most look to use copy and paste from existing
> uses).
> 
> James
> 

I think I have a compromise. If it is indeed the dev_set_name() leak
then we can just deallocate the name on the error return path. Therefore
any drivers that have the device embedded and rely on it been freed without
calling _put will be fine as before. And these calling _put will be fine
as well.

See below
Boaz
---
git diff --stat -p -M drivers/base/core.c
 drivers/base/core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index d1b2c9a..054fac2 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1068,6 +1068,7 @@ done:
 	if (parent)
 		put_device(parent);
 name_error:
+	kfree(dev->kobj.name);
 	kfree(dev->p);
 	dev->p = NULL;
 	goto done;

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Boaz Harrosh]

On 09/20/2010 05:21 PM, James Bottomley wrote:
> On Mon, 2010-09-20 at 08:13 -0700, Greg KH wrote:
>> On Mon, Sep 20, 2010 at 08:10:29AM -0700, Greg KH wrote:
>>> On Mon, Sep 20, 2010 at 06:58:17AM -0500, James Bottomley wrote:
>>>> On Sun, 2010-09-19 at 16:26 +0200, Dan Carpenter wrote:
>>>>> On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
>>>>>> If device_register() fails then call put_device().
>>>>>> See comment to device_register.
>>>>>>
>>>>>> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
>>>>>> ---
>>>>>>  compile tested.
>>>>>>
>>>>>>  drivers/scsi/osd/osd_uld.c |    4 +++-
>>>>>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
>>>>>> index cefb2c0..3e0edc2 100644
>>>>>> --- a/drivers/scsi/osd/osd_uld.c
>>>>>> +++ b/drivers/scsi/osd/osd_uld.c
>>>>>> @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
>>>>>>  	error = device_register(&oud->class_dev);
>>>>>>  	if (error) {
>>>>>>  		OSD_ERR("device_register failed => %d\n", error);
>>>>>> -		goto err_put_cdev;
>>>>>> +		goto err_put_device;
>>>>>>  	}
>>>>>>  
>>>>>>  	get_device(&oud->class_dev);
>>>>>> @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
>>>>>>  	OSD_INFO("osd_probe %s\n", disk->disk_name);
>>>>>>  	return 0;
>>>>>>  
>>>>>
>>>>> Hm...  So if device_register() fails then we should always call
>>>>> device_put()?  It seems like a lot of existing code does that but I
>>>>> hadn't realized until now that that is how it works.
>>>>
>>>> Heh, it wasn't a bug when most of the code was written.  It became a bug
>>>> when dev_set_name() was added because now the storage allocated for the
>>>> name has to be freed with a put.  Previous to this, the advice was just
>>>> to free the device if device_register() failed.
>>
>> That was a long time ago.  When the driver core changed, all callers
>> were audited from what I recall.
>>
>>>>> Why can't the device_put() just be added inside the device_register() so
>>>>> the unwinding works automatically?
>>>>
>>>> Since Greg and Kay didn't actually alter any of the device_register()
>>>> failure paths, this does sound to be the better course of action ... of
>>>> course, every device_register() introduced after the dev_set_name()
>>>> change may call put_device() on the cleanup path ... someone needs to
>>>> check.
>>>
>>> Yes, this patch series should not be needed at all.  If there's a
>>> problem with the driver core here, it should be fixed, not forcing the
>>> issue to all of the individual callers.
>>
>> Nope, I'm wrong, sorry, this is correct.  We can't just free the device
>> ourselves in the driver core because other parts that the device might
>> be embedded in need to be cleaned up before it can go away.
> 
> We're not asking you to free it; that would be wrong.  We're discussing
> doing a put on add fail.  This will free the name allocation and would
> call the release method if one exists, but most of these devices that
> use device_register() seem not to have one (being embedded).  The
> ultimate free would be done either directly in the error path or
> indirectly via release.
> 
> This would make the bug you and Kay introduced with the dev_set_name()
> patch series go away silently.  As I said ... this change would require
> verification since device_register() calls introduced after that patch
> series may do the put.
> 
> The question is really which is more effort.  Every device_register() up
> until the beginning of 2009 has been made buggy by the dev_set_name()
> patch set.  The chances are at least a few uses added after would be
> rendered wrong (although most look to use copy and paste from existing
> uses).
> 
> James
> 

I think I have a compromise. If it is indeed the dev_set_name() leak
then we can just deallocate the name on the error return path. Therefore
any drivers that have the device embedded and rely on it been freed without
calling _put will be fine as before. And these calling _put will be fine
as well.

See below
Boaz
---
git diff --stat -p -M drivers/base/core.c
 drivers/base/core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index d1b2c9a..054fac2 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1068,6 +1068,7 @@ done:
 	if (parent)
 		put_device(parent);
 name_error:
+	kfree(dev->kobj.name);
 	kfree(dev->p);
 	dev->p = NULL;
 	goto done;

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[James Bottomley]

On Mon, 2010-09-20 at 17:42 +0200, Boaz Harrosh wrote:
> I think I have a compromise. If it is indeed the dev_set_name() leak
> then we can just deallocate the name on the error return path. Therefore
> any drivers that have the device embedded and rely on it been freed without
> calling _put will be fine as before. And these calling _put will be fine
> as well.
> 
> See below
> Boaz
> ---
> git diff --stat -p -M drivers/base/core.c
>  drivers/base/core.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d1b2c9a..054fac2 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1068,6 +1068,7 @@ done:
>  	if (parent)
>  		put_device(parent);
>  name_error:
> +	kfree(dev->kobj.name);
>  	kfree(dev->p);
>  	dev->p = NULL;
>  	goto done;

That's not really good enough ... it will result in a double free
because the final put (if there is one) will free the name again.

This would work, but it feels a bit wrong to be poking at the kobject
from within the driver core ... perhaps a kobject_free_name() call might
be better?

James

---

diff --git a/drivers/base/core.c b/drivers/base/core.c
index d1b2c9a..88ffaca 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1067,6 +1067,8 @@ done:
 	cleanup_device_parent(dev);
 	if (parent)
 		put_device(parent);
+	kfree(dev->kobj.name);
+	dev->kobj.name = NULL;
 name_error:
 	kfree(dev->p);
 	dev->p = NULL;

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[James Bottomley]

On Mon, 2010-09-20 at 17:42 +0200, Boaz Harrosh wrote:
> I think I have a compromise. If it is indeed the dev_set_name() leak
> then we can just deallocate the name on the error return path. Therefore
> any drivers that have the device embedded and rely on it been freed without
> calling _put will be fine as before. And these calling _put will be fine
> as well.
> 
> See below
> Boaz
> ---
> git diff --stat -p -M drivers/base/core.c
>  drivers/base/core.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d1b2c9a..054fac2 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1068,6 +1068,7 @@ done:
>  	if (parent)
>  		put_device(parent);
>  name_error:
> +	kfree(dev->kobj.name);
>  	kfree(dev->p);
>  	dev->p = NULL;
>  	goto done;

That's not really good enough ... it will result in a double free
because the final put (if there is one) will free the name again.

This would work, but it feels a bit wrong to be poking at the kobject
from within the driver core ... perhaps a kobject_free_name() call might
be better?

James

---

diff --git a/drivers/base/core.c b/drivers/base/core.c
index d1b2c9a..88ffaca 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1067,6 +1067,8 @@ done:
 	cleanup_device_parent(dev);
 	if (parent)
 		put_device(parent);
+	kfree(dev->kobj.name);
+	dev->kobj.name = NULL;
 name_error:
 	kfree(dev->p);
 	dev->p = NULL;

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Boaz Harrosh]

On 09/20/2010 05:55 PM, James Bottomley wrote:
> On Mon, 2010-09-20 at 17:42 +0200, Boaz Harrosh wrote:
>> I think I have a compromise. If it is indeed the dev_set_name() leak
>> then we can just deallocate the name on the error return path. Therefore
>> any drivers that have the device embedded and rely on it been freed without
>> calling _put will be fine as before. And these calling _put will be fine
>> as well.
>>
>> See below
>> Boaz
>> ---
>> git diff --stat -p -M drivers/base/core.c
>>  drivers/base/core.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index d1b2c9a..054fac2 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -1068,6 +1068,7 @@ done:
>>  	if (parent)
>>  		put_device(parent);
>>  name_error:
>> +	kfree(dev->kobj.name);
>>  	kfree(dev->p);
>>  	dev->p = NULL;
>>  	goto done;
> 
> That's not really good enough ... it will result in a double free
> because the final put (if there is one) will free the name again.
> 
> This would work, but it feels a bit wrong to be poking at the kobject
> from within the driver core ... perhaps a kobject_free_name() call might
> be better?
> 

It would not be the first place to directly access kobj. members but sure
that will work as well.

> James
> 
> ---
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d1b2c9a..88ffaca 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1067,6 +1067,8 @@ done:
>  	cleanup_device_parent(dev);
>  	if (parent)
>  		put_device(parent);
> +	kfree(dev->kobj.name);
> +	dev->kobj.name = NULL;

Rrr thanks, the NULL. Therefore the lack of a signed-off ;-)

I take it that you agree with me that for some code it would be nice
if we add support for not mandating a device_put after a device_register
for the embedded types? For sake of already written code?

>  name_error:
>  	kfree(dev->p);
>  	dev->p = NULL;
> 
> 

Thanks
Boaz

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Boaz Harrosh]

On 09/20/2010 05:55 PM, James Bottomley wrote:
> On Mon, 2010-09-20 at 17:42 +0200, Boaz Harrosh wrote:
>> I think I have a compromise. If it is indeed the dev_set_name() leak
>> then we can just deallocate the name on the error return path. Therefore
>> any drivers that have the device embedded and rely on it been freed without
>> calling _put will be fine as before. And these calling _put will be fine
>> as well.
>>
>> See below
>> Boaz
>> ---
>> git diff --stat -p -M drivers/base/core.c
>>  drivers/base/core.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index d1b2c9a..054fac2 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -1068,6 +1068,7 @@ done:
>>  	if (parent)
>>  		put_device(parent);
>>  name_error:
>> +	kfree(dev->kobj.name);
>>  	kfree(dev->p);
>>  	dev->p = NULL;
>>  	goto done;
> 
> That's not really good enough ... it will result in a double free
> because the final put (if there is one) will free the name again.
> 
> This would work, but it feels a bit wrong to be poking at the kobject
> from within the driver core ... perhaps a kobject_free_name() call might
> be better?
> 

It would not be the first place to directly access kobj. members but sure
that will work as well.

> James
> 
> ---
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d1b2c9a..88ffaca 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1067,6 +1067,8 @@ done:
>  	cleanup_device_parent(dev);
>  	if (parent)
>  		put_device(parent);
> +	kfree(dev->kobj.name);
> +	dev->kobj.name = NULL;

Rrr thanks, the NULL. Therefore the lack of a signed-off ;-)

I take it that you agree with me that for some code it would be nice
if we add support for not mandating a device_put after a device_register
for the embedded types? For sake of already written code?

>  name_error:
>  	kfree(dev->p);
>  	dev->p = NULL;
> 
> 

Thanks
Boaz

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Boaz Harrosh]

On 09/19/2010 02:55 PM, Vasiliy Kulikov wrote:
> If device_register() fails then call put_device().
> See comment to device_register.
> 
> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> ---
>  compile tested.
> 
>  drivers/scsi/osd/osd_uld.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> index cefb2c0..3e0edc2 100644
> --- a/drivers/scsi/osd/osd_uld.c
> +++ b/drivers/scsi/osd/osd_uld.c
> @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
>  	error = device_register(&oud->class_dev);
>  	if (error) {
>  		OSD_ERR("device_register failed => %d\n", error);
> -		goto err_put_cdev;
> +		goto err_put_device;
>  	}
>  
>  	get_device(&oud->class_dev);
> @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
>  	OSD_INFO("osd_probe %s\n", disk->disk_name);
>  	return 0;
>  
> +err_put_device:
> +	put_device(&oud->class_dev);

I'm not sure we can do this here. We might need to disregard the
comment at device_register. Because this put_ will try to call the
registered __release which will try to free the oud structure which
has the ->class_dev embedded, and now we have a double free.

But I will add a fat comment if all agree.

I'm assuming that if the device_register has failed then we are not
yet on any exposed system lists. (proof of we don't need to call
device_unregister). Since we don't yet let anyone see this device
we can go head and free it regardless of it's initialized ref-count
= 1. The motivation here is to tear down the device without any
possible users. Is that guaranteed? From my code audit it is.

>  err_put_cdev:
>  	cdev_del(&oud->cdev);
>  err_put_disk:

And I think device_register has a very bad API side effect with this put.
If you are going to monitor all places that do not call put_device. Why
not go to all places that do, and remove them and fix device_register.
Do a majority vote. What is done more? put_device called or not called.

Thanks
Boaz

Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Boaz Harrosh]

On 09/19/2010 02:55 PM, Vasiliy Kulikov wrote:
> If device_register() fails then call put_device().
> See comment to device_register.
> 
> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> ---
>  compile tested.
> 
>  drivers/scsi/osd/osd_uld.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> index cefb2c0..3e0edc2 100644
> --- a/drivers/scsi/osd/osd_uld.c
> +++ b/drivers/scsi/osd/osd_uld.c
> @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
>  	error = device_register(&oud->class_dev);
>  	if (error) {
>  		OSD_ERR("device_register failed => %d\n", error);
> -		goto err_put_cdev;
> +		goto err_put_device;
>  	}
>  
>  	get_device(&oud->class_dev);
> @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
>  	OSD_INFO("osd_probe %s\n", disk->disk_name);
>  	return 0;
>  
> +err_put_device:
> +	put_device(&oud->class_dev);

I'm not sure we can do this here. We might need to disregard the
comment at device_register. Because this put_ will try to call the
registered __release which will try to free the oud structure which
has the ->class_dev embedded, and now we have a double free.

But I will add a fat comment if all agree.

I'm assuming that if the device_register has failed then we are not
yet on any exposed system lists. (proof of we don't need to call
device_unregister). Since we don't yet let anyone see this device
we can go head and free it regardless of it's initialized ref-count
== 1. The motivation here is to tear down the device without any
possible users. Is that guaranteed? From my code audit it is.

>  err_put_cdev:
>  	cdev_del(&oud->cdev);
>  err_put_disk:

And I think device_register has a very bad API side effect with this put.
If you are going to monitor all places that do not call put_device. Why
not go to all places that do, and remove them and fix device_register.
Do a majority vote. What is done more? put_device called or not called.

Thanks
Boaz