On Mon, Mar 20, 2017 at 05:12:20PM -0400, Alan Stern wrote: > On Mon, 20 Mar 2017, Guenter Roeck wrote: > > > While running a bind/unbind stress test with the dwc3 usb driver on rk3399, > > the following crash was observed. > > > > Unable to handle kernel NULL pointer dereference at virtual address 00000218 > > pgd = ffffffc00165f000 > > [00000218] *pgd=000000000174f003, *pud=000000000174f003, > > *pmd=0000000001750003, *pte=00e8000001751713 > > ... > > > Code analysis shows that intf is set to NULL in usb_disable_device() prior > > to setting actconfig to NULL. At the same time, usb_runtime_idle() does not > > lock the usb device, and neither does any of the functions in the > > traceback. This means that there is no protection against a race condition > > where usb_disable_device() is removing dev->actconfig->interface[] pointers > > while those are being accessed from autosuspend_check(). > > > > To solve the problem, synchronize and validate device state between > > autosuspend_check() and usb_disconnect(). > > > > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx> > > --- > > v2: Do not disable autosuspend in usb_disconnect(). Instead, check the > > usb device state in autosuspend_check(). > > In usb_disconnect(), call pm_runtime_barrier() earlier, immediately > > after changing the USB device state. > > > > drivers/usb/core/driver.c | 3 +++ > > drivers/usb/core/hub.c | 7 +++++++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > > index 7ebdf2a4e8fe..eb87a259d55c 100644 > > --- a/drivers/usb/core/driver.c > > +++ b/drivers/usb/core/driver.c > > @@ -1781,6 +1781,9 @@ static int autosuspend_check(struct usb_device *udev) > > int w, i; > > struct usb_interface *intf; > > > > + if (udev->state == USB_STATE_NOTATTACHED) > > + return -ENODEV; > > + > > /* Fail if autosuspend is disabled, or any interfaces are in use, or > > * any interface drivers require remote wakeup but it isn't available. > > */ > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > index 57bb427c8878..fd80513aaeff 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -2084,6 +2084,13 @@ void usb_disconnect(struct usb_device **pdev) > > * this quiesces everything except pending urbs. > > */ > > usb_set_device_state(udev, USB_STATE_NOTATTACHED); > > + > > + /* > > + * Ensure that the pm runtime code knows that the USB device > > + * is in the process of being disconnected. > > + */ > > + pm_runtime_barrier(&udev->dev); > > + > > dev_info(&udev->dev, "USB disconnect, device number %d\n", > > udev->devnum); > > > > Very minor nit: I would place the pm_runtime_barrier() after the > dev_info(), so that we would know the device is going away if anything > should go wrong during the barrier call. > Makes sense. I'll change that and resend. Thanks, Guenter > Aside from that, > > Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > Alan Stern > -- 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
