A kernel panic was observed due to a race condition between un-registering
sideband and creating sideband interrupters. The issue occurrs when thread
T1 runs uaudio_disconnect() and released sb->xhci via sideband_unregister,
while thread T2 simultaneously accessed the now-NULL sb->xhci in
xhci_sideband_create_interrupter() resulting in a crash.

By locking the mutex before modifying sb->xhci, any thread calling
xhci_sideband_create_interrupter() will either see a valid sb->xhci or wait
until xhci_sideband_unregister() completes.

Signed-off-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@oss.qualcomm.com>
---
 drivers/usb/host/xhci-sideband.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index e771a476fef2..74a58f759cee 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -481,10 +481,12 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 
 	xhci_sideband_remove_interrupter(sb);
 
+	mutex_lock(&sb->mutex);
 	spin_lock_irq(&xhci->lock);
 	sb->xhci = NULL;
 	sb->vdev->sideband = NULL;
 	spin_unlock_irq(&xhci->lock);
+	mutex_unlock(&sb->mutex);
 
 	kfree(sb);
 }
-- 
2.17.1

On 10/28/25 10:00, Uttkarsh Aggarwal wrote:
> A kernel panic was observed due to a race condition between un-registering
> sideband and creating sideband interrupters. The issue occurrs when thread
> T1 runs uaudio_disconnect() and released sb->xhci via sideband_unregister,
> while thread T2 simultaneously accessed the now-NULL sb->xhci in
> xhci_sideband_create_interrupter() resulting in a crash.
> 
> By locking the mutex before modifying sb->xhci, any thread calling
> xhci_sideband_create_interrupter() will either see a valid sb->xhci or wait
> until xhci_sideband_unregister() completes.
> 

Looks like there is a bigger issue with xhci_sideband_unregister() and the mutex.
New endpoints and interrupter can be added to the sideband after
xhci_sideband_unregister() cleared the existing ones, and released the mutex.

We should avoid taking and releasing the mutex several times in unregister,
and make sure we set a flag during first time unregister takes the muxtex, and make
sure no new endpoints and interrupter are added if this flag is set.
Also avoid creating unnecessary locking dependencies between mutex and xhci spinlock.

the xhci->lock looks correct

Maybe we can use sb->vdev as a flag, I'll look into this.

Thanks
Mathias

Uttkarsh Aggarwal observed a kernel panic during sideband un-register
and found it was caused by a race condition between sideband unregister,
and creating sideband interrupters.
The issue occurrs when thread T1 runs uaudio_disconnect() and released
sb->xhci via sideband_unregister, while thread T2 simultaneously accessed
the now-NULL sb->xhci in xhci_sideband_create_interrupter() resulting in
a crash.

Ensure new endpoints or interrupter can't be added to a sidenband after
xhci_sideband_unregister() cleared the existing ones, and unlocked the
sideband mutex.
Reorganise code so that mutex is only taken and released once in
xhci_sideband_unregister(), and clear sb->vdev while mutex is taken.

Refuse to add endpoints or interrupter if sb->vdev is not set.
sb->vdev is set when sideband is created and registered.

Reported-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@oss.qualcomm.com>
Closes: https://lore.kernel.org/linux-usb/20251028080043.27760-1-uttkarsh.aggarwal@oss.qualcomm.com
Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-sideband.c | 55 ++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index e771a476fef2..c308be9a8e9f 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -86,6 +86,22 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
 	sb->eps[ep->ep_index] = NULL;
 }
 
+static void
+__xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
+{
+	struct usb_device *udev;
+
+	if (!sb->ir)
+		return;
+
+	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);
+}
+
 /* sideband api functions */
 
 /**
@@ -132,6 +148,12 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
 	unsigned int ep_index;
 
 	mutex_lock(&sb->mutex);
+
+	if (!sb->vdev) {
+		mutex_unlock(&sb->mutex);
+		return -ENODEV;
+	}
+
 	ep_index = xhci_get_endpoint_index(&host_ep->desc);
 	ep = &sb->vdev->eps[ep_index];
 
@@ -317,6 +339,12 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
 		return -ENODEV;
 
 	mutex_lock(&sb->mutex);
+
+	if (!sb->vdev) {
+		ret = -ENODEV;
+		goto out;
+	}
+
 	if (sb->ir) {
 		ret = -EBUSY;
 		goto out;
@@ -352,20 +380,11 @@ EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
 void
 xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
 {
-	struct usb_device *udev;
-
-	if (!sb || !sb->ir)
+	if (!sb)
 		return;
 
 	mutex_lock(&sb->mutex);
-	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);
-
+	__xhci_sideband_remove_interrupter(sb);
 	mutex_unlock(&sb->mutex);
 }
 EXPORT_SYMBOL_GPL(xhci_sideband_remove_interrupter);
@@ -465,6 +484,7 @@ EXPORT_SYMBOL_GPL(xhci_sideband_register);
 void
 xhci_sideband_unregister(struct xhci_sideband *sb)
 {
+	struct xhci_virt_device *vdev;
 	struct xhci_hcd *xhci;
 	int i;
 
@@ -474,16 +494,23 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 	xhci = sb->xhci;
 
 	mutex_lock(&sb->mutex);
+
+	vdev = sb->vdev;
+	if (!vdev)
+		return;
+
 	for (i = 0; i < EP_CTX_PER_DEV; i++)
 		if (sb->eps[i])
 			__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
-	mutex_unlock(&sb->mutex);
 
-	xhci_sideband_remove_interrupter(sb);
+	__xhci_sideband_remove_interrupter(sb);
+
+	sb->vdev = NULL;
+	mutex_unlock(&sb->mutex);
 
 	spin_lock_irq(&xhci->lock);
 	sb->xhci = NULL;
-	sb->vdev->sideband = NULL;
+	vdev->sideband = NULL;
 	spin_unlock_irq(&xhci->lock);
 
 	kfree(sb);
-- 
2.43.0

On Tue, Oct 28, 2025 at 03:44:51PM +0200, Mathias Nyman wrote:
> Uttkarsh Aggarwal observed a kernel panic during sideband un-register
> and found it was caused by a race condition between sideband unregister,
> and creating sideband interrupters.
> The issue occurrs when thread T1 runs uaudio_disconnect() and released
> sb->xhci via sideband_unregister, while thread T2 simultaneously accessed
> the now-NULL sb->xhci in xhci_sideband_create_interrupter() resulting in
> a crash.
> 
> Ensure new endpoints or interrupter can't be added to a sidenband after
> xhci_sideband_unregister() cleared the existing ones, and unlocked the
> sideband mutex.
> Reorganise code so that mutex is only taken and released once in
> xhci_sideband_unregister(), and clear sb->vdev while mutex is taken.
> 
> Refuse to add endpoints or interrupter if sb->vdev is not set.
> sb->vdev is set when sideband is created and registered.
> 
> Reported-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@oss.qualcomm.com>
> Closes: https://lore.kernel.org/linux-usb/20251028080043.27760-1-uttkarsh.aggarwal@oss.qualcomm.com
> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-sideband.c | 55 ++++++++++++++++++++++++--------
>  1 file changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index e771a476fef2..c308be9a8e9f 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -86,6 +86,22 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
>  	sb->eps[ep->ep_index] = NULL;
>  }
>  
> +static void
> +__xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
> +{
> +	struct usb_device *udev;
> +
> +	if (!sb->ir)
> +		return;
> +
> +	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);
> +}
> +
>  /* sideband api functions */
>  
>  /**
> @@ -132,6 +148,12 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
>  	unsigned int ep_index;
>  
>  	mutex_lock(&sb->mutex);
> +
> +	if (!sb->vdev) {
> +		mutex_unlock(&sb->mutex);
> +		return -ENODEV;
> +	}
> +
>  	ep_index = xhci_get_endpoint_index(&host_ep->desc);
>  	ep = &sb->vdev->eps[ep_index];
>  
> @@ -317,6 +339,12 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
>  		return -ENODEV;
>  
>  	mutex_lock(&sb->mutex);
> +
> +	if (!sb->vdev) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
>  	if (sb->ir) {
>  		ret = -EBUSY;
>  		goto out;
> @@ -352,20 +380,11 @@ EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
>  void
>  xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
>  {
> -	struct usb_device *udev;
> -
> -	if (!sb || !sb->ir)
> +	if (!sb)
>  		return;
>  
>  	mutex_lock(&sb->mutex);
> -	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);
> -
> +	__xhci_sideband_remove_interrupter(sb);
>  	mutex_unlock(&sb->mutex);
>  }
>  EXPORT_SYMBOL_GPL(xhci_sideband_remove_interrupter);
> @@ -465,6 +484,7 @@ EXPORT_SYMBOL_GPL(xhci_sideband_register);
>  void
>  xhci_sideband_unregister(struct xhci_sideband *sb)
>  {
> +	struct xhci_virt_device *vdev;
>  	struct xhci_hcd *xhci;
>  	int i;
>  
> @@ -474,16 +494,23 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>  	xhci = sb->xhci;
>  
>  	mutex_lock(&sb->mutex);
> +
> +	vdev = sb->vdev;
> +	if (!vdev)
> +		return;

Lock is still held :(

I think you need to use guard() to make this more sane.

thanks,

greg k-h

On 10/28/25 15:56, Greg KH wrote:

>> @@ -474,16 +494,23 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>>   	xhci = sb->xhci;
>>   
>>   	mutex_lock(&sb->mutex);
>> +
>> +	vdev = sb->vdev;
>> +	if (!vdev)
>> +		return;
> 
> Lock is still held :(
> 

Oops, missed one

> I think you need to use guard() to make this more sane.

Clearly yes, I'll send a v2

> 
> thanks,
> 
> greg k-h
> 

Thanks
Mathias

Uttkarsh Aggarwal observed a kernel panic during sideband un-register
and found it was caused by a race condition between sideband unregister,
and creating sideband interrupters.
The issue occurrs when thread T1 runs uaudio_disconnect() and released
sb->xhci via sideband_unregister, while thread T2 simultaneously accessed
the now-NULL sb->xhci in xhci_sideband_create_interrupter() resulting in
a crash.

Ensure new endpoints or interrupter can't be added to a sidenband after
xhci_sideband_unregister() cleared the existing ones, and unlocked the
sideband mutex.
Reorganize code so that mutex is only taken and released once in
xhci_sideband_unregister(), and clear sb->vdev while mutex is taken.

Use mutex guards to reduce human unlock errors in code

Refuse to add endpoints or interrupter if sb->vdev is not set.
sb->vdev is set when sideband is created and registered.

Reported-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@oss.qualcomm.com>
Closes: https://lore.kernel.org/linux-usb/20251028080043.27760-1-uttkarsh.aggarwal@oss.qualcomm.com
Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---

v2:
  use guard() and fix missing mutex_unlock as recommended by greg k-h 

---
 drivers/usb/host/xhci-sideband.c | 97 +++++++++++++++++---------------
 1 file changed, 53 insertions(+), 44 deletions(-)

diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index e771a476fef2..2daa0ba7ad9a 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -86,6 +86,22 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
 	sb->eps[ep->ep_index] = NULL;
 }
 
+static void
+__xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
+{
+	struct usb_device *udev;
+
+	if (!sb->ir)
+		return;
+
+	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);
+}
+
 /* sideband api functions */
 
 /**
@@ -131,14 +147,17 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
 	struct xhci_virt_ep *ep;
 	unsigned int ep_index;
 
-	mutex_lock(&sb->mutex);
+	guard(mutex)(&sb->mutex);
+
+	if (!sb->vdev)
+		return -ENODEV;
+
 	ep_index = xhci_get_endpoint_index(&host_ep->desc);
 	ep = &sb->vdev->eps[ep_index];
 
-	if (ep->ep_state & EP_HAS_STREAMS) {
-		mutex_unlock(&sb->mutex);
+	if (ep->ep_state & EP_HAS_STREAMS)
 		return -EINVAL;
-	}
+
 
 	/*
 	 * Note, we don't know the DMA mask of the audio DSP device, if its
@@ -148,14 +167,11 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
 	 * and let this function add the endpoint and allocate the ring buffer
 	 * with the smallest common DMA mask
 	 */
-	if (sb->eps[ep_index] || ep->sideband) {
-		mutex_unlock(&sb->mutex);
+	if (sb->eps[ep_index] || ep->sideband)
 		return -EBUSY;
-	}
 
 	ep->sideband = sb;
 	sb->eps[ep_index] = ep;
-	mutex_unlock(&sb->mutex);
 
 	return 0;
 }
@@ -180,18 +196,16 @@ xhci_sideband_remove_endpoint(struct xhci_sideband *sb,
 	struct xhci_virt_ep *ep;
 	unsigned int ep_index;
 
-	mutex_lock(&sb->mutex);
+	guard(mutex)(&sb->mutex);
+
 	ep_index = xhci_get_endpoint_index(&host_ep->desc);
 	ep = sb->eps[ep_index];
 
-	if (!ep || !ep->sideband || ep->sideband != sb) {
-		mutex_unlock(&sb->mutex);
+	if (!ep || !ep->sideband || ep->sideband != sb)
 		return -ENODEV;
-	}
 
 	__xhci_sideband_remove_endpoint(sb, ep);
 	xhci_initialize_ring_info(ep->ring);
-	mutex_unlock(&sb->mutex);
 
 	return 0;
 }
@@ -316,28 +330,25 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
 	if (!sb || !sb->xhci)
 		return -ENODEV;
 
-	mutex_lock(&sb->mutex);
-	if (sb->ir) {
-		ret = -EBUSY;
-		goto out;
-	}
+	guard(mutex)(&sb->mutex);
+
+	if (!sb->vdev)
+		return -ENODEV;
+
+	if (sb->ir)
+		return -EBUSY;
 
 	sb->ir = xhci_create_secondary_interrupter(xhci_to_hcd(sb->xhci),
 						   num_seg, imod_interval,
 						   intr_num);
-	if (!sb->ir) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!sb->ir)
+		return -ENOMEM;
 
 	udev = sb->vdev->udev;
 	ret = usb_offload_get(udev);
 
 	sb->ir->ip_autoclear = ip_autoclear;
 
-out:
-	mutex_unlock(&sb->mutex);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
@@ -352,21 +363,12 @@ EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
 void
 xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
 {
-	struct usb_device *udev;
-
-	if (!sb || !sb->ir)
+	if (!sb)
 		return;
 
-	mutex_lock(&sb->mutex);
-	xhci_remove_secondary_interrupter(xhci_to_hcd(sb->xhci), sb->ir);
-
-	sb->ir = NULL;
-	udev = sb->vdev->udev;
+	guard(mutex)(&sb->mutex);
 
-	if (udev->state != USB_STATE_NOTATTACHED)
-		usb_offload_put(udev);
-
-	mutex_unlock(&sb->mutex);
+	__xhci_sideband_remove_interrupter(sb);
 }
 EXPORT_SYMBOL_GPL(xhci_sideband_remove_interrupter);
 
@@ -465,6 +467,7 @@ EXPORT_SYMBOL_GPL(xhci_sideband_register);
 void
 xhci_sideband_unregister(struct xhci_sideband *sb)
 {
+	struct xhci_virt_device *vdev;
 	struct xhci_hcd *xhci;
 	int i;
 
@@ -473,17 +476,23 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 
 	xhci = sb->xhci;
 
-	mutex_lock(&sb->mutex);
-	for (i = 0; i < EP_CTX_PER_DEV; i++)
-		if (sb->eps[i])
-			__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
-	mutex_unlock(&sb->mutex);
+	scoped_guard(mutex, &sb->mutex) {
+		vdev = sb->vdev;
+		if (!vdev)
+			return;
+
+		for (i = 0; i < EP_CTX_PER_DEV; i++)
+			if (sb->eps[i])
+				__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
 
-	xhci_sideband_remove_interrupter(sb);
+		__xhci_sideband_remove_interrupter(sb);
+
+		sb->vdev = NULL;
+	}
 
 	spin_lock_irq(&xhci->lock);
 	sb->xhci = NULL;
-	sb->vdev->sideband = NULL;
+	vdev->sideband = NULL;
 	spin_unlock_irq(&xhci->lock);
 
 	kfree(sb);
-- 
2.43.0

On Tue, Oct 28, 2025 at 06:51:53PM +0200, Mathias Nyman wrote:
> Uttkarsh Aggarwal observed a kernel panic during sideband un-register
> and found it was caused by a race condition between sideband unregister,
> and creating sideband interrupters.
> The issue occurrs when thread T1 runs uaudio_disconnect() and released
> sb->xhci via sideband_unregister, while thread T2 simultaneously accessed
> the now-NULL sb->xhci in xhci_sideband_create_interrupter() resulting in
> a crash.
> 
> Ensure new endpoints or interrupter can't be added to a sidenband after
> xhci_sideband_unregister() cleared the existing ones, and unlocked the
> sideband mutex.
> Reorganize code so that mutex is only taken and released once in
> xhci_sideband_unregister(), and clear sb->vdev while mutex is taken.
> 
> Use mutex guards to reduce human unlock errors in code
> 
> Refuse to add endpoints or interrupter if sb->vdev is not set.
> sb->vdev is set when sideband is created and registered.
> 
> Reported-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@oss.qualcomm.com>
> Closes: https://lore.kernel.org/linux-usb/20251028080043.27760-1-uttkarsh.aggarwal@oss.qualcomm.com
> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
> 
> v2:
>   use guard() and fix missing mutex_unlock as recommended by greg k-h 
> 
> ---
>  drivers/usb/host/xhci-sideband.c | 97 +++++++++++++++++---------------
>  1 file changed, 53 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index e771a476fef2..2daa0ba7ad9a 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -86,6 +86,22 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
>  	sb->eps[ep->ep_index] = NULL;
>  }
>  
> +static void
> +__xhci_sideband_remove_interrupter(struct xhci_sideband *sb)

This function must be called with the mutex locked, so shouldn't you
document that here so that the compiler can catch it if we mess up in
the future and forget to grab it?

> +{
> +	struct usb_device *udev;
> +
> +	if (!sb->ir)
> +		return;
> +
> +	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);
> +}
> +
>  /* sideband api functions */
>  
>  /**
> @@ -131,14 +147,17 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
>  	struct xhci_virt_ep *ep;
>  	unsigned int ep_index;
>  
> -	mutex_lock(&sb->mutex);
> +	guard(mutex)(&sb->mutex);
> +
> +	if (!sb->vdev)
> +		return -ENODEV;
> +
>  	ep_index = xhci_get_endpoint_index(&host_ep->desc);
>  	ep = &sb->vdev->eps[ep_index];
>  
> -	if (ep->ep_state & EP_HAS_STREAMS) {
> -		mutex_unlock(&sb->mutex);
> +	if (ep->ep_state & EP_HAS_STREAMS)
>  		return -EINVAL;
> -	}
> +

Very minor nit, just delete the extra line, like you did in the rest of
the diff below here, otherwise you have 2 blank lines.

thanks,

greg k-h

Uttkarsh Aggarwal observed a kernel panic during sideband un-register
and found it was caused by a race condition between sideband unregister,
and creating sideband interrupters.
The issue occurrs when thread T1 runs uaudio_disconnect() and released
sb->xhci via sideband_unregister, while thread T2 simultaneously accessed
the now-NULL sb->xhci in xhci_sideband_create_interrupter() resulting in
a crash.

Ensure new endpoints or interrupter can't be added to a sidenband after
xhci_sideband_unregister() cleared the existing ones, and unlocked the
sideband mutex.
Reorganize code so that mutex is only taken and released once in
xhci_sideband_unregister(), and clear sb->vdev while mutex is taken.

Use mutex guards to reduce human unlock errors in code

Refuse to add endpoints or interrupter if sb->vdev is not set.
sb->vdev is set when sideband is created and registered.

Reported-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@oss.qualcomm.com>
Closes: https://lore.kernel.org/linux-usb/20251028080043.27760-1-uttkarsh.aggarwal@oss.qualcomm.com
Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---

v3:
  Remove extra blank line
  Add lockdep_assert_held() and comment for functions where caller must hold mutex

v2:
  use guard() and fix missing mutex_unlock as recommended by greg k-h

---
 drivers/usb/host/xhci-sideband.c | 102 ++++++++++++++++++-------------
 1 file changed, 58 insertions(+), 44 deletions(-)

diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index e771a476fef2..a85f62a73313 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -73,9 +73,12 @@ xhci_ring_to_sgtable(struct xhci_sideband *sb, struct xhci_ring *ring)
 	return NULL;
 }
 
+/* Caller must hold sb->mutex */
 static void
 __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *ep)
 {
+	lockdep_assert_held(&sb->mutex);
+
 	/*
 	 * Issue a stop endpoint command when an endpoint is removed.
 	 * The stop ep cmd handler will handle the ring cleanup.
@@ -86,6 +89,25 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
 	sb->eps[ep->ep_index] = NULL;
 }
 
+/* Caller must hold sb->mutex */
+static void
+__xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
+{
+	struct usb_device *udev;
+
+	lockdep_assert_held(&sb->mutex);
+
+	if (!sb->ir)
+		return;
+
+	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);
+}
+
 /* sideband api functions */
 
 /**
@@ -131,14 +153,16 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
 	struct xhci_virt_ep *ep;
 	unsigned int ep_index;
 
-	mutex_lock(&sb->mutex);
+	guard(mutex)(&sb->mutex);
+
+	if (!sb->vdev)
+		return -ENODEV;
+
 	ep_index = xhci_get_endpoint_index(&host_ep->desc);
 	ep = &sb->vdev->eps[ep_index];
 
-	if (ep->ep_state & EP_HAS_STREAMS) {
-		mutex_unlock(&sb->mutex);
+	if (ep->ep_state & EP_HAS_STREAMS)
 		return -EINVAL;
-	}
 
 	/*
 	 * Note, we don't know the DMA mask of the audio DSP device, if its
@@ -148,14 +172,11 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
 	 * and let this function add the endpoint and allocate the ring buffer
 	 * with the smallest common DMA mask
 	 */
