This patch (as1013) was suggested by David Woodhouse; it fixes a race
in the driver core. If a device is unregistered at the same time as
its driver is unloaded, the driver's code pages may be unmapped while
the remove method is still running. The calls to get_driver() and
put_driver() were intended to prevent this, but they don't work if the
driver's module count has already dropped to 0.
Instead, the patch keeps the device on the driver's list until after
the remove method has returned. This forces the necessary
synchronization to occur.
Signed-off-by: Alan Stern <[email protected]>
CC: David Woodhouse <[email protected]>
---
This should be considered for 2.6.24.
Index: usb-2.6/drivers/base/dd.c
===================================================================
--- usb-2.6.orig/drivers/base/dd.c
+++ usb-2.6/drivers/base/dd.c
@@ -289,11 +289,10 @@ static void __device_release_driver(stru
{
struct device_driver * drv;
- drv = get_driver(dev->driver);
+ drv = dev->driver;
if (drv) {
driver_sysfs_remove(dev);
sysfs_remove_link(&dev->kobj, "driver");
- klist_remove(&dev->knode_driver);
if (dev->bus)
blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
@@ -306,7 +305,7 @@ static void __device_release_driver(stru
drv->remove(dev);
devres_release_all(dev);
dev->driver = NULL;
- put_driver(drv);
+ klist_remove(&dev->knode_driver);
}
}
On Fri, 2007-11-16 at 11:57 -0500, Alan Stern wrote:
> This patch (as1013) was suggested by David Woodhouse; it fixes a race
> in the driver core. If a device is unregistered at the same time as
> its driver is unloaded, the driver's code pages may be unmapped while
> the remove method is still running. The calls to get_driver() and
> put_driver() were intended to prevent this, but they don't work if the
> driver's module count has already dropped to 0.
>
> Instead, the patch keeps the device on the driver's list until after
> the remove method has returned. This forces the necessary
> synchronization to occur.
>
> Signed-off-by: Alan Stern <[email protected]>
> CC: David Woodhouse <[email protected]>
Since we're submitting it rather than just using it to explain the
problem, I suppose I should add:
Signed-off-by: David Woodhouse <[email protected]>
> ---
>
> This should be considered for 2.6.24.
>
>
> Index: usb-2.6/drivers/base/dd.c
> ===================================================================
> --- usb-2.6.orig/drivers/base/dd.c
> +++ usb-2.6/drivers/base/dd.c
> @@ -289,11 +289,10 @@ static void __device_release_driver(stru
> {
> struct device_driver * drv;
>
> - drv = get_driver(dev->driver);
> + drv = dev->driver;
> if (drv) {
> driver_sysfs_remove(dev);
> sysfs_remove_link(&dev->kobj, "driver");
> - klist_remove(&dev->knode_driver);
>
> if (dev->bus)
> blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> @@ -306,7 +305,7 @@ static void __device_release_driver(stru
> drv->remove(dev);
> devres_release_all(dev);
> dev->driver = NULL;
> - put_driver(drv);
> + klist_remove(&dev->knode_driver);
> }
> }
>
>
--
dwmw2
On Fri, Nov 16, 2007 at 11:57:28AM -0500, Alan Stern wrote:
> This patch (as1013) was suggested by David Woodhouse; it fixes a race
> in the driver core. If a device is unregistered at the same time as
> its driver is unloaded, the driver's code pages may be unmapped while
> the remove method is still running. The calls to get_driver() and
> put_driver() were intended to prevent this, but they don't work if the
> driver's module count has already dropped to 0.
>
> Instead, the patch keeps the device on the driver's list until after
> the remove method has returned. This forces the necessary
> synchronization to occur.
Is this something that you all feel is worth getting in for 2.6.24?
Does it fix a regression that just showed up, or is just a bugfix for
something that people finally realized has always been there?
Can it wait for 2.6.25 to get some more testing in -mm?
thanks,
greg k-h
On Fri, 2007-11-16 at 12:37 -0800, Greg KH wrote:
> Is this something that you all feel is worth getting in for 2.6.24?
> Does it fix a regression that just showed up, or is just a bugfix for
> something that people finally realized has always been there?
The latter. We just happen to kick the wireless device in the head by
toggling its out-of-band reset signal when we unload the driver, which
causes us to trigger this "unlikely" race with almost 100% reliability.
But it's always been there, AFAICT.
> Can it wait for 2.6.25 to get some more testing in -mm?
Yeah, I think so.
--
dwmw2
On Fri, 16 Nov 2007, Greg KH wrote:
> On Fri, Nov 16, 2007 at 11:57:28AM -0500, Alan Stern wrote:
> > This patch (as1013) was suggested by David Woodhouse; it fixes a race
> > in the driver core. If a device is unregistered at the same time as
> > its driver is unloaded, the driver's code pages may be unmapped while
> > the remove method is still running. The calls to get_driver() and
> > put_driver() were intended to prevent this, but they don't work if the
> > driver's module count has already dropped to 0.
> >
> > Instead, the patch keeps the device on the driver's list until after
> > the remove method has returned. This forces the necessary
> > synchronization to occur.
>
> Is this something that you all feel is worth getting in for 2.6.24?
> Does it fix a regression that just showed up, or is just a bugfix for
> something that people finally realized has always been there?
>
> Can it wait for 2.6.25 to get some more testing in -mm?
Considering how few people have been affected by it, I'm willing to
let it wait for 2.6.25. David Woodhouse may feel differently.
Alan Stern
On Fri, Nov 16, 2007 at 03:45:55PM -0500, David Woodhouse wrote:
> On Fri, 2007-11-16 at 12:37 -0800, Greg KH wrote:
> > Is this something that you all feel is worth getting in for 2.6.24?
> > Does it fix a regression that just showed up, or is just a bugfix for
> > something that people finally realized has always been there?
>
> The latter. We just happen to kick the wireless device in the head by
> toggling its out-of-band reset signal when we unload the driver, which
> causes us to trigger this "unlikely" race with almost 100% reliability.
> But it's always been there, AFAICT.
>
> > Can it wait for 2.6.25 to get some more testing in -mm?
>
> Yeah, I think so.
Thanks for letting me know, I'll add it to the proper place in my queue.
greg k-h
This is a note to let you know that I've just added the patch titled
Subject: Driver core: fix race in __device_release_driver
to my gregkh-2.6 tree. Its filename is
driver-core-fix-race-in-__device_release_driver.patch
This tree can be found at
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/
>From [email protected] Mon Nov 26 22:49:20 2007
From: Alan Stern <[email protected]>
Date: Fri, 16 Nov 2007 11:57:28 -0500 (EST)
Subject: Driver core: fix race in __device_release_driver
To: Greg KH <[email protected]>, David Woodhouse <[email protected]>
Cc: USB development list <[email protected]>, Kernel development list <[email protected]>
Message-ID: <[email protected]>
This patch (as1013) was suggested by David Woodhouse; it fixes a race
in the driver core. If a device is unregistered at the same time as
its driver is unloaded, the driver's code pages may be unmapped while
the remove method is still running. The calls to get_driver() and
put_driver() were intended to prevent this, but they don't work if the
driver's module count has already dropped to 0.
Instead, the patch keeps the device on the driver's list until after
the remove method has returned. This forces the necessary
synchronization to occur.
Signed-off-by: Alan Stern <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/base/dd.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -289,11 +289,10 @@ static void __device_release_driver(stru
{
struct device_driver * drv;
- drv = get_driver(dev->driver);
+ drv = dev->driver;
if (drv) {
driver_sysfs_remove(dev);
sysfs_remove_link(&dev->kobj, "driver");
- klist_remove(&dev->knode_driver);
if (dev->bus)
blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
@@ -306,7 +305,7 @@ static void __device_release_driver(stru
drv->remove(dev);
devres_release_all(dev);
dev->driver = NULL;
- put_driver(drv);
+ klist_remove(&dev->knode_driver);
}
}
Patches currently in gregkh-2.6 which might be from [email protected] are
driver/pm-acquire-device-locks-prior-to-suspending.patch
driver/create-sys-...-power-when-config_pm-is-set.patch
driver/driver-core-fix-race-in-__device_release_driver.patch
usb/usb-add-support-for-an-older-firmware-revision-for-the-nikon-d200.patch
usb/usb-fix-priority-mistakes-in-drivers-usb-core-hub.c.patch
usb/usb-fix-signr-comment-in-usbdevice_fs.h.patch
usb/usb-mailing-lists-have-changed.patch
usb/usb-power-management-documenation-update.patch
usb/usb-hcd-avoid-duplicate-local_irq_disable.patch
usb/usb-usb-mon-mon_bin.c-cleanups.patch
usb/usb-keep-track-of-whether-interface-sysfs-files-exist.patch
usb/usb-uevent-environment-key-fix.patch
usb/usb-autosuspend-for-cdc-acm.patch
usb/usb-fix-up-ehci-startup-synchronization.patch
usb/usb-usb-storage-new-lockable-subclass-0x07.patch
usb/usb-don-t-change-hc-power-state-for-a-freeze.patch
usb/usb-dummy_hcd-don-t-register-drivers-on-the-platform-bus.patch
usb/usb-force-handover-port-to-companion-when-hub_port_connect_change-fails.patch
usb/usb-make-ksuspend_usbd-thread-non-freezable.patch
usb/usb-usb-storage-unusual_devs-entry-for-jetflash-ts1gjf2a.patch
usb/usb-storage-always-set-the-allow_restart-flag.patch