On Mon, 17 Sep 2012, Alan Stern wrote:
> On Mon, 17 Sep 2012, Don Zickus wrote:
>
> > I have added some in-depth analysis from our customer.
> >
> >
> > The problem is that the failing routine was called with a pointer to usbdev in
> > memory that has already been freed and overwritten with the pool poison pattern.
> > This causes an access violation on line 502:
>
> The stack trace shows that the failure occurred in usb_device_dump(),
> which was called from usb_device_read(). Since it wasn't a recursive
> call, the usbdev argument must have pointed to the root-hub device.
>
> I don't see how this could have happened if the kernel included the
> patch I sent earlier. usb_device_read() holds the usb_bus_list_lock
> mutex throughout, and it tests that the root hub's devnum field is not
> equal to 0 before calling usb_dump_device().
>
> Meanwhile, usb_remove_hcd() sets hcd->self.bus.root_hub->devnum to 0
> while holding usb_bus_list_lock, and it doesn't deallocate the root hub
> device until after the mutex is dropped.
>
> Are you certain that the test was conducted with the patch in place?
>
> If you want to track down what's going wrong, you'll have to add some
> debugging code to usb_device_read() and usb_remove_hcd(). By the time
> usb_device_dump() starts running, it's already too late.
After thinking about this some more, I realized that my patch still
leaves a race -- although the oops would occur in a different place
(where usb_device_read checks bus->root_hub->devnum).
Here's a different patch which should work better. It relies on the
rh_registered flag in the usb_hcd structure, which persists as long as
the usb_bus structure does, rather than on anything stored in the
root-hub device structure.
Alan Stern
Index: usb-3.6/drivers/usb/core/devices.c
===================================================================
--- usb-3.6.orig/drivers/usb/core/devices.c
+++ usb-3.6/drivers/usb/core/devices.c
@@ -624,7 +624,7 @@ static ssize_t usb_device_read(struct fi
/* print devices for all busses */
list_for_each_entry(bus, &usb_bus_list, bus_list) {
/* recurse through all children of the root hub */
- if (!bus->root_hub)
+ if (!bus_to_hcd(bus)->rh_registered)
continue;
usb_lock_device(bus->root_hub);
ret = usb_device_dump(&buf, &nbytes, &skip_bytes, ppos,
Index: usb-3.6/drivers/usb/core/hcd.c
===================================================================
--- usb-3.6.orig/drivers/usb/core/hcd.c
+++ usb-3.6/drivers/usb/core/hcd.c
@@ -1011,10 +1011,7 @@ static int register_root_hub(struct usb_
if (retval) {
dev_err (parent_dev, "can't register root hub for %s, %d\n",
dev_name(&usb_dev->dev), retval);
- }
- mutex_unlock(&usb_bus_list_lock);
-
- if (retval == 0) {
+ } else {
spin_lock_irq (&hcd_root_hub_lock);
hcd->rh_registered = 1;
spin_unlock_irq (&hcd_root_hub_lock);
@@ -1023,6 +1020,7 @@ static int register_root_hub(struct usb_
if (HCD_DEAD(hcd))
usb_hc_died (hcd); /* This time clean up */
}
+ mutex_unlock(&usb_bus_list_lock);
return retval;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
