From akpm@osdl.org Tue Apr 5 23:47:11 2005 Date: Tue, 5 Apr 2005 23:46:33 -0700 From: Andrew Morton To: Patrick Mochel Cc: greg@kroah.com Subject: Driver Core: fix bk-driver-core kills ppc64 From: Patrick Mochel There's no check to see if the device is already bound to a driver, which could do bad things. The first thing to go wrong is that it will try to match a driver with a device already bound to one. In some cases (it appears with USB with drivers/usb/core/usb.c::usb_match_id()), some drivers will match a device based on the class type, so it would be common (especially for HID devices) to match a device that is already bound. The fun comes when ->probe() is called, it fails, then driver_probe_device() does this: dev->driver = NULL; Later on, that pointer could be be dereferenced without checking and cause hell to break loose. This problem could be nasty. It's very hardware dependent, since some devices could have a different set of matching qualifiers than others. Now, I don't quite see exactly where/how you were getting that crash. You're dereferencing bad memory, but I'm not sure which pointer was bad and where it came from, but it could have come from a couple of different places. The patch below will hopefully fix it all up for you. It's against 2.6.12-rc2-mm1, and does the following: - Move logic to driver_probe_device() and comments uncommon returns: 1 - If device is bound 0 - If device not bound, and no error error - If there was an error. - Move locking to caller of that function, since we want to lock a device for the entire time we're trying to bind it to a driver (to prevent against a driver being loaded at the same time). - Update __device_attach() and __driver_attach() to do that locking. - Check if device is already bound in __driver_attach() - Update the converse device_release_driver() so it locks the device around all of the operations. - Mark driver_probe_device() as static and remove export. It's an internal function, it should stay that way, and there are no other callers. If there is ever a need to export it, we can audit it as necessary. Signed-off-by: Andrew Morton --- drivers/base/dd.c | 141 +++++++++++++++++++++++++------------------------ include/linux/device.h | 1 2 files changed, 74 insertions(+), 68 deletions(-) --- gregkh-2.6.orig/drivers/base/dd.c 2005-05-06 09:23:31.000000000 -0700 +++ gregkh-2.6/drivers/base/dd.c 2005-05-06 09:23:32.000000000 -0700 @@ -35,6 +35,8 @@ * nor take the bus's rwsem. Please verify those are accounted * for before calling this. (It is ok to call with no other effort * from a driver's probe() method.) + * + * This function must be called with @dev->sem held. */ void device_bind_driver(struct device * dev) @@ -61,56 +63,59 @@ * * If we find a match, we call @drv->probe(@dev) if it exists, and * call device_bind_driver() above. + * + * This function returns 1 if a match is found, an error if one + * occurs (that is not -ENODEV or -ENXIO), and 0 otherwise. + * + * This function must be called with @dev->sem held. */ -int driver_probe_device(struct device_driver * drv, struct device * dev) +static int driver_probe_device(struct device_driver * drv, struct device * dev) { - int error = 0; + int ret = 0; if (drv->bus->match && !drv->bus->match(dev, drv)) - return -ENODEV; + goto Done; - down(&dev->sem); + pr_debug("%s: Matched Device %s with Driver %s\n", + drv->bus->name, dev->bus_id, drv->name); dev->driver = drv; if (drv->probe) { - error = drv->probe(dev); - if (error) { + ret = drv->probe(dev); + if (ret) { dev->driver = NULL; - up(&dev->sem); - return error; + goto ProbeFailed; } } - up(&dev->sem); device_bind_driver(dev); - return 0; + ret = 1; + pr_debug("%s: Bound Device %s to Driver %s\n", + drv->bus->name, dev->bus_id, drv->name); + goto Done; + + ProbeFailed: + if (ret == -ENODEV || ret == -ENXIO) { + /* Driver matched, but didn't support device + * or device not found. + * Not an error; keep going. + */ + ret = 0; + } else { + /* driver matched but the probe failed */ + printk(KERN_WARNING + "%s: probe of %s failed with error %d\n", + drv->name, dev->bus_id, ret); + } + Done: + return ret; } static int __device_attach(struct device_driver * drv, void * data) { struct device * dev = data; - int error; - - error = driver_probe_device(drv, dev); - if (error) { - if ((error == -ENODEV) || (error == -ENXIO)) { - /* Driver matched, but didn't support device - * or device not found. - * Not an error; keep going. - */ - error = 0; - } else { - /* driver matched but the probe failed */ - printk(KERN_WARNING - "%s: probe of %s failed with error %d\n", - drv->name, dev->bus_id, error); - } - return error; - } - /* stop looking, this device is attached */ - return 1; + return driver_probe_device(drv, dev); } - /** * device_attach - try to attach device to a driver. * @dev: device. @@ -118,38 +123,44 @@ * Walk the list of drivers that the bus has and call * driver_probe_device() for each pair. If a compatible * pair is found, break out and return. + * + * Returns 1 if the device was bound to a driver; 0 otherwise. */ int device_attach(struct device * dev) { + int ret = 0; + + down(&dev->sem); if (dev->driver) { device_bind_driver(dev); - return 1; - } - - return bus_for_each_drv(dev->bus, NULL, dev, __device_attach); + ret = 1; + } else + ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach); + up(&dev->sem); + return ret; } static int __driver_attach(struct device * dev, void * data) { struct device_driver * drv = data; - int error = 0; - if (!dev->driver) { - error = driver_probe_device(drv, dev); - if (error) { - if (error != -ENODEV) { - /* driver matched but the probe failed */ - printk(KERN_WARNING - "%s: probe of %s failed with error %d\n", - drv->name, dev->bus_id, error); - } else - error = 0; - return error; - } - /* stop looking, this driver is attached */ - return 1; - } + /* + * Lock device and try to bind to it. We drop the error + * here and always return 0, because we need to keep trying + * to bind to devices and some drivers will return an error + * simply if it didn't support the device. + * + * driver_probe_device() will spit a warning if there + * is an error. + */ + + down(&dev->sem); + if (!dev->driver) + driver_probe_device(drv, dev); + up(&dev->sem); + + return 0; } @@ -161,9 +172,6 @@ * match the driver with each one. If driver_probe_device() * returns 0 and the @dev->driver is set, we've found a * compatible pair. - * - * Note that we ignore the -ENODEV error from driver_probe_device(), - * since it's perfectly valid for a driver not to bind to any devices. */ void driver_attach(struct device_driver * drv) { @@ -183,20 +191,20 @@ void device_release_driver(struct device * dev) { - struct device_driver * drv = dev->driver; - - if (!drv) - return; - - sysfs_remove_link(&drv->kobj, kobject_name(&dev->kobj)); - sysfs_remove_link(&dev->kobj, "driver"); - klist_del(&dev->knode_driver); + struct device_driver * drv; down(&dev->sem); - device_detach_shutdown(dev); - if (drv->remove) - drv->remove(dev); - dev->driver = NULL; + if (dev->driver) { + drv = dev->driver; + sysfs_remove_link(&drv->kobj, kobject_name(&dev->kobj)); + sysfs_remove_link(&dev->kobj, "driver"); + klist_del(&dev->knode_driver); + + device_detach_shutdown(dev); + if (drv->remove) + drv->remove(dev); + dev->driver = NULL; + } up(&dev->sem); } @@ -217,7 +225,6 @@ } -EXPORT_SYMBOL_GPL(driver_probe_device); EXPORT_SYMBOL_GPL(device_bind_driver); EXPORT_SYMBOL_GPL(device_release_driver); EXPORT_SYMBOL_GPL(device_attach); --- gregkh-2.6.orig/include/linux/device.h 2005-05-06 09:23:29.000000000 -0700 +++ gregkh-2.6/include/linux/device.h 2005-05-06 09:23:32.000000000 -0700 @@ -328,7 +328,6 @@ * Manual binding of a device to driver. See drivers/base/bus.c * for information on use. */ -extern int driver_probe_device(struct device_driver * drv, struct device * dev); extern void device_bind_driver(struct device * dev); extern void device_release_driver(struct device * dev); extern int device_attach(struct device * dev);