Commit c2b71462 authored by Alan Stern's avatar Alan Stern Committed by Greg Kroah-Hartman

USB: core: Fix bug caused by duplicate interface PM usage counter

The syzkaller fuzzer reported a bug in the USB hub driver which turned
out to be caused by a negative runtime-PM usage counter.  This allowed
a hub to be runtime suspended at a time when the driver did not expect
it.  The symptom is a WARNING issued because the hub's status URB is
submitted while it is already active:

	URB 0000000031fb463e submitted while active
	WARNING: CPU: 0 PID: 2917 at drivers/usb/core/urb.c:363

The negative runtime-PM usage count was caused by an unfortunate
design decision made when runtime PM was first implemented for USB.
At that time, USB class drivers were allowed to unbind from their
interfaces without balancing the usage counter (i.e., leaving it with
a positive count).  The core code would take care of setting the
counter back to 0 before allowing another driver to bind to the
interface.

Later on when runtime PM was implemented for the entire kernel, the
opposite decision was made: Drivers were required to balance their
runtime-PM get and put calls.  In order to maintain backward
compatibility, however, the USB subsystem adapted to the new
implementation by keeping an independent usage counter for each
interface and using it to automatically adjust the normal usage
counter back to 0 whenever a driver was unbound.

This approach involves duplicating information, but what is worse, it
doesn't work properly in cases where a USB class driver delays
decrementing the usage counter until after the driver's disconnect()
routine has returned and the counter has been adjusted back to 0.
Doing so would cause the usage counter to become negative.  There's
even a warning about this in the USB power management documentation!

As it happens, this is exactly what the hub driver does.  The
kick_hub_wq() routine increments the runtime-PM usage counter, and the
corresponding decrement is carried out by hub_event() in the context
of the hub_wq work-queue thread.  This work routine may sometimes run
after the driver has been unbound from its interface, and when it does
it causes the usage counter to go negative.

