Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755368AbdCTVMX (ORCPT ); Mon, 20 Mar 2017 17:12:23 -0400 Received: from netrider.rowland.org ([192.131.102.5]:39367 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1754417AbdCTVMW (ORCPT ); Mon, 20 Mar 2017 17:12:22 -0400 Date: Mon, 20 Mar 2017 17:12:20 -0400 (EDT) From: Alan Stern X-X-Sender: stern@netrider.rowland.org To: Guenter Roeck cc: Greg Kroah-Hartman , , , Douglas Anderson , Brian Norris Subject: Re: [PATCH v2] usb: hub: Do not attempt to autosuspend disconnected devices In-Reply-To: <1490025036-6578-1-git-send-email-linux@roeck-us.net> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2763 Lines: 75 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 > --- > 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. Aside from that, Acked-by: Alan Stern Alan Stern