On Mon, 2009-12-14 at 20:17 -0700, russ.dill@xxxxxxxxx wrote:
>
> On Mon, Dec 14, 2009 at 8:09 PM, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> >
> > On Mon, 14 Dec 2009, Linus Torvalds wrote:
> >>
> >> I'll double-check by reverting it from current -tip, but if you don't hear
> >> anything different from me, you can assume that that double-check
> >> succeeded and confirms that that commit really is the cause of my printer
> >> no longer working.
> >
> > Confirmed.
> >
> > With that commit reverted, everything works fine again. So it's definitely
> > a2582bd478c13c574d4c16ef1209d333f2a25935, although I have no idea _why_ it
> > causes problems.
> >
> > One thing I note is that it literally changed semantics, as per the
> > comment in there. It used to be:
> >
> > This walks the driver device list and returns a pointer to the interface
> > with the matching minor.
> >
> > and after that it is
> >
> > This walks the bus device list and returns a pointer to the interface
> > with the matching minor.
> >
> > Notice the "driver device list" -> "bus device list" change.
> >
> > Also notice that the minor number of my 'lp0' device is zero, so I could
> > easily imagine that some device without a driver at all or whatever will
> > have a zero there, and then it would return the wrong device entirely.
> >
> > So I really think that commit is very suspect. Not matching against the
> > driver that we passed in looks like some total screwup.
> >
> > But I don't know the code or the use. I do know that reverting it fixes
> > it.
>
> Yes, you are right. Sorry for breaking your printer. There is both USB_MAJOR and USB_DEVICE_MAJOR. I haven't investigated it in great detail yet, but that seems likely to be the issue. I'll clean up the patch. And yes, it should definitely not be going into stable yet. Revert for now and I'll resubmit.
Here is a cleaned up patch that matches against minor and driver, not
just minor. I'll haven't test it yet, but will get a chance to tomorrow.
From 4441c99c80d8298a99b6f2fbd4ece9d3cfe7fd4d Mon Sep 17 00:00:00 2001
From: Russ Dill <Russ.Dill@xxxxxxxxx>
Date: Wed, 18 Nov 2009 10:31:27 -0700
Subject: [PATCH] Close usb_find_interface race v2
USB drivers that create character devices call usb_register_dev in their
probe function. This associates the usb_interface device with that minor
number and creates the character device and announces it to the world.
However, the driver's probe function is called before the new
usb_interface is added to the driver's klist_devices.
This is a problem because userspace will respond to the character device
creation announcement by opening the character device. The driver's open
function will the call usb_find_interface to find the usb_interface
associated with that minor number. usb_find_interface will walk the
driver's list of devices and find the usb_interface with the matching
minor number.
Because the announcement happens before the usb_interface is added to the
driver's klist_devices, a race condition exists. A straightforward fix
is to walk the list of devices on usb_bus_type instead since the device
is added to that list before the announcement occurs.
bus_find_device calls get_device to bump the reference count on the found
device. It is arguable that the reference count should be dropped by the
caller of usb_find_interface instead of usb_find_interface, however,
the current users of usb_find_interface do not expect this.
The original version of this patch only matched against minor number
instead of driver and minor number. This version matches against both.
Signed-off-by: Russ Dill <Russ.Dill@xxxxxxxxx>
---
drivers/usb/core/usb.c | 28 +++++++++++++++-------------
1 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index b1b85ab..2091fac 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -132,7 +132,7 @@ EXPORT_SYMBOL_GPL(usb_altnum_to_altsetting);
struct find_interface_arg {
int minor;
- struct usb_interface *interface;
+ struct device_driver *drv;
};
static int __find_interface(struct device *dev, void *data)
@@ -144,10 +144,9 @@ static int __find_interface(struct device *dev, void *data)
return 0;
intf = to_usb_interface(dev);
- if (intf->minor != -1 && intf->minor == arg->minor) {
- arg->interface = intf;
+ if (intf->minor != -1 && intf->minor == arg->minor &&
+ dev->driver == arg->drv)
return 1;
- }
return 0;
}
@@ -156,21 +155,24 @@ static int __find_interface(struct device *dev, void *data)
* @drv: the driver whose current configuration is considered
* @minor: the minor number of the desired device
*
- * This walks the driver device list and returns a pointer to the interface
- * with the matching minor. Note, this only works for devices that share the
- * USB major number.
+ * This walks the bus device list and returns a pointer to the interface
+ * with the matching minor and driver. Note, this only works for devices
+ * that share the USB major number.
*/
struct usb_interface *usb_find_interface(struct usb_driver *drv, int minor)
{
struct find_interface_arg argb;
- int retval;
+ struct device *dev;
argb.minor = minor;
- argb.interface = NULL;
- /* eat the error, it will be in argb.interface */
- retval = driver_for_each_device(&drv->drvwrap.driver, NULL, &argb,
- __find_interface);
- return argb.interface;
+ argb.drv = &drv->drvwrap.driver;
+
+ dev = bus_find_device(&usb_bus_type, NULL, &argb, __find_interface);
+
+ /* Drop reference count from bus_find_device */
+ put_device(dev);
+
+ return dev ? to_usb_interface(dev) : NULL;
}
EXPORT_SYMBOL_GPL(usb_find_interface);
--
1.6.5
Attachment:
signature.asc
Description: This is a digitally signed message part