It is not possible for hub_disconnect() to wait for a pending
hub_event() call to finish, because hub_disconnect() is called with
the device lock held and hub_event() acquires that lock.  The only
feasible fix is to reverse the original design decision: remove the
duplicate interface-specific usage counter and require USB drivers to
balance their runtime PM gets and puts.  As far as I know, all
existing drivers currently do this.
Signed-off-by: default avatarAlan Stern <stern@rowland.harvard.edu>
Reported-and-tested-by: syzbot+7634edaea4d0b341c625@syzkaller.appspotmail.com
CC: <stable@vger.kernel.org>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent fc834e60
...@@ -370,11 +370,15 @@ autosuspend the interface's device. When the usage counter is = 0 ...@@ -370,11 +370,15 @@ autosuspend the interface's device. When the usage counter is = 0
then the interface is considered to be idle, and the kernel may then the interface is considered to be idle, and the kernel may
autosuspend the device. autosuspend the device.
Drivers need not be concerned about balancing changes to the usage Drivers must be careful to balance their overall changes to the usage
counter; the USB core will undo any remaining "get"s when a driver counter. Unbalanced "get"s will remain in effect when a driver is
is unbound from its interface. As a corollary, drivers must not call unbound from its interface, preventing the device from going into
any of the ``usb_autopm_*`` functions after their ``disconnect`` runtime suspend should the interface be bound to a driver again. On
routine has returned. the other hand, drivers are allowed to achieve this balance by calling
the ``usb_autopm_*`` functions even after their ``disconnect`` routine
has returned -- say from within a work-queue routine -- provided they
retain an active reference to the interface (via ``usb_get_intf`` and
``usb_put_intf``).
Drivers using the async routines are responsible for their own Drivers using the async routines are responsible for their own
synchronization and mutual exclusion. synchronization and mutual exclusion.
......
...@@ -473,11 +473,6 @@ static int usb_unbind_interface(struct device *dev) ...@@ -473,11 +473,6 @@ static int usb_unbind_interface(struct device *dev)
pm_runtime_disable(dev); pm_runtime_disable(dev);
pm_runtime_set_suspended(dev); pm_runtime_set_suspended(dev);
/* Undo any residual pm_autopm_get_interface_* calls */
for (r = atomic_read(&intf->pm_usage_cnt); r > 0; --r)
usb_autopm_put_interface_no_suspend(intf);
atomic_set(&intf->pm_usage_cnt, 0);
if (!error) if (!error)
usb_autosuspend_device(udev); usb_autosuspend_device(udev);
...@@ -1633,7 +1628,6 @@ void usb_autopm_put_interface(struct usb_interface *intf) ...@@ -1633,7 +1628,6 @@ void usb_autopm_put_interface(struct usb_interface *intf)
int status; int status;
usb_mark_last_busy(udev); usb_mark_last_busy(udev);
atomic_dec(&intf->pm_usage_cnt);
status = pm_runtime_put_sync(&intf->dev); status = pm_runtime_put_sync(&intf->dev);
dev_vdbg(&intf->dev, "%s: cnt %d -> %d\n", dev_vdbg(&intf->dev, "%s: cnt %d -> %d\n",
__func__, atomic_read(&intf->dev.power.usage_count), __func__, atomic_read(&intf->dev.power.usage_count),
...@@ -1662,7 +1656,6 @@ void usb_autopm_put_interface_async(struct usb_interface *intf) ...@@ -1662,7 +1656,6 @@ void usb_autopm_put_interface_async(struct usb_interface *intf)
int status; int status;
usb_mark_last_busy(udev); usb_mark_last_busy(udev);
atomic_dec(&intf->pm_usage_cnt);
status = pm_runtime_put(&intf->dev); status = pm_runtime_put(&intf->dev);
dev_vdbg(&intf->dev, "%s: cnt %d -> %d\n", dev_vdbg(&intf->dev, "%s: cnt %d -> %d\n",
__func__, atomic_read(&intf->dev.power.usage_count), __func__, atomic_read(&intf->dev.power.usage_count),
...@@ -1684,7 +1677,6 @@ void usb_autopm_put_interface_no_suspend(struct usb_interface *intf) ...@@ -1684,7 +1677,6 @@ void usb_autopm_put_interface_no_suspend(struct usb_interface *intf)
struct usb_device *udev = interface_to_usbdev(intf); struct usb_device *udev = interface_to_usbdev(intf);
usb_mark_last_busy(udev); usb_mark_last_busy(udev);
atomic_dec(&intf->pm_usage_cnt);
pm_runtime_put_noidle(&intf->dev); pm_runtime_put_noidle(&intf->dev);
} }
EXPORT_SYMBOL_GPL(usb_autopm_put_interface_no_suspend); EXPORT_SYMBOL_GPL(usb_autopm_put_interface_no_suspend);
...@@ -1715,8 +1707,6 @@ int usb_autopm_get_interface(struct usb_interface *intf) ...@@ -1715,8 +1707,6 @@ int usb_autopm_get_interface(struct usb_interface *intf)
status = pm_runtime_get_sync(&intf->dev); status = pm_runtime_get_sync(&intf->dev);
if (status < 0) if (status < 0)
pm_runtime_put_sync(&intf->dev); pm_runtime_put_sync(&intf->dev);
else
atomic_inc(&intf->pm_usage_cnt);
dev_vdbg(&intf->dev, "%s: cnt %d -> %d\n", dev_vdbg(&intf->dev, "%s: cnt %d -> %d\n",
__func__, atomic_read(&intf->dev.power.usage_count), __func__, atomic_read(&intf->dev.power.usage_count),
status); status);
...@@ -1750,8 +1740,6 @@ int usb_autopm_get_interface_async(struct usb_interface *intf) ...@@ -1750,8 +1740,6 @@ int usb_autopm_get_interface_async(struct usb_interface *intf)
status = pm_runtime_get(&intf->dev); status = pm_runtime_get(&intf->dev);
if (status < 0 && status != -EINPROGRESS) if (status < 0 && status != -EINPROGRESS)
pm_runtime_put_noidle(&intf->dev); pm_runtime_put_noidle(&intf->dev);
else
atomic_inc(&intf->pm_usage_cnt);
dev_vdbg(&intf->dev, "%s: cnt %d -> %d\n", dev_vdbg(&intf->dev, "%s: cnt %d -> %d\n",
__func__, atomic_read(&intf->dev.power.usage_count), __func__, atomic_read(&intf->dev.power.usage_count),
status); status);
...@@ -1775,7 +1763,6 @@ void usb_autopm_get_interface_no_resume(struct usb_interface *intf) ...@@ -1775,7 +1763,6 @@ void usb_autopm_get_interface_no_resume(struct usb_interface *intf)
struct usb_device *udev = interface_to_usbdev(intf); struct usb_device *udev = interface_to_usbdev(intf);
usb_mark_last_busy(udev); usb_mark_last_busy(udev);
atomic_inc(&intf->pm_usage_cnt);
pm_runtime_get_noresume(&intf->dev); pm_runtime_get_noresume(&intf->dev);
} }
EXPORT_SYMBOL_GPL(usb_autopm_get_interface_no_resume); EXPORT_SYMBOL_GPL(usb_autopm_get_interface_no_resume);
......
...@@ -763,18 +763,16 @@ static void rts51x_suspend_timer_fn(struct timer_list *t) ...@@ -763,18 +763,16 @@ static void rts51x_suspend_timer_fn(struct timer_list *t)
break; break;
case RTS51X_STAT_IDLE: case RTS51X_STAT_IDLE:
case RTS51X_STAT_SS: case RTS51X_STAT_SS:
usb_stor_dbg(us, "RTS51X_STAT_SS, intf->pm_usage_cnt:%d, power.usage:%d\n", usb_stor_dbg(us, "RTS51X_STAT_SS, power.usage:%d\n",
atomic_read(&us->pusb_intf->pm_usage_cnt),
atomic_read(&us->pusb_intf->dev.power.usage_count)); atomic_read(&us->pusb_intf->dev.power.usage_count));
if (atomic_read(&us->pusb_intf->pm_usage_cnt) > 0) { if (atomic_read(&us->pusb_intf->dev.power.usage_count) > 0) {
usb_stor_dbg(us, "Ready to enter SS state\n"); usb_stor_dbg(us, "Ready to enter SS state\n");
rts51x_set_stat(chip, RTS51X_STAT_SS); rts51x_set_stat(chip, RTS51X_STAT_SS);
/* ignore mass storage interface's children */ /* ignore mass storage interface's children */
pm_suspend_ignore_children(&us->pusb_intf->dev, true); pm_suspend_ignore_children(&us->pusb_intf->dev, true);
usb_autopm_put_interface_async(us->pusb_intf); usb_autopm_put_interface_async(us->pusb_intf);
usb_stor_dbg(us, "RTS51X_STAT_SS 01, intf->pm_usage_cnt:%d, power.usage:%d\n", usb_stor_dbg(us, "RTS51X_STAT_SS 01, power.usage:%d\n",
atomic_read(&us->pusb_intf->pm_usage_cnt),
atomic_read(&us->pusb_intf->dev.power.usage_count)); atomic_read(&us->pusb_intf->dev.power.usage_count));
} }
break; break;
...@@ -807,11 +805,10 @@ static void rts51x_invoke_transport(struct scsi_cmnd *srb, struct us_data *us) ...@@ -807,11 +805,10 @@ static void rts51x_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)
int ret; int ret;
if (working_scsi(srb)) { if (working_scsi(srb)) {
usb_stor_dbg(us, "working scsi, intf->pm_usage_cnt:%d, power.usage:%d\n", usb_stor_dbg(us, "working scsi, power.usage:%d\n",
atomic_read(&us->pusb_intf->pm_usage_cnt),
atomic_read(&us->pusb_intf->dev.power.usage_count)); atomic_read(&us->pusb_intf->dev.power.usage_count));
if (atomic_read(&us->pusb_intf->pm_usage_cnt) <= 0) { if (atomic_read(&us->pusb_intf->dev.power.usage_count) <= 0) {
ret = usb_autopm_get_interface(us->pusb_intf); ret = usb_autopm_get_interface(us->pusb_intf);
usb_stor_dbg(us, "working scsi, ret=%d\n", ret); usb_stor_dbg(us, "working scsi, ret=%d\n", ret);
} }
......
...@@ -200,7 +200,6 @@ usb_find_last_int_out_endpoint(struct usb_host_interface *alt, ...@@ -200,7 +200,6 @@ usb_find_last_int_out_endpoint(struct usb_host_interface *alt,
* @dev: driver model's view of this device * @dev: driver model's view of this device
* @usb_dev: if an interface is bound to the USB major, this will point * @usb_dev: if an interface is bound to the USB major, this will point
* to the sysfs representation for that device. * to the sysfs representation for that device.
* @pm_usage_cnt: PM usage counter for this interface
* @reset_ws: Used for scheduling resets from atomic context. * @reset_ws: Used for scheduling resets from atomic context.
* @resetting_device: USB core reset the device, so use alt setting 0 as * @resetting_device: USB core reset the device, so use alt setting 0 as
* current; needs bandwidth alloc after reset. * current; needs bandwidth alloc after reset.
...@@ -257,7 +256,6 @@ struct usb_interface { ...@@ -257,7 +256,6 @@ struct usb_interface {
struct device dev; /* interface specific device info */ struct device dev; /* interface specific device info */
struct device *usb_dev; struct device *usb_dev;
atomic_t pm_usage_cnt; /* usage counter for autosuspend */
struct work_struct reset_ws; /* for resets in atomic context */ struct work_struct reset_ws; /* for resets in atomic context */
}; };
#define to_usb_interface(d) container_of(d, struct usb_interface, dev) #define to_usb_interface(d) container_of(d, struct usb_interface, dev)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment