Commit 0d3e5a2e authored by Patrick Mochel's avatar Patrick Mochel Committed by Greg Kroah-Hartman

[PATCH] Driver Core: fix bk-driver-core kills ppc64

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: default avatarAndrew Morton <akpm@osdl.org>
parent b86c1df1
...@@ -35,6 +35,8 @@ ...@@ -35,6 +35,8 @@
* nor take the bus's rwsem. Please verify those are accounted * nor take the bus's rwsem. Please verify those are accounted
* for before calling this. (It is ok to call with no other effort * for before calling this. (It is ok to call with no other effort
* from a driver's probe() method.) * from a driver's probe() method.)
*
* This function must be called with @dev->sem held.
*/ */
void device_bind_driver(struct device * dev) void device_bind_driver(struct device * dev)
{ {
...@@ -57,54 +59,56 @@ void device_bind_driver(struct device * dev) ...@@ -57,54 +59,56 @@ void device_bind_driver(struct device * dev)
* because we don't know the format of the ID structures, nor what * because we don't know the format of the ID structures, nor what
* is to be considered a match and what is not. * is to be considered a match and what is not.
* *
* 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)) 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; dev->driver = drv;
if (drv->probe) { if (drv->probe) {
error = drv->probe(dev); ret = drv->probe(dev);
if (error) { if (ret) {
dev->driver = NULL; dev->driver = NULL;
up(&dev->sem); goto ProbeFailed;
return error;
} }
} }
up(&dev->sem);
device_bind_driver(dev); 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) static int __device_attach(struct device_driver * drv, void * data)
{ {
struct device * dev = data; struct device * dev = data;
int error; return driver_probe_device(drv, dev);
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;
} }
/** /**
...@@ -114,37 +118,43 @@ static int __device_attach(struct device_driver * drv, void * data) ...@@ -114,37 +118,43 @@ static int __device_attach(struct device_driver * drv, void * data)
* Walk the list of drivers that the bus has and call * Walk the list of drivers that the bus has and call
* driver_probe_device() for each pair. If a compatible * driver_probe_device() for each pair. If a compatible
* pair is found, break out and return. * 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 device_attach(struct device * dev)
{ {
int ret = 0;
down(&dev->sem);
if (dev->driver) { if (dev->driver) {
device_bind_driver(dev); device_bind_driver(dev);
return 1; ret = 1;
} } else
ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach);
return bus_for_each_drv(dev->bus, NULL, dev, __device_attach); up(&dev->sem);
return ret;
} }
static int __driver_attach(struct device * dev, void * data) static int __driver_attach(struct device * dev, void * data)
{ {
struct device_driver * drv = data; struct device_driver * drv = data;
int error = 0;
/*
if (!dev->driver) { * Lock device and try to bind to it. We drop the error
error = driver_probe_device(drv, dev); * here and always return 0, because we need to keep trying
if (error) { * to bind to devices and some drivers will return an error
if (error != -ENODEV) { * simply if it didn't support the device.
/* driver matched but the probe failed */ *
printk(KERN_WARNING * driver_probe_device() will spit a warning if there
"%s: probe of %s failed with error %d\n", * is an error.
drv->name, dev->bus_id, error); */
} else
error = 0; down(&dev->sem);
return error; if (!dev->driver)
} driver_probe_device(drv, dev);
/* stop looking, this driver is attached */ up(&dev->sem);
return 1;
}
return 0; return 0;
} }
...@@ -156,9 +166,6 @@ static int __driver_attach(struct device * dev, void * data) ...@@ -156,9 +166,6 @@ static int __driver_attach(struct device * dev, void * data)
* match the driver with each one. If driver_probe_device() * match the driver with each one. If driver_probe_device()
* returns 0 and the @dev->driver is set, we've found a * returns 0 and the @dev->driver is set, we've found a
* compatible pair. * 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) void driver_attach(struct device_driver * drv)
{ {
...@@ -176,19 +183,19 @@ void driver_attach(struct device_driver * drv) ...@@ -176,19 +183,19 @@ void driver_attach(struct device_driver * drv)
*/ */
void device_release_driver(struct device * dev) void device_release_driver(struct device * dev)
{ {
struct device_driver * drv = dev->driver; struct device_driver * drv;
if (!drv)
return;
sysfs_remove_link(&drv->kobj, kobject_name(&dev->kobj));
sysfs_remove_link(&dev->kobj, "driver");
klist_del(&dev->knode_driver);
down(&dev->sem); down(&dev->sem);
if (drv->remove) if (dev->driver) {
drv->remove(dev); drv = dev->driver;
dev->driver = NULL; sysfs_remove_link(&drv->kobj, kobject_name(&dev->kobj));
sysfs_remove_link(&dev->kobj, "driver");
klist_del(&dev->knode_driver);
if (drv->remove)
drv->remove(dev);
dev->driver = NULL;
}
up(&dev->sem); up(&dev->sem);
} }
...@@ -208,7 +215,6 @@ void driver_detach(struct device_driver * drv) ...@@ -208,7 +215,6 @@ void driver_detach(struct device_driver * drv)
} }
EXPORT_SYMBOL_GPL(driver_probe_device);
EXPORT_SYMBOL_GPL(device_bind_driver); EXPORT_SYMBOL_GPL(device_bind_driver);
EXPORT_SYMBOL_GPL(device_release_driver); EXPORT_SYMBOL_GPL(device_release_driver);
EXPORT_SYMBOL_GPL(device_attach); EXPORT_SYMBOL_GPL(device_attach);
......
...@@ -325,7 +325,6 @@ extern int device_for_each_child(struct device *, void *, ...@@ -325,7 +325,6 @@ extern int device_for_each_child(struct device *, void *,
* Manual binding of a device to driver. See drivers/base/bus.c * Manual binding of a device to driver. See drivers/base/bus.c
* for information on use. * 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_bind_driver(struct device * dev);
extern void device_release_driver(struct device * dev); extern void device_release_driver(struct device * dev);
extern int device_attach(struct device * dev); extern int device_attach(struct device * dev);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment