* [PATCH] Driver core: fix race in dev_driver_string [not found] <20091204044337.GE14819@suse.de> @ 2009-12-04 16:06 ` Alan Stern 2009-12-04 16:16 ` Oliver Neukum 0 siblings, 1 reply; 15+ messages in thread From: Alan Stern @ 2009-12-04 16:06 UTC (permalink / raw) To: Greg KH Cc: stable, Oliver Neukum, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list This patch (as1310) works around a race in dev_driver_string(). If the device is unbound while the function is running, dev->driver might become NULL after we test it and before we dereference it. Signed-off-by: Alan Stern <stern@rowland.harvard.edu> CC: stable@kernel.org --- Oliver: We don't have to worry about the device structure being deallocated while the routine is running. If that happens it's a bug in the caller: improper refcounting. Alan Stern Index: usb-2.6/drivers/base/core.c =================================================================== --- usb-2.6.orig/drivers/base/core.c +++ usb-2.6/drivers/base/core.c @@ -56,7 +56,14 @@ static inline int device_is_not_partitio */ const char *dev_driver_string(const struct device *dev) { - return dev->driver ? dev->driver->name : + struct device_driver *drv; + + /* dev->driver can change to NULL underneath us because of unbinding, + * so be careful about accessing it. dev->bus and dev->class should + * never change once they are set, so they don't need special care. + */ + drv = ACCESS_ONCE(dev->driver); + return drv ? drv->name : (dev->bus ? dev->bus->name : (dev->class ? dev->class->name : "")); } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 16:06 ` [PATCH] Driver core: fix race in dev_driver_string Alan Stern @ 2009-12-04 16:16 ` Oliver Neukum 2009-12-04 16:50 ` Greg KH 2009-12-04 16:57 ` Alan Stern 0 siblings, 2 replies; 15+ messages in thread From: Oliver Neukum @ 2009-12-04 16:16 UTC (permalink / raw) To: Alan Stern Cc: Greg KH, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list Am Freitag, 4. Dezember 2009 17:06:57 schrieb Alan Stern: > Oliver: > > We don't have to worry about the device structure being deallocated > while the routine is running. If that happens it's a bug in the > caller: improper refcounting. That raises two points 1. am I supposed to get a reference just so that I can use dev_err? 2. what happens if this is a soft disconnect and the device is reconnected? It seems to me that you'd print the wrong driver's name. Regards Oliver ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 16:16 ` Oliver Neukum @ 2009-12-04 16:50 ` Greg KH 2009-12-04 19:55 ` Oliver Neukum 2009-12-04 16:57 ` Alan Stern 1 sibling, 1 reply; 15+ messages in thread From: Greg KH @ 2009-12-04 16:50 UTC (permalink / raw) To: Oliver Neukum Cc: Alan Stern, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list On Fri, Dec 04, 2009 at 05:16:19PM +0100, Oliver Neukum wrote: > Am Freitag, 4. Dezember 2009 17:06:57 schrieb Alan Stern: > > Oliver: > > > > We don't have to worry about the device structure being deallocated > > while the routine is running. If that happens it's a bug in the > > caller: improper refcounting. > > That raises two points > > 1. am I supposed to get a reference just so that I can use dev_err? No, you should already have a reference on the device when doing the call, right? > 2. what happens if this is a soft disconnect and the device is reconnected? > It seems to me that you'd print the wrong driver's name. Then we can live with that, it's not that big of a deal. thanks, greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 16:50 ` Greg KH @ 2009-12-04 19:55 ` Oliver Neukum 2009-12-04 20:57 ` Alan Stern 0 siblings, 1 reply; 15+ messages in thread From: Oliver Neukum @ 2009-12-04 19:55 UTC (permalink / raw) To: Greg KH Cc: Alan Stern, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list Am Freitag, 4. Dezember 2009 17:50:40 schrieb Greg KH: > On Fri, Dec 04, 2009 at 05:16:19PM +0100, Oliver Neukum wrote: > > Am Freitag, 4. Dezember 2009 17:06:57 schrieb Alan Stern: > > > Oliver: > > > > > > We don't have to worry about the device structure being deallocated > > > while the routine is running. If that happens it's a bug in the > > > caller: improper refcounting. > > > > > > That raises two points > > > > 1. am I supposed to get a reference just so that I can use dev_err? > > No, you should already have a reference on the device when doing the > call, right? No, why? Consider this: int write(...) { ... mutex_lock(&instance->lock); if (instance->disconnected) { dev_dbg(instance->dev,"writing to disconnected device"); rv = -ENODEV; } else { res = usb_submit_urb(...); rv = res < 0 ? -EIO : count; } mutex_unlock(&instance->lock); return rv; } void disconnect(...) { ... mutex_lock(&instance->lock); instance->disconnected = 1; usb_kill_urb(...); usb_kill_urb(...); mutex_unlock(&instance->lock); } This would be perfectly valid code without any references taken save for the pesky dev_dbg() Regards Oliver ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 19:55 ` Oliver Neukum @ 2009-12-04 20:57 ` Alan Stern 2009-12-04 21:18 ` Oliver Neukum 0 siblings, 1 reply; 15+ messages in thread From: Alan Stern @ 2009-12-04 20:57 UTC (permalink / raw) To: Oliver Neukum Cc: Greg KH, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list On Fri, 4 Dec 2009, Oliver Neukum wrote: > > > 1. am I supposed to get a reference just so that I can use dev_err? > > > > No, you should already have a reference on the device when doing the > > call, right? > > No, why? Consider this: > > int write(...) > { > ... > mutex_lock(&instance->lock); > if (instance->disconnected) { > dev_dbg(instance->dev,"writing to disconnected device"); > rv = -ENODEV; > } else { > res = usb_submit_urb(...); > rv = res < 0 ? -EIO : count; > } > mutex_unlock(&instance->lock); > return rv; > } > > void disconnect(...) > { > ... > mutex_lock(&instance->lock); > instance->disconnected = 1; > usb_kill_urb(...); > usb_kill_urb(...); > mutex_unlock(&instance->lock); > } > > This would be perfectly valid code without any references taken save > for the pesky dev_dbg() Whoever calls write() must possess a valid reference. Otherwise instance might already be deallocated when write() starts, causing an oops well before the call to dev_dbg(). Typically the driver would take a reference during open() and drop it during close(). Alan Stern ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 20:57 ` Alan Stern @ 2009-12-04 21:18 ` Oliver Neukum 2009-12-04 21:36 ` Alan Stern 0 siblings, 1 reply; 15+ messages in thread From: Oliver Neukum @ 2009-12-04 21:18 UTC (permalink / raw) To: Alan Stern Cc: Greg KH, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list Am Freitag, 4. Dezember 2009 21:57:50 schrieb Alan Stern: > On Fri, 4 Dec 2009, Oliver Neukum wrote: > > > > 1. am I supposed to get a reference just so that I can use dev_err? > > > > > > No, you should already have a reference on the device when doing the > > > call, right? > > > > No, why? Consider this: > > > > int write(...) > > { > > ... > > mutex_lock(&instance->lock); > > if (instance->disconnected) { > > dev_dbg(instance->dev,"writing to disconnected device"); > > rv = -ENODEV; > > } else { > > res = usb_submit_urb(...); > > rv = res < 0 ? -EIO : count; > > } > > mutex_unlock(&instance->lock); > > return rv; > > } > > > > void disconnect(...) > > { > > ... > > mutex_lock(&instance->lock); > > instance->disconnected = 1; > > usb_kill_urb(...); > > usb_kill_urb(...); > > mutex_unlock(&instance->lock); > > } > > > > This would be perfectly valid code without any references taken save > > for the pesky dev_dbg() > > Whoever calls write() must possess a valid reference. Otherwise > instance might already be deallocated when write() starts, causing an > oops well before the call to dev_dbg(). He needs a valid reference to "instance", not to the device. In fact he may do IO to the device only if he knows it hasn't been disconnected. > Typically the driver would take a reference during open() and drop it > during close(). You can do that but then you must not do IO prior to open() or after close(). That is you must actually wait for IO to finish in close() and cannot prefill your buffers before open(). Regards Oliver ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 21:18 ` Oliver Neukum @ 2009-12-04 21:36 ` Alan Stern 2009-12-04 21:58 ` Oliver Neukum 0 siblings, 1 reply; 15+ messages in thread From: Alan Stern @ 2009-12-04 21:36 UTC (permalink / raw) To: Oliver Neukum Cc: Greg KH, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list On Fri, 4 Dec 2009, Oliver Neukum wrote: > Am Freitag, 4. Dezember 2009 21:57:50 schrieb Alan Stern: > > On Fri, 4 Dec 2009, Oliver Neukum wrote: > > > > > 1. am I supposed to get a reference just so that I can use dev_err? > > > > > > > > No, you should already have a reference on the device when doing the > > > > call, right? > > > > > > No, why? Consider this: > > > > > > int write(...) > > > { > > > ... > > > mutex_lock(&instance->lock); > > > if (instance->disconnected) { > > > dev_dbg(instance->dev,"writing to disconnected device"); > > > rv = -ENODEV; > > > } else { > > > res = usb_submit_urb(...); > > > rv = res < 0 ? -EIO : count; > > > } > > > mutex_unlock(&instance->lock); > > > return rv; > > > } > > > > > > void disconnect(...) > > > { > > > ... > > > mutex_lock(&instance->lock); > > > instance->disconnected = 1; > > > usb_kill_urb(...); > > > usb_kill_urb(...); > > > mutex_unlock(&instance->lock); > > > } > > > > > > This would be perfectly valid code without any references taken save > > > for the pesky dev_dbg() > > > > Whoever calls write() must possess a valid reference. Otherwise > > instance might already be deallocated when write() starts, causing an > > oops well before the call to dev_dbg(). > > He needs a valid reference to "instance", not to the device. In fact > he may do IO to the device only if he knows it hasn't been disconnected. Okay, yes. In fact, that is what your write() routine does -- it checks the disconnected flag. > > Typically the driver would take a reference during open() and drop it > > during close(). > > You can do that but then you must not do IO prior to open() or after > close(). That is you must actually wait for IO to finish in close() and > cannot prefill your buffers before open(). If open() or close() is called before disconnect() then you don't have to worry. If close() is called after disconnect() there's nothing to wait for, because disconnect() should call usb_kill_urb() on all outstanding transfers (actually usbcore will do that for you). Likewise with open(). The problem in this example stems from the fact that you are using instance->dev at a time when you don't know that it is valid -- in fact, you have good reason to believe it _isn't_ valid because instance->disconnected is set. One approach is to set instance->dev to NULL in disconnect(). That wouldn't do much good for your dev_dbg(), though. A better solution is to refcount the instance->dev pointer: Take a reference to the device when setting instance->dev and drop it when clearing instance->dev (or when instance is freed). Alan Stern ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 21:36 ` Alan Stern @ 2009-12-04 21:58 ` Oliver Neukum 2009-12-04 22:07 ` Alan Stern 2009-12-05 0:33 ` Greg KH 0 siblings, 2 replies; 15+ messages in thread From: Oliver Neukum @ 2009-12-04 21:58 UTC (permalink / raw) To: Alan Stern Cc: Greg KH, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list Am Freitag, 4. Dezember 2009 22:36:22 schrieb Alan Stern: > > > Typically the driver would take a reference during open() and drop it > > > during close(). > > > > > > You can do that but then you must not do IO prior to open() or after > > close(). That is you must actually wait for IO to finish in close() and > > cannot prefill your buffers before open(). > > If open() or close() is called before disconnect() then you don't have > to worry. > > If close() is called after disconnect() there's nothing to wait for, > because disconnect() should call usb_kill_urb() on all outstanding > transfers (actually usbcore will do that for you). Likewise with > open(). > > The problem in this example stems from the fact that you are using > instance->dev at a time when you don't know that it is valid -- in > fact, you have good reason to believe it _isn't_ valid because > instance->disconnected is set. OK, yes. It's a bad example. However this is tricky. This is a bug then: mutex_lock(...); if (instance->error) { rv = instance->error; instance->error = 0; dev_dbg(instance->dev,...); goto err_out; } rv = -ENODEV; if (instance->disconnected) goto err_out; > One approach is to set instance->dev to NULL in disconnect(). That > wouldn't do much good for your dev_dbg(), though. A better solution is > to refcount the instance->dev pointer: Take a reference to the device > when setting instance->dev and drop it when clearing instance->dev (or > when instance is freed). That would mean that I am forced to adopt refcounting just to print something. This seems very inelegant. Regards Oliver ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 21:58 ` Oliver Neukum @ 2009-12-04 22:07 ` Alan Stern 2009-12-04 22:23 ` Dmitry Torokhov 2009-12-05 0:33 ` Greg KH 1 sibling, 1 reply; 15+ messages in thread From: Alan Stern @ 2009-12-04 22:07 UTC (permalink / raw) To: Oliver Neukum Cc: Greg KH, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list On Fri, 4 Dec 2009, Oliver Neukum wrote: > OK, yes. It's a bad example. However this is tricky. > > This is a bug then: > > mutex_lock(...); > > if (instance->error) { > rv = instance->error; > instance->error = 0; > dev_dbg(instance->dev,...); Unless you can guarantee at this point that instance->dev isn't stale, it is indeed a bug. > goto err_out; > } > > rv = -ENODEV; > if (instance->disconnected) > goto err_out; > > > One approach is to set instance->dev to NULL in disconnect(). That > > wouldn't do much good for your dev_dbg(), though. A better solution is > > to refcount the instance->dev pointer: Take a reference to the device > > when setting instance->dev and drop it when clearing instance->dev (or > > when instance is freed). > > That would mean that I am forced to adopt refcounting just to print > something. This seems very inelegant. What can I say? When the something you want to print can be deallocated at any time, there isn't much choice. Maybe reference counting is inelegant; it depends on your point of view. Can you think of a more elegant way to make sure that a pointer isn't stale? Alan Stern ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 22:07 ` Alan Stern @ 2009-12-04 22:23 ` Dmitry Torokhov 2009-12-04 23:50 ` Alan Stern 0 siblings, 1 reply; 15+ messages in thread From: Dmitry Torokhov @ 2009-12-04 22:23 UTC (permalink / raw) To: Alan Stern Cc: Oliver Neukum, Greg KH, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list On Friday 04 December 2009 02:07:11 pm Alan Stern wrote: > On Fri, 4 Dec 2009, Oliver Neukum wrote: > > OK, yes. It's a bad example. However this is tricky. > > > > This is a bug then: > > > > mutex_lock(...); > > > > if (instance->error) { > > rv = instance->error; > > instance->error = 0; > > dev_dbg(instance->dev,...); > > Unless you can guarantee at this point that instance->dev isn't stale, > it is indeed a bug. > > > goto err_out; > > } > > > > rv = -ENODEV; > > if (instance->disconnected) > > goto err_out; > > > > > One approach is to set instance->dev to NULL in disconnect(). That > > > wouldn't do much good for your dev_dbg(), though. A better solution is > > > to refcount the instance->dev pointer: Take a reference to the device > > > when setting instance->dev and drop it when clearing instance->dev (or > > > when instance is freed). > > > > That would mean that I am forced to adopt refcounting just to print > > something. This seems very inelegant. > > What can I say? When the something you want to print can be > deallocated at any time, there isn't much choice. > > Maybe reference counting is inelegant; it depends on your point of > view. Can you think of a more elegant way to make sure that a pointer > isn't stale? Yes, just say "no" to device_create() and friends. Embed device structure in yours, be mindful of lifetime rules and only use "your" device (i.e device bound to your driver). This way, as long as your refcount your instance you can rest assured the device structure is there as well. -- Dmitry ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 22:23 ` Dmitry Torokhov @ 2009-12-04 23:50 ` Alan Stern 2009-12-05 0:35 ` Greg KH 0 siblings, 1 reply; 15+ messages in thread From: Alan Stern @ 2009-12-04 23:50 UTC (permalink / raw) To: Dmitry Torokhov Cc: Oliver Neukum, Greg KH, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list On Fri, 4 Dec 2009, Dmitry Torokhov wrote: > > Maybe reference counting is inelegant; it depends on your point of > > view. Can you think of a more elegant way to make sure that a pointer > > isn't stale? > > Yes, just say "no" to device_create() and friends. device_create() wasn't used in the case Oliver is discussing. > Embed device structure in > yours, You can't do that when the device structure wasn't created by your driver. > be mindful of lifetime rules and only use "your" device (i.e device > bound to your driver). What do you mean by "use"? In Oliver's case he wasn't using the device, he was using the device structure. (Maybe that's what you meant.) And he wanted to use it at a time when it wasn't bound to his driver, because userspace still had an open file reference to it. There isn't really any way around this. > This way, as long as your refcount your instance you > can rest assured the device structure is there as well. I rather think that a simple device_get() and device_put() is easier than trying to follow a bunch of rules, especially in cases where they don't apply! :-) Alan Stern ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 23:50 ` Alan Stern @ 2009-12-05 0:35 ` Greg KH 2009-12-05 2:37 ` Alan Stern 0 siblings, 1 reply; 15+ messages in thread From: Greg KH @ 2009-12-05 0:35 UTC (permalink / raw) To: Alan Stern Cc: Dmitry Torokhov, Oliver Neukum, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list On Fri, Dec 04, 2009 at 06:50:35PM -0500, Alan Stern wrote: > On Fri, 4 Dec 2009, Dmitry Torokhov wrote: > > > > Maybe reference counting is inelegant; it depends on your point of > > > view. Can you think of a more elegant way to make sure that a pointer > > > isn't stale? > > > > Yes, just say "no" to device_create() and friends. > > device_create() wasn't used in the case Oliver is discussing. It was implied, as you had a pointer to the device, not the device itself. > > Embed device structure in > > yours, > > You can't do that when the device structure wasn't created by your > driver. But for USB devices, it is part of the device you are handed. Same goes for PCI devices, and most other types of drivers, right? > > be mindful of lifetime rules and only use "your" device (i.e device > > bound to your driver). > > What do you mean by "use"? In Oliver's case he wasn't using the > device, he was using the device structure. (Maybe that's what you > meant.) I think that is what is meant here. > And he wanted to use it at a time when it wasn't bound to his > driver, because userspace still had an open file reference to it. > There isn't really any way around this. But you still have a valid device, just not maybe a driver bound to it. > > This way, as long as your refcount your instance you > > can rest assured the device structure is there as well. > > I rather think that a simple device_get() and device_put() is easier > than trying to follow a bunch of rules, especially in cases where they > don't apply! :-) Like here :) thanks, greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-05 0:35 ` Greg KH @ 2009-12-05 2:37 ` Alan Stern 0 siblings, 0 replies; 15+ messages in thread From: Alan Stern @ 2009-12-05 2:37 UTC (permalink / raw) To: Greg KH Cc: Dmitry Torokhov, Oliver Neukum, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list On Fri, 4 Dec 2009, Greg KH wrote: > On Fri, Dec 04, 2009 at 06:50:35PM -0500, Alan Stern wrote: > > On Fri, 4 Dec 2009, Dmitry Torokhov wrote: > > > > > > Maybe reference counting is inelegant; it depends on your point of > > > > view. Can you think of a more elegant way to make sure that a pointer > > > > isn't stale? > > > > > > Yes, just say "no" to device_create() and friends. > > > > device_create() wasn't used in the case Oliver is discussing. > > It was implied, as you had a pointer to the device, not the device > itself. Not necessarily. For example, the serial drivers have pointers to struct tty, not the tty structures themselves. That doesn't imply the tty structures were constructed using device_create(). > > > Embed device structure in > > > yours, > > > > You can't do that when the device structure wasn't created by your > > driver. > > But for USB devices, it is part of the device you are handed. Same goes > for PCI devices, and most other types of drivers, right? Yes. Dmitry's word "yours" is ambiguous here. It's true that struct pci_device contains an embedded struct device. But for example, struct ehci_hcd doesn't -- even when the EHCI controller is a PCI device. So if you are the ehci-hcd driver, which structure is "yours": the struct pci_device or the struct ehci_hcd? > > > be mindful of lifetime rules and only use "your" device (i.e device > > > bound to your driver). > > > > What do you mean by "use"? In Oliver's case he wasn't using the > > device, he was using the device structure. (Maybe that's what you > > meant.) > > I think that is what is meant here. > > > And he wanted to use it at a time when it wasn't bound to his > > driver, because userspace still had an open file reference to it. > > There isn't really any way around this. > > But you still have a valid device, just not maybe a driver bound to it. If a driver isn't bound to it then you don't know whether the device structure is valid or not. It could have been deallocated. Unless you have taken a reference to it -- then you know. Alan Stern ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 21:58 ` Oliver Neukum 2009-12-04 22:07 ` Alan Stern @ 2009-12-05 0:33 ` Greg KH 1 sibling, 0 replies; 15+ messages in thread From: Greg KH @ 2009-12-05 0:33 UTC (permalink / raw) To: Oliver Neukum Cc: Alan Stern, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list On Fri, Dec 04, 2009 at 10:58:48PM +0100, Oliver Neukum wrote: > Am Freitag, 4. Dezember 2009 22:36:22 schrieb Alan Stern: > > > > Typically the driver would take a reference during open() and drop it > > > > during close(). > > > > > > > > > You can do that but then you must not do IO prior to open() or after > > > close(). That is you must actually wait for IO to finish in close() and > > > cannot prefill your buffers before open(). > > > > If open() or close() is called before disconnect() then you don't have > > to worry. > > > > If close() is called after disconnect() there's nothing to wait for, > > because disconnect() should call usb_kill_urb() on all outstanding > > transfers (actually usbcore will do that for you). Likewise with > > open(). > > > > The problem in this example stems from the fact that you are using > > instance->dev at a time when you don't know that it is valid -- in > > fact, you have good reason to believe it _isn't_ valid because > > instance->disconnected is set. > > OK, yes. It's a bad example. However this is tricky. > > This is a bug then: > > mutex_lock(...); > > if (instance->error) { > rv = instance->error; > instance->error = 0; > dev_dbg(instance->dev,...); > goto err_out; > } > > rv = -ENODEV; > if (instance->disconnected) > goto err_out; > > > One approach is to set instance->dev to NULL in disconnect(). That > > wouldn't do much good for your dev_dbg(), though. A better solution is > > to refcount the instance->dev pointer: Take a reference to the device > > when setting instance->dev and drop it when clearing instance->dev (or > > when instance is freed). > > That would mean that I am forced to adopt refcounting just to print > something. This seems very inelegant. Don't print anything if you are disconnecting :) thanks, greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Driver core: fix race in dev_driver_string 2009-12-04 16:16 ` Oliver Neukum 2009-12-04 16:50 ` Greg KH @ 2009-12-04 16:57 ` Alan Stern 1 sibling, 0 replies; 15+ messages in thread From: Alan Stern @ 2009-12-04 16:57 UTC (permalink / raw) To: Oliver Neukum Cc: Greg KH, stable, Rickard Bellini, linux-usb@vger.kernel.org, Torgny Johansson, Kernel development list On Fri, 4 Dec 2009, Oliver Neukum wrote: > Am Freitag, 4. Dezember 2009 17:06:57 schrieb Alan Stern: > > Oliver: > > > > We don't have to worry about the device structure being deallocated > > while the routine is running. If that happens it's a bug in the > > caller: improper refcounting. > > That raises two points > > 1. am I supposed to get a reference just so that I can use dev_err? You're supposed to hold a reference before using dev at all. If the only use you make of dev is to call dev_err(), then yes. That's how refcounting is meant to work. > 2. what happens if this is a soft disconnect and the device is reconnected? > It seems to me that you'd print the wrong driver's name. You mean the device is unbound from driver A and then bound to driver B, after which a thread running in driver A calls dev_err()? Then yes, the "wrong" name will be printed. This is part of the idea behind dev_err() and friends. They print the name of the currently bound driver, not the name of the calling source file. That's why, for example, the name "usb-storage" shows up in a message printed from within usbcore rather than "hub.c". Alan Stern ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2009-12-05 2:37 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20091204044337.GE14819@suse.de>
2009-12-04 16:06 ` [PATCH] Driver core: fix race in dev_driver_string Alan Stern
2009-12-04 16:16 ` Oliver Neukum
2009-12-04 16:50 ` Greg KH
2009-12-04 19:55 ` Oliver Neukum
2009-12-04 20:57 ` Alan Stern
2009-12-04 21:18 ` Oliver Neukum
2009-12-04 21:36 ` Alan Stern
2009-12-04 21:58 ` Oliver Neukum
2009-12-04 22:07 ` Alan Stern
2009-12-04 22:23 ` Dmitry Torokhov
2009-12-04 23:50 ` Alan Stern
2009-12-05 0:35 ` Greg KH
2009-12-05 2:37 ` Alan Stern
2009-12-05 0:33 ` Greg KH
2009-12-04 16:57 ` Alan Stern
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox