USB: fix crash when URBs are unlinked after the device is gone

This patch (as1151) protects usbcore against drivers that try to
unlink an URB after the URB's device or bus have been removed.  The
core does not currently check for this, and certain drivers can cause
a crash if they are running while an HCD is unloaded.

Certainly it would be best to fix the guilty drivers.  But a little
defensive programming doesn't hurt, especially since it appears that
quite a few drivers need to be fixed.

The patch prevents the problem by grabbing a reference to the device
while an unlink is in progress and using a new spinlock to synchronize
unlinks with device removal.  (There's no need to acquire a reference
to the bus as well, since the device structure itself keeps a
reference to the bus.)  In addition, the kerneldoc is updated to
indicate that URBs should not be unlinked after the disconnect method
returns.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Cc: stable <stable@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index fc9018e..e1b4262 100644 (file)
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -106,6 +106,9 @@ static DEFINE_SPINLOCK(hcd_root_hub_lock);
 /* used when updating an endpoint's URB list */
 static DEFINE_SPINLOCK(hcd_urb_list_lock);
 
+/* used to protect against unlinking URBs after the device is gone */
+static DEFINE_SPINLOCK(hcd_urb_unlink_lock);
+
 /* wait queue for synchronous unlinks */
 DECLARE_WAIT_QUEUE_HEAD(usb_kill_urb_queue);
 
@@ -1376,10 +1379,25 @@ static int unlink1(struct usb_hcd *hcd, struct urb *urb, int status)
 int usb_hcd_unlink_urb (struct urb *urb, int status)
 {
        struct usb_hcd          *hcd;
-       int                     retval;
+       int                     retval = -EIDRM;
+       unsigned long           flags;
 
-       hcd = bus_to_hcd(urb->dev->bus);
-       retval = unlink1(hcd, urb, status);
+       /* Prevent the device and bus from going away while
+        * the unlink is carried out.  If they are already gone
+        * then urb->use_count must be 0, since disconnected
+        * devices can't have any active URBs.
+        */
+       spin_lock_irqsave(&hcd_urb_unlink_lock, flags);
+       if (atomic_read(&urb->use_count) > 0) {
+               retval = 0;
+               usb_get_dev(urb->dev);
+       }
+       spin_unlock_irqrestore(&hcd_urb_unlink_lock, flags);
+       if (retval == 0) {
+               hcd = bus_to_hcd(urb->dev->bus);
+               retval = unlink1(hcd, urb, status);
+               usb_put_dev(urb->dev);
+       }
 
        if (retval == 0)
                retval = -EINPROGRESS;
@@ -1528,6 +1546,17 @@ void usb_hcd_disable_endpoint(struct usb_device *udev,
                hcd->driver->endpoint_disable(hcd, ep);
 }
 
+/* Protect against drivers that try to unlink URBs after the device
+ * is gone, by waiting until all unlinks for @udev are finished.
+ * Since we don't currently track URBs by device, simply wait until
+ * nothing is running in the locked region of usb_hcd_unlink_urb().
+ */
+void usb_hcd_synchronize_unlinks(struct usb_device *udev)
+{
+       spin_lock_irq(&hcd_urb_unlink_lock);
+       spin_unlock_irq(&hcd_urb_unlink_lock);
+}
+
 /*-------------------------------------------------------------------------*/
 
 /* called in any context */

diff --git a/drivers/usb/core/hcd.h b/drivers/usb/core/hcd.h
index 2dcde61..9465e70 100644 (file)
--- a/drivers/usb/core/hcd.h
+++ b/drivers/usb/core/hcd.h
@@ -232,6 +232,7 @@ extern void usb_hcd_flush_endpoint(struct usb_device *udev,
                struct usb_host_endpoint *ep);
 extern void usb_hcd_disable_endpoint(struct usb_device *udev,
                struct usb_host_endpoint *ep);
+extern void usb_hcd_synchronize_unlinks(struct usb_device *udev);
 extern int usb_hcd_get_frame_number(struct usb_device *udev);
 
 extern struct usb_hcd *usb_create_hcd(const struct hc_driver *driver,

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 9b3f16b..37ff8ae 100644 (file)
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1429,6 +1429,7 @@ void usb_disconnect(struct usb_device **pdev)
         */
        dev_dbg (&udev->dev, "unregistering device\n");
        usb_disable_device(udev, 0);
+       usb_hcd_synchronize_unlinks(udev);
 
        usb_unlock_device(udev);
 

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index f263800..4342bd9 100644 (file)
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -474,6 +474,12 @@ EXPORT_SYMBOL_GPL(usb_submit_urb);
  * indicating that the request has been canceled (rather than any other
  * code).
  *
+ * Drivers should not call this routine or related routines, such as
+ * usb_kill_urb() or usb_unlink_anchored_urbs(), after their disconnect
+ * method has returned.  The disconnect function should synchronize with
+ * a driver's I/O routines to insure that all URB-related activity has
+ * completed before it returns.
+ *
  * This request is always asynchronous.  Success is indicated by
  * returning -EINPROGRESS, at which time the URB will probably not yet
  * have been given back to the device driver.  When it is eventually
@@ -550,6 +556,9 @@ EXPORT_SYMBOL_GPL(usb_unlink_urb);
  * This routine may not be used in an interrupt context (such as a bottom
  * half or a completion handler), or when holding a spinlock, or in other
  * situations where the caller can't schedule().
+ *
+ * This routine should not be called by a driver after its disconnect
+ * method has returned.
  */
 void usb_kill_urb(struct urb *urb)
 {
@@ -588,6 +597,9 @@ EXPORT_SYMBOL_GPL(usb_kill_urb);
  * This routine may not be used in an interrupt context (such as a bottom
  * half or a completion handler), or when holding a spinlock, or in other
  * situations where the caller can't schedule().
+ *
+ * This routine should not be called by a driver after its disconnect
+ * method has returned.
  */
 void usb_poison_urb(struct urb *urb)
 {
@@ -622,6 +634,9 @@ EXPORT_SYMBOL_GPL(usb_unpoison_urb);
  *
  * this allows all outstanding URBs to be killed starting
  * from the back of the queue
+ *
+ * This routine should not be called by a driver after its disconnect
+ * method has returned.
  */
 void usb_kill_anchored_urbs(struct usb_anchor *anchor)
 {
@@ -651,6 +666,9 @@ EXPORT_SYMBOL_GPL(usb_kill_anchored_urbs);
  * this allows all outstanding URBs to be poisoned starting
  * from the back of the queue. Newly added URBs will also be
  * poisoned
+ *
+ * This routine should not be called by a driver after its disconnect
+ * method has returned.
  */
 void usb_poison_anchored_urbs(struct usb_anchor *anchor)
 {
@@ -672,6 +690,7 @@ void usb_poison_anchored_urbs(struct usb_anchor *anchor)
        spin_unlock_irq(&anchor->lock);
 }
 EXPORT_SYMBOL_GPL(usb_poison_anchored_urbs);
+
 /**
  * usb_unlink_anchored_urbs - asynchronously cancel transfer requests en masse
  * @anchor: anchor the requests are bound to
@@ -680,6 +699,9 @@ EXPORT_SYMBOL_GPL(usb_poison_anchored_urbs);
  * from the back of the queue. This function is asynchronous.
  * The unlinking is just tiggered. It may happen after this
  * function has returned.
+ *
+ * This routine should not be called by a driver after its disconnect
+ * method has returned.
  */
 void usb_unlink_anchored_urbs(struct usb_anchor *anchor)
 {