-	if (sb->eps[ep_index] || ep->sideband) {
-		mutex_unlock(&sb->mutex);
+	if (sb->eps[ep_index] || ep->sideband)
 		return -EBUSY;
-	}
 
 	ep->sideband = sb;
 	sb->eps[ep_index] = ep;
-	mutex_unlock(&sb->mutex);
 
 	return 0;
 }
@@ -180,18 +201,16 @@ xhci_sideband_remove_endpoint(struct xhci_sideband *sb,
 	struct xhci_virt_ep *ep;
 	unsigned int ep_index;
 
-	mutex_lock(&sb->mutex);
+	guard(mutex)(&sb->mutex);
+
 	ep_index = xhci_get_endpoint_index(&host_ep->desc);
 	ep = sb->eps[ep_index];
 
-	if (!ep || !ep->sideband || ep->sideband != sb) {
-		mutex_unlock(&sb->mutex);
+	if (!ep || !ep->sideband || ep->sideband != sb)
 		return -ENODEV;
-	}
 
 	__xhci_sideband_remove_endpoint(sb, ep);
 	xhci_initialize_ring_info(ep->ring);
-	mutex_unlock(&sb->mutex);
 
 	return 0;
 }
@@ -316,28 +335,25 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
 	if (!sb || !sb->xhci)
 		return -ENODEV;
 
-	mutex_lock(&sb->mutex);
-	if (sb->ir) {
-		ret = -EBUSY;
-		goto out;
-	}
+	guard(mutex)(&sb->mutex);
+
+	if (!sb->vdev)
+		return -ENODEV;
+
+	if (sb->ir)
+		return -EBUSY;
 
 	sb->ir = xhci_create_secondary_interrupter(xhci_to_hcd(sb->xhci),
 						   num_seg, imod_interval,
 						   intr_num);
-	if (!sb->ir) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!sb->ir)
+		return -ENOMEM;
 
 	udev = sb->vdev->udev;
 	ret = usb_offload_get(udev);
 
 	sb->ir->ip_autoclear = ip_autoclear;
 
-out:
-	mutex_unlock(&sb->mutex);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
@@ -352,21 +368,12 @@ EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
 void
 xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
 {
-	struct usb_device *udev;
-
-	if (!sb || !sb->ir)
+	if (!sb)
 		return;
 
-	mutex_lock(&sb->mutex);
-	xhci_remove_secondary_interrupter(xhci_to_hcd(sb->xhci), sb->ir);
-
-	sb->ir = NULL;
-	udev = sb->vdev->udev;
+	guard(mutex)(&sb->mutex);
 
-	if (udev->state != USB_STATE_NOTATTACHED)
-		usb_offload_put(udev);
-
-	mutex_unlock(&sb->mutex);
+	__xhci_sideband_remove_interrupter(sb);
 }
 EXPORT_SYMBOL_GPL(xhci_sideband_remove_interrupter);
 
