Re: [PATCH v2] usb: hub: Do not attempt to autosuspend disconnected devices

[Guenter Roeck <linux@roeck-us.net>]

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@roeck-us.net>
> > ---
> > 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@rowland.harvard.edu>
> 
> Alan Stern
>