| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20260130074746.287750-1-guanyulin@google.com>
Date: Fri, 30 Jan 2026 07:47:46 +0000
From: Guan-Yu Lin <guanyulin@...gle.com>
To: gregkh@...uxfoundation.org, mathias.nyman@...el.com,
stern@...land.harvard.edu, wesley.cheng@....qualcomm.com
Cc: linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
Guan-Yu Lin <guanyulin@...gle.com>, stable@...r.kernel.org
Subject: [RFC PATCH] usb: host: xhci-sideband: fix deadlock in unregister path
When a USB device is disconnected or a driver is unbound, the USB core
invokes the driver's disconnect callback while holding the udev device
lock. If the driver calls xhci_sideband_unregister(), it eventually
reaches usb_offload_put(), which attempts to acquire the same udev
lock, resulting in a self-deadlock.
Introduce lockless variants __usb_offload_get() and __usb_offload_put()
to allow modifying the offload usage count when the device lock is
already held. These helpers use device_lock_assert() to ensure callers
meet the locking requirements.
Update the xHCI sideband implementation to use these lockless variants
in the unregister and interrupter removal paths. For public sideband
APIs that can be called from other contexts, wrap the calls with
explicit udev locking to maintain synchronization.
Cc: stable@...r.kernel.org
Fixes: ef82a4803aab ("xhci: sideband: add api to trace sideband usage")
Signed-off-by: Guan-Yu Lin <guanyulin@...gle.com>
---
Hi Maintainers,
In the current implementation, xhci_sideband_unregister() is only invoked
when a driver manages a USB interface via snd_usb_platform_ops callbacks
(.connect_cb and .disconnect_cb). In these contexts, the parent USB
device lock is already held by the USB core. However, the original design
of usb_offload_put() attempts to re-acquire the same USB device lock,
leading to a recursive locking scenario.
This patch resolves the issue by:
1. Refactoring Lock Ownership: Shifting lock acquisition responsibility
to the upper-level USB driver. This ensures usb_offload_put() can be
safely called from contexts where the lock is already held.
2. Standardizing the Lock Hierarchy: To prevent potential ABBA deadlocks,
the locking sequence is unified across the driver. The functions
xhci_sideband_create_interrupter() and xhci_sideband_remove_interrupter()
have been updated to strictly follow the same hierarchy.
---
drivers/usb/core/offload.c | 92 +++++++++++++++++++++-----------
drivers/usb/host/xhci-sideband.c | 48 +++++++++++------
include/linux/usb.h | 6 +++
3 files changed, 100 insertions(+), 46 deletions(-)
diff --git a/drivers/usb/core/offload.c b/drivers/usb/core/offload.c
index 7c699f1b8d2b..bc5f337fb355 100644
--- a/drivers/usb/core/offload.c
+++ b/drivers/usb/core/offload.c
@@ -13,44 +13,59 @@
#include "usb.h"
/**
- * usb_offload_get - increment the offload_usage of a USB device
+ * __usb_offload_get - increment the offload_usage of a USB device
* @udev: the USB device to increment its offload_usage
*
- * Incrementing the offload_usage of a usb_device indicates that offload is
- * enabled on this usb_device; that is, another entity is actively handling USB
- * transfers. This information allows the USB driver to adjust its power
- * management policy based on offload activity.
+ * This is the lockless version of usb_offload_get. The caller must hold
+ * @udev's device lock.
*
* Return: 0 on success. A negative error code otherwise.
*/
-int usb_offload_get(struct usb_device *udev)
+int __usb_offload_get(struct usb_device *udev)
{
int ret;
- usb_lock_device(udev);
- if (udev->state == USB_STATE_NOTATTACHED) {
- usb_unlock_device(udev);
+ device_lock_assert(&udev->dev);
+
+ if (udev->state == USB_STATE_NOTATTACHED)
return -ENODEV;
- }
if (udev->state == USB_STATE_SUSPENDED ||
- udev->offload_at_suspend) {
- usb_unlock_device(udev);
+ udev->offload_at_suspend)
return -EBUSY;
- }
/*
* offload_usage could only be modified when the device is active, since
* it will alter the suspend flow of the device.
*/
ret = usb_autoresume_device(udev);
- if (ret < 0) {
- usb_unlock_device(udev);
+ if (ret < 0)
return ret;
- }
udev->offload_usage++;
usb_autosuspend_device(udev);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(__usb_offload_get);
+
+/**
+ * usb_offload_get - increment the offload_usage of a USB device
+ * @udev: the USB device to increment its offload_usage
+ *
+ * Incrementing the offload_usage of a usb_device indicates that offload is
+ * enabled on this usb_device; that is, another entity is actively handling USB
+ * transfers. This information allows the USB driver to adjust its power
+ * management policy based on offload activity.
+ *
+ * Return: 0 on success. A negative error code otherwise.
+ */
+int usb_offload_get(struct usb_device *udev)
+{
+ int ret;
+
+ usb_lock_device(udev);
+ ret = __usb_offload_get(udev);
usb_unlock_device(udev);
return ret;
@@ -58,45 +73,60 @@ int usb_offload_get(struct usb_device *udev)
EXPORT_SYMBOL_GPL(usb_offload_get);
/**
- * usb_offload_put - drop the offload_usage of a USB device
+ * __usb_offload_put - drop the offload_usage of a USB device
* @udev: the USB device to drop its offload_usage
*
- * The inverse operation of usb_offload_get, which drops the offload_usage of
- * a USB device. This information allows the USB driver to adjust its power
- * management policy based on offload activity.
+ * This is the lockless version of usb_offload_put. The caller must hold
+ * @udev's device lock.
*
* Return: 0 on success. A negative error code otherwise.
*/
-int usb_offload_put(struct usb_device *udev)
+int __usb_offload_put(struct usb_device *udev)
{
int ret;
- usb_lock_device(udev);
- if (udev->state == USB_STATE_NOTATTACHED) {
- usb_unlock_device(udev);
+ device_lock_assert(&udev->dev);
+
+ if (udev->state == USB_STATE_NOTATTACHED)
return -ENODEV;
- }
if (udev->state == USB_STATE_SUSPENDED ||
- udev->offload_at_suspend) {
- usb_unlock_device(udev);
+ udev->offload_at_suspend)
return -EBUSY;
- }
/*
* offload_usage could only be modified when the device is active, since
* it will alter the suspend flow of the device.
*/
ret = usb_autoresume_device(udev);
- if (ret < 0) {
- usb_unlock_device(udev);
+ if (ret < 0)
return ret;
- }
/* Drop the count when it wasn't 0, ignore the operation otherwise. */
if (udev->offload_usage)
udev->offload_usage--;
usb_autosuspend_device(udev);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(__usb_offload_put);
+
+/**
+ * usb_offload_put - drop the offload_usage of a USB device
+ * @udev: the USB device to drop its offload_usage
+ *
+ * The inverse operation of usb_offload_get, which drops the offload_usage of
+ * a USB device. This information allows the USB driver to adjust its power
+ * management policy based on offload activity.
+ *
+ * Return: 0 on success. A negative error code otherwise.
+ */
+int usb_offload_put(struct usb_device *udev)
+{
+ int ret;
+
+ usb_lock_device(udev);
+ ret = __usb_offload_put(udev);
usb_unlock_device(udev);
return ret;
diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index 2bd77255032b..c37c3f3adf4a 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -89,7 +89,7 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
sb->eps[ep->ep_index] = NULL;
}
-/* Caller must hold sb->mutex */
+/* Caller must hold sb->mutex and udev device lock */
static void
__xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
{
@@ -102,10 +102,10 @@ __xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
xhci_remove_secondary_interrupter(xhci_to_hcd(sb->xhci), sb->ir);
sb->ir = NULL;
- udev = sb->vdev->udev;
- if (udev->state != USB_STATE_NOTATTACHED)
- usb_offload_put(udev);
+ udev = interface_to_usbdev(sb->intf);
+ device_lock_assert(&udev->dev);
+ __usb_offload_put(udev);
}
/* sideband api functions */
@@ -334,25 +334,36 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
if (!sb || !sb->xhci)
return -ENODEV;
- guard(mutex)(&sb->mutex);
+ udev = interface_to_usbdev(sb->intf);
+ usb_lock_device(udev);
+ mutex_lock(&sb->mutex);
- if (!sb->vdev)
- return -ENODEV;
+ if (!sb->vdev) {
+ ret = -ENODEV;
+ goto unlock;
+ }
- if (sb->ir)
- return -EBUSY;
+ if (sb->ir) {
+ ret = -EBUSY;
+ goto unlock;
+ }
sb->ir = xhci_create_secondary_interrupter(xhci_to_hcd(sb->xhci),
num_seg, imod_interval,
intr_num);
- if (!sb->ir)
- return -ENOMEM;
+ if (!sb->ir) {
+ ret = -ENOMEM;
+ goto unlock;
+ }
- udev = sb->vdev->udev;
- ret = usb_offload_get(udev);
+ ret = __usb_offload_get(udev);
sb->ir->ip_autoclear = ip_autoclear;
+unlock:
+ mutex_unlock(&sb->mutex);
+ usb_unlock_device(udev);
+
return ret;
}
EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
@@ -367,12 +378,19 @@ EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
void
xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
{
- if (!sb)
+ struct usb_device *udev;
+
+ if (!sb || !sb->vdev)
return;
- guard(mutex)(&sb->mutex);
+ udev = interface_to_usbdev(sb->intf);
+ usb_lock_device(udev);
+ mutex_lock(&sb->mutex);
__xhci_sideband_remove_interrupter(sb);
+
+ mutex_unlock(&sb->mutex);
+ usb_unlock_device(udev);
}
EXPORT_SYMBOL_GPL(xhci_sideband_remove_interrupter);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index fbfcc70b07fb..19c84c05f376 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -846,11 +846,17 @@ static inline void usb_mark_last_busy(struct usb_device *udev)
#endif
#if IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND)
+int __usb_offload_get(struct usb_device *udev);
+int __usb_offload_put(struct usb_device *udev);
int usb_offload_get(struct usb_device *udev);
int usb_offload_put(struct usb_device *udev);
bool usb_offload_check(struct usb_device *udev);
#else
+static inline int __usb_offload_get(struct usb_device *udev)
+{ return 0; }
+static inline int __usb_offload_put(struct usb_device *udev)
+{ return 0; }
static inline int usb_offload_get(struct usb_device *udev)
{ return 0; }
static inline int usb_offload_put(struct usb_device *udev)
--
2.53.0.rc1.225.gd81095ad13-goog
Powered by blists - more mailing lists