@@ -465,6 +472,7 @@ EXPORT_SYMBOL_GPL(xhci_sideband_register);
 void
 xhci_sideband_unregister(struct xhci_sideband *sb)
 {
+	struct xhci_virt_device *vdev;
 	struct xhci_hcd *xhci;
 	int i;
 
@@ -473,17 +481,23 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 
 	xhci = sb->xhci;
 
-	mutex_lock(&sb->mutex);
-	for (i = 0; i < EP_CTX_PER_DEV; i++)
-		if (sb->eps[i])
-			__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
-	mutex_unlock(&sb->mutex);
+	scoped_guard(mutex, &sb->mutex) {
+		vdev = sb->vdev;
+		if (!vdev)
+			return;
+
+		for (i = 0; i < EP_CTX_PER_DEV; i++)
+			if (sb->eps[i])
+				__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
 
-	xhci_sideband_remove_interrupter(sb);
+		__xhci_sideband_remove_interrupter(sb);
+
+		sb->vdev = NULL;
+	}
 
 	spin_lock_irq(&xhci->lock);
 	sb->xhci = NULL;
-	sb->vdev->sideband = NULL;
+	vdev->sideband = NULL;
 	spin_unlock_irq(&xhci->lock);
 
 	kfree(sb);
-- 
2.43.0

On Wed, Oct 29, 2025 at 02:24:35PM +0200, Mathias Nyman wrote:
> Uttkarsh Aggarwal observed a kernel panic during sideband un-register
> and found it was caused by a race condition between sideband unregister,
> and creating sideband interrupters.
> The issue occurrs when thread T1 runs uaudio_disconnect() and released
> sb->xhci via sideband_unregister, while thread T2 simultaneously accessed
> the now-NULL sb->xhci in xhci_sideband_create_interrupter() resulting in
> a crash.
> 
> Ensure new endpoints or interrupter can't be added to a sidenband after
> xhci_sideband_unregister() cleared the existing ones, and unlocked the
> sideband mutex.
> Reorganize code so that mutex is only taken and released once in
> xhci_sideband_unregister(), and clear sb->vdev while mutex is taken.
> 
> Use mutex guards to reduce human unlock errors in code
> 
> Refuse to add endpoints or interrupter if sb->vdev is not set.
> sb->vdev is set when sideband is created and registered.
> 
> Reported-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@oss.qualcomm.com>
> Closes: https://lore.kernel.org/linux-usb/20251028080043.27760-1-uttkarsh.aggarwal@oss.qualcomm.com
> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---

Looks good, thanks for respinning this.  I don't know if it fixes the
issue, but it looks sane :)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

On 10/29/25 14:51, Greg KH wrote:
> On Wed, Oct 29, 2025 at 02:24:35PM +0200, Mathias Nyman wrote:
>> Uttkarsh Aggarwal observed a kernel panic during sideband un-register
>> and found it was caused by a race condition between sideband unregister,
>> and creating sideband interrupters.
>> The issue occurrs when thread T1 runs uaudio_disconnect() and released
>> sb->xhci via sideband_unregister, while thread T2 simultaneously accessed
>> the now-NULL sb->xhci in xhci_sideband_create_interrupter() resulting in
>> a crash.
>>
>> Ensure new endpoints or interrupter can't be added to a sidenband after
>> xhci_sideband_unregister() cleared the existing ones, and unlocked the
>> sideband mutex.
>> Reorganize code so that mutex is only taken and released once in
>> xhci_sideband_unregister(), and clear sb->vdev while mutex is taken.
>>
>> Use mutex guards to reduce human unlock errors in code
>>
>> Refuse to add endpoints or interrupter if sb->vdev is not set.
>> sb->vdev is set when sideband is created and registered.
>>
>> Reported-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@oss.qualcomm.com>
>> Closes: https://lore.kernel.org/linux-usb/20251028080043.27760-1-uttkarsh.aggarwal@oss.qualcomm.com
>> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
> 
> Looks good, thanks for respinning this.  I don't know if it fixes the
> issue, but it looks sane :)
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks for reviewing this.

I don't have the hardware to test this myself either

Thanks
Mathias

