scsi: osd: fix device_register() error handling
Subsystems (3)
PATCH 10/14 scsi: osd: fix device_register() error handling
If device_register() fails then call put_device(). See comment to device_register.
---
compile tested.
drivers/scsi/osd/osd_uld.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
drivers/scsi/osd/osd_uld.c +3 -1
@@ -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:
Signature · 1 line
1.7.0.4
Quoted 24 lines · Vasiliy Kulikov
If
device_register()fails then callput_device(). See comment todevice_register.Signed-off-by Vasiliy Kulikov <[email protected]>---
compile tested.drivers/scsi/osd/osd_uld.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)drivers/scsi/osd/osd_uld.c +1
-1@@ -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);
drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 484
Quoted 3 lines
drivers/scsi/osd/osd_uld.c +0
-0@@ -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);
drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 489
Quoted 5 lines
drivers/scsi/osd/osd_uld.c +2
-0+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
Quoted 6 lines · Dan Carpenter
Quoted 5 lines · Vasiliy Kulikov
If
device_register()fails then callput_device(). See comment todevice_register.Signed-off-by Vasiliy Kulikov <[email protected]>---
...
Quoted 4 lines
Hm... So if
device_register()fails then we should always calldevice_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 :-(
Quoted 2 lines
Why can't the
device_put()just be added inside thedevice_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.
Quoted 3 lines
Also if someone add some more stuff to the end of this function, will the
device_unregister()followed by adevice_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".
Quoted 11 lines
(quoted diff) +3
-0+err_free_something: + kfree(foo); + device_unregister(&oud->class_dev);drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 489
Quoted 5 lines
drivers/scsi/osd/osd_uld.c +2
-0+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,
Signature · 1 line
Vasiliy
Quoted 7 lines · Vasiliy Kulikov
Quoted 3 lines
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
Quoted 4 lines
Thanks,
Signature · 1 line
Vasiliy
Quoted 6 lines · Dan Carpenter
Quoted 5 lines · Vasiliy Kulikov
If
device_register()fails then callput_device(). See comment todevice_register.Signed-off-by Vasiliy Kulikov <[email protected]>---
...
Quoted 4 lines
Hm... So if
device_register()fails then we should always calldevice_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 :-(
Quoted 2 lines
Why can't the
device_put()just be added inside thedevice_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.
Quoted 3 lines
Also if someone add some more stuff to the end of this function, will the
device_unregister()followed by adevice_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".
Quoted 11 lines
(quoted diff) +3
-0+err_free_something: + kfree(foo); + device_unregister(&oud->class_dev);drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 489
Quoted 5 lines
drivers/scsi/osd/osd_uld.c +2
-0+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,
Signature · 1 line
Vasiliy
Quoted 23 lines · Dan Carpenter
Quoted 22 lines · Vasiliy Kulikov
If
device_register()fails then callput_device(). See comment todevice_register.Signed-off-by Vasiliy Kulikov <[email protected]>---
compile tested.drivers/scsi/osd/osd_uld.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)drivers/scsi/osd/osd_uld.c +1
-1@@ -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; }
Quoted 9 lines
drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 484
Quoted 5 lines
drivers/scsi/osd/osd_uld.c +0
-0get_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 calldevice_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.
Quoted 2 lines
Why can't the
device_put()just be added inside thedevice_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
Quoted 39 lines · James Bottomley
Quoted 23 lines · Dan Carpenter
Quoted 22 lines · Vasiliy Kulikov
If
device_register()fails then callput_device(). See comment todevice_register.Signed-off-by Vasiliy Kulikov <[email protected]>---
compile tested.drivers/scsi/osd/osd_uld.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)drivers/scsi/osd/osd_uld.c +1
-1@@ -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; }Quoted 9 lines
drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 484
Quoted 5 lines
drivers/scsi/osd/osd_uld.c +0
-0get_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 calldevice_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 ifdevice_register()failed.
Quoted 8 lines
Quoted 2 lines
Why can't the
device_put()just be added inside thedevice_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, everydevice_register()introduced after thedev_set_name()change may callput_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
Quoted 35 lines · Greg KH
Quoted 34 lines · James Bottomley
Quoted 32 lines · Dan Carpenter
Quoted 24 lines · Vasiliy Kulikov
If
device_register()fails then callput_device(). See comment todevice_register.Signed-off-by Vasiliy Kulikov <[email protected]>---
compile tested.drivers/scsi/osd/osd_uld.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)drivers/scsi/osd/osd_uld.c +1
-1@@ -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);drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 484
Quoted 3 lines
drivers/scsi/osd/osd_uld.c +0
-0@@ -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 calldevice_put()? It seems like a lot of existing code does that but I hadn't realized until now that that is how it works.
Quoted 4 lines
Quoted 4 lines
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 ifdevice_register()failed.
That was a long time ago. When the driver core changed, all callers were audited from what I recall.
Quoted 9 lines
Quoted 8 lines
Quoted 2 lines
Why can't the
device_put()just be added inside thedevice_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, everydevice_register()introduced after thedev_set_name()change may callput_device()on the cleanup path ... someone needs to check.
Quoted 3 lines
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
Quoted 53 lines · Greg KH
Quoted 35 lines · Greg KH
Quoted 34 lines · James Bottomley
Quoted 32 lines · Dan Carpenter
Quoted 24 lines · Vasiliy Kulikov
If
device_register()fails then callput_device(). See comment todevice_register.Signed-off-by Vasiliy Kulikov <[email protected]>---
compile tested.drivers/scsi/osd/osd_uld.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)drivers/scsi/osd/osd_uld.c +1
-1@@ -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);drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 484
Quoted 3 lines
drivers/scsi/osd/osd_uld.c +0
-0@@ -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 calldevice_put()? It seems like a lot of existing code does that but I hadn't realized until now that that is how it works.Quoted 4 lines
Quoted 4 lines
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 ifdevice_register()failed.That was a long time ago. When the driver core changed, all callers were audited from what I recall.
Quoted 9 lines
Quoted 8 lines
Quoted 2 lines
Why can't the
device_put()just be added inside thedevice_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, everydevice_register()introduced after thedev_set_name()change may callput_device()on the cleanup path ... someone needs to check.
Quoted 7 lines
Quoted 3 lines
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
Quoted 3 lines
So this patch series is correct, thanks.
greg k-h
Quoted 74 lines · James Bottomley
Quoted 53 lines · Greg KH
Quoted 35 lines · Greg KH
Quoted 34 lines · James Bottomley
Quoted 32 lines · Dan Carpenter
Quoted 24 lines · Vasiliy Kulikov
If
device_register()fails then callput_device(). See comment todevice_register.Signed-off-by Vasiliy Kulikov <[email protected]>---
compile tested.drivers/scsi/osd/osd_uld.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)drivers/scsi/osd/osd_uld.c +1
-1@@ -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);drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 484
Quoted 3 lines
drivers/scsi/osd/osd_uld.c +0
-0@@ -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 calldevice_put()? It seems like a lot of existing code does that but I hadn't realized until now that that is how it works.Quoted 4 lines
Quoted 4 lines
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 ifdevice_register()failed.That was a long time ago. When the driver core changed, all callers were audited from what I recall.
Quoted 9 lines
Quoted 8 lines
Quoted 2 lines
Why can't the
device_put()just be added inside thedevice_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, everydevice_register()introduced after thedev_set_name()change may callput_device()on the cleanup path ... someone needs to check.Quoted 7 lines
Quoted 3 lines
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 sincedevice_register()calls introduced after that patch series may do the put.
Quoted 8 lines
The question is really which is more effort. Every
device_register()up until the beginning of 2009 has been made buggy by thedev_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(-)
drivers/base/core.c +1 -0
@@ -1068,6 +1068,7 @@ done:if (parent)put_device (parent); name_error: +kfree (dev->kobj.name);kfree (dev->p); dev->p =NULL ; goto done;
Quoted 18 lines · Boaz Harrosh
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_putwill be fine as before. And these calling_putwill be fine as well.See below
Boaz
---
git diff --stat -p -Mdrivers/base/core.cdrivers/base/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/base/core.c +0
-0
drivers/base/core.c done: · line 1074
Quoted 7 lines
drivers/base/core.c +1
-0@@ -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
---
drivers/base/core.c +2 -0
@@ -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 ;
Quoted 30 lines · James Bottomley
Quoted 18 lines · Boaz Harrosh
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_putwill be fine as before. And these calling_putwill be fine as well.See below
Boaz
---
git diff --stat -p -Mdrivers/base/core.cdrivers/base/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/base/core.c +0
-0drivers/base/core.c done: · line 1074
Quoted 7 lines
drivers/base/core.c +1
-0@@ -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.
Quoted 4 lines
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.
Quoted 9 lines
James
---
drivers/base/core.c +0
-0
drivers/base/core.c done: · line 1071
Quoted 5 lines
drivers/base/core.c +2
-0@@ -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?
drivers/base/core.c done: · line 1074
Quoted 5 lines
drivers/base/core.c +0
-0name_error:kfree (dev->p); dev->p =NULL ;
Thanks
Boaz
Quoted 24 lines · Vasiliy Kulikov
If
device_register()fails then callput_device(). See comment todevice_register.Signed-off-by Vasiliy Kulikov <[email protected]>---
compile tested.drivers/scsi/osd/osd_uld.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)drivers/scsi/osd/osd_uld.c +1
-1@@ -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);
drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 486
Quoted 5 lines
drivers/scsi/osd/osd_uld.c +2
-0@@ -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.
drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 489
Quoted 3 lines
drivers/scsi/osd/osd_uld.c +0
-0err_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