Skip to content

Commit

Permalink
USB: fix deadlock in bConfigurationValue attribute method
Browse files Browse the repository at this point in the history
This patch (as154) fixes a self-deadlock that occurs when userspace
writes to the bConfigurationValue sysfs attribute for a hub with
children.  The task tries to lock the bandwidth_mutex at a time when
it already owns the lock:

	The attribute's method calls usb_set_configuration(),
	which calls usb_disable_device() with the bandwidth_mutex
	held.

	usb_disable_device() unregisters the existing interfaces,
	which causes the hub driver to be unbound.

	The hub_disconnect() routine calls hub_quiesce(), which
	calls usb_disconnect() for each of the hub's children.

	usb_disconnect() attempts to acquire the bandwidth_mutex
	around a call to usb_disable_device().

The solution is to make usb_disable_device() acquire the mutex for
itself instead of requiring the caller to hold it.  Then the mutex can
cover only the bandwidth deallocation operation and not the region
where the interfaces are unregistered.

This has the potential to change system behavior slightly when a
config change races with another config or altsetting change.  Some of
the bandwidth released from the old config might get claimed by the
other config or altsetting, make it impossible to restore the old
config in case of a failure.  But since we don't try to recover from
config-change failures anyway, this doesn't matter.

[This should be marked for stable kernels that contain the commit
fccf4e8 "USB: Free bandwidth when
usb_disable_device is called."
That commit was marked for stable kernels as old as 2.6.32.]

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
Alan Stern authored and Greg Kroah-Hartman committed Apr 17, 2012
1 parent edffaa0 commit 8963c48
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 6 deletions.
3 changes: 0 additions & 3 deletions drivers/usb/core/hub.c
Original file line number Diff line number Diff line change
Expand Up @@ -1667,7 +1667,6 @@ void usb_disconnect(struct usb_device **pdev)
{
struct usb_device *udev = *pdev;
int i;
struct usb_hcd *hcd = bus_to_hcd(udev->bus);

/* mark the device as inactive, so any further urb submissions for
* this device (and any of its children) will fail immediately.
Expand All @@ -1690,9 +1689,7 @@ void usb_disconnect(struct usb_device **pdev)
* so that the hardware is now fully quiesced.
*/
dev_dbg (&udev->dev, "unregistering device\n");
mutex_lock(hcd->bandwidth_mutex);
usb_disable_device(udev, 0);
mutex_unlock(hcd->bandwidth_mutex);
usb_hcd_synchronize_unlinks(udev);

usb_remove_ep_devs(&udev->ep0);
Expand Down
6 changes: 3 additions & 3 deletions drivers/usb/core/message.c
Original file line number Diff line number Diff line change
Expand Up @@ -1136,8 +1136,6 @@ void usb_disable_interface(struct usb_device *dev, struct usb_interface *intf,
* Deallocates hcd/hardware state for the endpoints (nuking all or most
* pending urbs) and usbcore state for the interfaces, so that usbcore
* must usb_set_configuration() before any interfaces could be used.
*
* Must be called with hcd->bandwidth_mutex held.
*/
void usb_disable_device(struct usb_device *dev, int skip_ep0)
{
Expand Down Expand Up @@ -1190,7 +1188,9 @@ void usb_disable_device(struct usb_device *dev, int skip_ep0)
usb_disable_endpoint(dev, i + USB_DIR_IN, false);
}
/* Remove endpoints from the host controller internal state */
mutex_lock(hcd->bandwidth_mutex);
usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL);
mutex_unlock(hcd->bandwidth_mutex);
/* Second pass: remove endpoint pointers */
}
for (i = skip_ep0; i < 16; ++i) {
Expand Down Expand Up @@ -1750,7 +1750,6 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
/* if it's already configured, clear out old state first.
* getting rid of old interfaces means unbinding their drivers.
*/
mutex_lock(hcd->bandwidth_mutex);
if (dev->state != USB_STATE_ADDRESS)
usb_disable_device(dev, 1); /* Skip ep0 */

Expand All @@ -1763,6 +1762,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
* host controller will not allow submissions to dropped endpoints. If
* this call fails, the device state is unchanged.
*/
mutex_lock(hcd->bandwidth_mutex);
ret = usb_hcd_alloc_bandwidth(dev, cp, NULL, NULL);
if (ret < 0) {
mutex_unlock(hcd->bandwidth_mutex);
Expand Down

0 comments on commit 8963c48

Please sign in to comment.