On 10/29/2025 7:22 PM, Mathias Nyman wrote:
> On 10/29/25 14:51, Greg KH wrote:
>> On Wed, Oct 29, 2025 at 02:24:35PM +0200, Mathias Nyman wrote:
>>> Uttkarsh Aggarwal observed a kernel panic during sideband un-register
>>> and found it was caused by a race condition between sideband unregister,
>>> and creating sideband interrupters.
>>> The issue occurrs when thread T1 runs uaudio_disconnect() and released
>>> sb->xhci via sideband_unregister, while thread T2 simultaneously 
>>> accessed
>>> the now-NULL sb->xhci in xhci_sideband_create_interrupter() resulting in
>>> a crash.
>>>
>>> Ensure new endpoints or interrupter can't be added to a sidenband after
>>> xhci_sideband_unregister() cleared the existing ones, and unlocked the
>>> sideband mutex.
>>> Reorganize code so that mutex is only taken and released once in
>>> xhci_sideband_unregister(), and clear sb->vdev while mutex is taken.
>>>
>>> Use mutex guards to reduce human unlock errors in code
>>>
>>> Refuse to add endpoints or interrupter if sb->vdev is not set.
>>> sb->vdev is set when sideband is created and registered.
>>>
>>> Reported-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@oss.qualcomm.com>
>>> Closes: https://lore.kernel.org/linux-usb/20251028080043.27760-1- 
>>> uttkarsh.aggarwal@oss.qualcomm.com
>>> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a 
>>> secondary interrupter entity")
>>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>> ---
>>
>> Looks good, thanks for respinning this.  I don't know if it fixes the
>> issue, but it looks sane :)
>>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Thanks for reviewing this.
> 
> I don't have the hardware to test this myself either
> 
> Thanks
> Mathias

Hi Mathias,

I have shared the patch with our OEMs and so far, have not received any 
reports of issues from them.

Thanks
Uttkarsh

On 11/7/25 08:16, Uttkarsh Aggarwal wrote:
> 
> 
> On 10/29/2025 7:22 PM, Mathias Nyman wrote:
>> On 10/29/25 14:51, Greg KH wrote:
>>> On Wed, Oct 29, 2025 at 02:24:35PM +0200, Mathias Nyman wrote:
>>>> Uttkarsh Aggarwal observed a kernel panic during sideband un-register
>>>> and found it was caused by a race condition between sideband unregister,
>>>> and creating sideband interrupters.
>>>> The issue occurrs when thread T1 runs uaudio_disconnect() and released
>>>> sb->xhci via sideband_unregister, while thread T2 simultaneously accessed
>>>> the now-NULL sb->xhci in xhci_sideband_create_interrupter() resulting in
>>>> a crash.
>>>>
>>>> Ensure new endpoints or interrupter can't be added to a sidenband after
>>>> xhci_sideband_unregister() cleared the existing ones, and unlocked the
>>>> sideband mutex.
>>>> Reorganize code so that mutex is only taken and released once in
>>>> xhci_sideband_unregister(), and clear sb->vdev while mutex is taken.
>>>>
>>>> Use mutex guards to reduce human unlock errors in code
>>>>
>>>> Refuse to add endpoints or interrupter if sb->vdev is not set.
>>>> sb->vdev is set when sideband is created and registered.
>>>>
>>>> Reported-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@oss.qualcomm.com>
>>>> Closes: https://lore.kernel.org/linux-usb/20251028080043.27760-1- uttkarsh.aggarwal@oss.qualcomm.com
>>>> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
>>>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>>> ---
>>>
>>> Looks good, thanks for respinning this.  I don't know if it fixes the
>>> issue, but it looks sane :)
>>>
>>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> Thanks for reviewing this.
>>
>> I don't have the hardware to test this myself either
>>
>> Thanks
>> Mathias
> 
> Hi Mathias,
> 
> I have shared the patch with our OEMs and so far, have not received any reports of issues from them.
> 
> Thanks
> Uttkarsh
> 

Thanks, I'll send it forward

-Mathias

On Tue, Oct 28, 2025 at 01:30:43PM +0530, Uttkarsh Aggarwal wrote:
> A kernel panic was observed due to a race condition between un-registering
> sideband and creating sideband interrupters. The issue occurrs when thread
> T1 runs uaudio_disconnect() and released sb->xhci via sideband_unregister,
> while thread T2 simultaneously accessed the now-NULL sb->xhci in
> xhci_sideband_create_interrupter() resulting in a crash.
> 
> By locking the mutex before modifying sb->xhci, any thread calling
> xhci_sideband_create_interrupter() will either see a valid sb->xhci or wait
> until xhci_sideband_unregister() completes.
> 
> Signed-off-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@oss.qualcomm.com>

What commit id does this fix?  Should it be backported to older kernels?

> ---
>  drivers/usb/host/xhci-sideband.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index e771a476fef2..74a58f759cee 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -481,10 +481,12 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>  
>  	xhci_sideband_remove_interrupter(sb);
>  
> +	mutex_lock(&sb->mutex);
>  	spin_lock_irq(&xhci->lock);

A mutex and a spinlock irq?  That just feels wrong for the obvious
reasons, only one should be needed.

thanks,

greg k-h