[PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[胡连勤]

xhci_sideband_unregister() assumes the virtual device (vdev) is still
alive when iterating sideband endpoints and issuing stop endpoint
commands. However, xhci_disable_and_free_slot() may have already freed
vdev and its out_ctx before xhci_sideband_unregister() is invoked.

This happens when xhci_setup_device() gets COMP_USB_TRANSACTION_ERROR
(e.g. device not responding to setup address during bus reset recovery),
causing vdev to be freed before xhci_sideband_unregister() is called:

  hub_event()
    xhci_setup_device()                  <-- COMP_USB_TRANSACTION_ERROR
    xhci_disable_and_free_slot()
      xhci_free_virt_device()
        kfree(out_ctx), kfree(vdev)
        xhci->devs[slot_id] = NULL
    ...
    usb_disconnect()
      uaudio_disconnect()
        xhci_sideband_unregister()
          xhci_stop_endpoint_sync()
            xhci_get_ep_ctx()            <-- CRASH (deref freed out_ctx)

Unable to handle kernel paging request at virtual address dead000000000122
Call trace:
 xhci_get_ep_ctx+0x0/0x38
 xhci_sideband_unregister+0x68/0xf0
 uaudio_disconnect+0x70/0x144
 usb_audio_disconnect+0x7c/0x268
 usb_unbind_interface+0x13c/0x340
 device_release_driver_internal+0x1c4/0x2bc
 device_release_driver+0x18/0x28
 bus_remove_device+0x158/0x170
 device_del+0x1c8/0x320
 usb_disable_device+0x84/0x190
 usb_disconnect+0xe8/0x338
 hub_event+0xbd8/0x19ac
 process_scheduled_works+0x200/0x9d8
 worker_thread+0x154/0x3b0
 kthread+0x11c/0x1a0

Fix this by caching the slot_id in the sideband structure at
registration time, then checking under xhci->lock whether
xhci->devs[slot_id] still matches sb->vdev before issuing stop
endpoint commands. If vdev has been freed, skip endpoint cleanup
entirely - the xHCI has already disabled the slot.
The interrupter is still removed as it does not depend on vdev.

The slot_id is cached in sb->slot_id rather than read from vdev at
unregister time because vdev may already be freed, making
sb->vdev->slot_id a dangling dereference.

Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
Cc: stable@vger.kernel.org
Signed-off-by: Lianqin Hu <hulianqin@vivo.com>
---
 drivers/usb/host/xhci-sideband.c  | 35 ++++++++++++++++++++++++++-----
 include/linux/usb/xhci-sideband.h |  2 ++
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index a5deeee4d5dc..1a83f03c2cf6 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -437,6 +437,7 @@ xhci_sideband_register(struct usb_interface *intf, enum xhci_sideband_type type,
 
 	sb->xhci = xhci;
 	sb->vdev = vdev;
+	sb->slot_id = udev->slot_id;
 	sb->intf = intf;
 	sb->type = type;
 	sb->notify_client = notify_client;
@@ -464,6 +465,7 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 	struct xhci_virt_device *vdev;
 	struct xhci_hcd *xhci;
 	int i;
+	bool vdev_alive = false;
 
 	if (!sb)
 		return;
@@ -473,11 +475,32 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 	scoped_guard(mutex, &sb->mutex) {
 		vdev = sb->vdev;
 		if (!vdev)
-			return;
+			goto out;
+
+		/*
+		 * Check if vdev is still the active device for its slot.
+		 * xhci_disable_and_free_slot() may have already freed vdev
+		 * and cleared xhci->devs[slot_id] (e.g. on
+		 * COMP_USB_TRANSACTION_ERROR during bus reset recovery in
+		 * xhci_setup_device). In that case sb->vdev->out_ctx is
+		 * dangling and issuing stop endpoint commands would crash
+		 * with a paging request at LIST_POISON1 + offset. If vdev
+		 * is gone, just clear the sideband pointers without
+		 * touching xHCI.
+		 */
+		spin_lock_irq(&xhci->lock);
+		vdev_alive = (xhci->devs[sb->slot_id] == vdev);
+		spin_unlock_irq(&xhci->lock);
 
-		for (i = 0; i < EP_CTX_PER_DEV; i++)
-			if (sb->eps[i])
-				__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
+		if (vdev_alive) {
+			for (i = 0; i < EP_CTX_PER_DEV; i++)
+				if (sb->eps[i])
+					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
+		} else {
+			xhci_warn(xhci, "sideband unreg: vdev slot %d already freed, skipping ep cleanup\n",
+				  sb->slot_id);
+			for (i = 0; i < EP_CTX_PER_DEV; i++)
+				sb->eps[i] = NULL;
+		}
 
 		__xhci_sideband_remove_interrupter(sb);
 
@@ -486,9 +509,11 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 
 	spin_lock_irq(&xhci->lock);
 	sb->xhci = NULL;
-	vdev->sideband = NULL;
+	if (vdev_alive)
+		vdev->sideband = NULL;
 	spin_unlock_irq(&xhci->lock);
 
+out:
 	kfree(sb);
 }
 EXPORT_SYMBOL_GPL(xhci_sideband_unregister);
diff --git a/include/linux/usb/xhci-sideband.h b/include/linux/usb/xhci-sideband.h
index 005257085dcb..9d86b1daee27 100644
--- a/include/linux/usb/xhci-sideband.h
+++ b/include/linux/usb/xhci-sideband.h
@@ -40,6 +40,7 @@ struct xhci_sideband_event {
  * struct xhci_sideband - representation of a sideband accessed usb device.
  * @xhci: The xhci host controller the usb device is connected to
  * @vdev: the usb device accessed via sideband
+ * @slot_id: cached slot ID for vdev validity checking
  * @eps: array of endpoints controlled via sideband
  * @ir: event handling and buffer for sideband accessed device
  * @type: xHCI sideband type
@@ -52,6 +53,7 @@ struct xhci_sideband_event {
 struct xhci_sideband {
 	struct xhci_hcd                 *xhci;
 	struct xhci_virt_device         *vdev;
+	int                             slot_id;
 	struct xhci_virt_ep             *eps[EP_CTX_PER_DEV];
 	struct xhci_interrupter         *ir;
 	enum xhci_sideband_type		type;
-- 
2.48.1

Re: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Mathias Nyman]

On 9/7/26 15:24, 胡连勤 wrote:
> xhci_sideband_unregister() assumes the virtual device (vdev) is still
> alive when iterating sideband endpoints and issuing stop endpoint
> commands. However, xhci_disable_and_free_slot() may have already freed
> vdev and its out_ctx before xhci_sideband_unregister() is invoked.
> 
> This happens when xhci_setup_device() gets COMP_USB_TRANSACTION_ERROR
> (e.g. device not responding to setup address during bus reset recovery),
> causing vdev to be freed before xhci_sideband_unregister() is called:
> 
>    hub_event()
>      xhci_setup_device()                  <-- COMP_USB_TRANSACTION_ERROR
>      xhci_disable_and_free_slot()
>        xhci_free_virt_device()
>          kfree(out_ctx), kfree(vdev)
>          xhci->devs[slot_id] = NULL
>      ...
>      usb_disconnect()
>        uaudio_disconnect()
>          xhci_sideband_unregister()
>            xhci_stop_endpoint_sync()
>              xhci_get_ep_ctx()            <-- CRASH (deref freed out_ctx)
> 
> Unable to handle kernel paging request at virtual address dead000000000122
> Call trace:
>   xhci_get_ep_ctx+0x0/0x38
>   xhci_sideband_unregister+0x68/0xf0
>   uaudio_disconnect+0x70/0x144
>   usb_audio_disconnect+0x7c/0x268
>   usb_unbind_interface+0x13c/0x340
>   device_release_driver_internal+0x1c4/0x2bc
>   device_release_driver+0x18/0x28
>   bus_remove_device+0x158/0x170
>   device_del+0x1c8/0x320
>   usb_disable_device+0x84/0x190
>   usb_disconnect+0xe8/0x338
>   hub_event+0xbd8/0x19ac
>   process_scheduled_works+0x200/0x9d8
>   worker_thread+0x154/0x3b0
>   kthread+0x11c/0x1a0
> 
> Fix this by caching the slot_id in the sideband structure at
> registration time, then checking under xhci->lock whether
> xhci->devs[slot_id] still matches sb->vdev before issuing stop
> endpoint commands. If vdev has been freed, skip endpoint cleanup
> entirely - the xHCI has already disabled the slot.
> The interrupter is still removed as it does not depend on vdev.
> 
> The slot_id is cached in sb->slot_id rather than read from vdev at
> unregister time because vdev may already be freed, making
> sb->vdev->slot_id a dangling dereference.
> 
> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lianqin Hu <hulianqin@vivo.com>

Thanks, nice catch and layout of the problem.

I think we need to address this issue a lot earlier than in xhci_sideband_unregister()

xhci_free_virt_device() shouldn't leave any dangling pointers, if vdev->sideband
is still set at this point then something is wrong, and should as a final resort be
fixed here. Print a debug message and set  vdev->sideband->vdev = NULL before freeing vdev.

Another issue is the transaction error recovery during address device.
xHCI specs say we should disable and re-enable the slot.
xhci driver additionally frees and reallocates the vdev.
We could probably avoid this and just re-initialize the contexts without
reallocating vdev.  This being said I think it would be even better to not
try to 'usb persist' sideband over a usb device reset.

Might be best to unregister sideband in qualcomm usb audio driver completely in
the drv->pre_reset, and re-register it back in drv->post_reset

But to avoid this specific issue we should also set vdev->sideband->vdev to NULL
in xhci_free_virt_device()

Thanks
Mathias

Re: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Selvarasu Ganesan]

On 9/10/2026 3:04 PM, Mathias Nyman wrote:
> On 9/7/26 15:24, 胡连勤 wrote:
>> xhci_sideband_unregister() assumes the virtual device (vdev) is still
>> alive when iterating sideband endpoints and issuing stop endpoint
>> commands. However, xhci_disable_and_free_slot() may have already freed
>> vdev and its out_ctx before xhci_sideband_unregister() is invoked.
>>
>> This happens when xhci_setup_device() gets COMP_USB_TRANSACTION_ERROR
>> (e.g. device not responding to setup address during bus reset recovery),
>> causing vdev to be freed before xhci_sideband_unregister() is called:
>>
>>    hub_event()
>>      xhci_setup_device()                  <-- COMP_USB_TRANSACTION_ERROR
>>      xhci_disable_and_free_slot()
>>        xhci_free_virt_device()
>>          kfree(out_ctx), kfree(vdev)
>>          xhci->devs[slot_id] = NULL
>>      ...
>>      usb_disconnect()
>>        uaudio_disconnect()
>>          xhci_sideband_unregister()
>>            xhci_stop_endpoint_sync()
>>              xhci_get_ep_ctx()            <-- CRASH (deref freed 
>> out_ctx)
>>
>> Unable to handle kernel paging request at virtual address 
>> dead000000000122
>> Call trace:
>>   xhci_get_ep_ctx+0x0/0x38
>>   xhci_sideband_unregister+0x68/0xf0
>>   uaudio_disconnect+0x70/0x144
>>   usb_audio_disconnect+0x7c/0x268
>>   usb_unbind_interface+0x13c/0x340
>>   device_release_driver_internal+0x1c4/0x2bc
>>   device_release_driver+0x18/0x28
>>   bus_remove_device+0x158/0x170
>>   device_del+0x1c8/0x320
>>   usb_disable_device+0x84/0x190
>>   usb_disconnect+0xe8/0x338
>>   hub_event+0xbd8/0x19ac
>>   process_scheduled_works+0x200/0x9d8
>>   worker_thread+0x154/0x3b0
>>   kthread+0x11c/0x1a0
>>
>> Fix this by caching the slot_id in the sideband structure at
>> registration time, then checking under xhci->lock whether
>> xhci->devs[slot_id] still matches sb->vdev before issuing stop
>> endpoint commands. If vdev has been freed, skip endpoint cleanup
>> entirely - the xHCI has already disabled the slot.
>> The interrupter is still removed as it does not depend on vdev.
>>
>> The slot_id is cached in sb->slot_id rather than read from vdev at
>> unregister time because vdev may already be freed, making
>> sb->vdev->slot_id a dangling dereference.
>>
>> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a 
>> secondary interrupter entity")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Lianqin Hu <hulianqin@vivo.com>
>
> Thanks, nice catch and layout of the problem.
>
> I think we need to address this issue a lot earlier than in 
> xhci_sideband_unregister()
>
> xhci_free_virt_device() shouldn't leave any dangling pointers, if 
> vdev->sideband
> is still set at this point then something is wrong, and should as a 
> final resort be
> fixed here. Print a debug message and set vdev->sideband->vdev = NULL 
> before freeing vdev.
>
> Another issue is the transaction error recovery during address device.
> xHCI specs say we should disable and re-enable the slot.
> xhci driver additionally frees and reallocates the vdev.
> We could probably avoid this and just re-initialize the contexts without
> reallocating vdev.  This being said I think it would be even better to 
> not
> try to 'usb persist' sideband over a usb device reset.
>
> Might be best to unregister sideband in qualcomm usb audio driver 
> completely in
> the drv->pre_reset, and re-register it back in drv->post_reset
>
> But to avoid this specific issue we should also set 
> vdev->sideband->vdev to NULL
> in xhci_free_virt_device()

Regarding the suggestion to set vdev->sideband->vdev = NULL within 
xhci_free_virt_device() to avoid dangling pointers, I agree that this 
effectively prevents the use after free during unregistration.

But, I would like to highlight a critical point regarding the 
interrupter lifecycle. Even if sb->vdev is set to NULL , 
__xhci_sideband_remove_interrupter() must still be invoked during the 
unregistration sequence.

If the interrupter removal is skipped because vdev=NULL , the secondary 
interrupter resource is leaked. In our observations, this leads to a 
failure during the subsequent device connection and registration 
attempt, resulting in the error: "Failed to add secondary interrupter, 
max interrupters".

So it is essential that the cleanup path ensures the interrupter is 
released regardless of whether the vdev is still alive.


Thanks,
Selva
>
> Thanks
> Mathias

答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[胡连勤]

Hi Mathias, Selva,

> > I think we need to address this issue a lot earlier than in
> > xhci_sideband_unregister()
> >
> > xhci_free_virt_device() shouldn't leave any dangling pointers, if
> > vdev->sideband
> > is still set at this point then something is wrong, and should as a
> > final resort be
> > fixed here. Print a debug message and set vdev->sideband->vdev = NULL
> > before freeing vdev.
> >
> > Another issue is the transaction error recovery during address device.
> > xHCI specs say we should disable and re-enable the slot.
> > xhci driver additionally frees and reallocates the vdev.
> > We could probably avoid this and just re-initialize the contexts without
> > reallocating vdev.  This being said I think it would be even better to
> > not
> > try to 'usb persist' sideband over a usb device reset.
> >
> > Might be best to unregister sideband in qualcomm usb audio driver
> > completely in
> > the drv->pre_reset, and re-register it back in drv->post_reset
> >
> > But to avoid this specific issue we should also set
> > vdev->sideband->vdev to NULL
> > in xhci_free_virt_device()
> 
> Regarding the suggestion to set vdev->sideband->vdev = NULL within
> xhci_free_virt_device() to avoid dangling pointers, I agree that this
> effectively prevents the use after free during unregistration.
> 
> But, I would like to highlight a critical point regarding the
> interrupter lifecycle. Even if sb->vdev is set to NULL ,
> __xhci_sideband_remove_interrupter() must still be invoked during the
> unregistration sequence.
> 
> If the interrupter removal is skipped because vdev=NULL , the secondary
> interrupter resource is leaked. In our observations, this leads to a
> failure during the subsequent device connection and registration
> attempt, resulting in the error: "Failed to add secondary interrupter,
> max interrupters".
> 
> So it is essential that the cleanup path ensures the interrupter is
> released regardless of whether the vdev is still alive.
> 

Thanks for the review and the detailed suggestions.

You're right, xhci_free_virt_device() is the right place to ensure
no dangling pointers are left. I've updated the patch accordingly:

1. In xhci_free_virt_device(), if vdev->sideband is still set at free
   time, print a debug message and set vdev->sideband->vdev = NULL
   before kfree(dev). This breaks the dangling pointer at the source.

2. In xhci_sideband_unregister(), check sb->vdev before issuing stop
   endpoint commands. If already NULL (cleared by
   xhci_free_virt_device), skip endpoint cleanup but still remove the
   interrupter and free the sideband instance. The interrupter and
   sideband struct are host-level resources independent of vdev's
   lifecycle, so they must be released unconditionally to avoid
   leaks.

Regarding Selva's point on the interrupter lifecycle: I entirely
agree. If the interrupter removal is skipped when vdev is NULL,
the secondary interrupter leaks and causes "Failed to add secondary
interrupter, max interrupters" on subsequent device connections.
This is exactly why the updated patch ensures
__xhci_sideband_remove_interrupter() is called regardless of whether
vdev is still alive.

Regarding the USB device reset path: I agree that unregistering
sideband in drv->pre_reset and re-registering in drv->post_reset
would be the cleaner approach. I'll look into implementing this as
a follow-up change in the qualcomm usb audio offload driver.

Regarding the vdev free+realloc during address device error recovery:
while re-initializing contexts without reallocating vdev could reduce
the risk of dangling pointers, this is a separate concern from the
immediate fix and would require a thorough analysis of the slot
lifecycle. I plan to investigate this as a separate effort.

Proposed changes below for review:

 drivers/usb/host/xhci-mem.c      |  8 ++++++++
 drivers/usb/host/xhci-sideband.c | 25 ++++++++++++++++++-------
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index af8d4b74c4ba..afdcfb38b35f 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -922,6 +922,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
 		dev->rhub_port->slot_id = 0;
 	if (xhci->devs[slot_id] == dev)
 		xhci->devs[slot_id] = NULL;
+
+	if (dev->sideband) {
+		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
+			  slot_id);
+		dev->sideband->vdev = NULL;
+		dev->sideband = NULL;
+	}
+
 	kfree(dev);
 }
 
diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index a5deeee4d5dc..6312c9e3af65 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -472,12 +472,22 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 
 	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]);
+		/*
+		 * If vdev is NULL, xhci_free_virt_device() has already
+		 * cleared sb->vdev and freed vdev (e.g. on
+		 * COMP_USB_TRANSACTION_ERROR during address device
+		 * recovery). Skip endpoint cleanup as the xHC has already
+		 * disabled the slot.
+		 *
+		 * The interrupter and sideband instance are host-level
+		 * resources independent of vdev, so still remove and free
+		 * them to avoid leaks.
+		 */
+		if (vdev) {
+			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);
 
@@ -486,7 +496,8 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 
 	spin_lock_irq(&xhci->lock);
 	sb->xhci = NULL;
-	vdev->sideband = NULL;
+	if (vdev)
+		vdev->sideband = NULL;
 	spin_unlock_irq(&xhci->lock);
 
 	kfree(sb);

Does this approach look good to you? If so I'll send a formal v2 patch.

Thanks
Lianqin

Re: 答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Mathias Nyman]

On 9/10/26 15:11, 胡连勤 wrote:
> Hi Mathias, Selva,
> 
>>> I think we need to address this issue a lot earlier than in
>>> xhci_sideband_unregister()
>>>
>>> xhci_free_virt_device() shouldn't leave any dangling pointers, if
>>> vdev->sideband
>>> is still set at this point then something is wrong, and should as a
>>> final resort be
>>> fixed here. Print a debug message and set vdev->sideband->vdev = NULL
>>> before freeing vdev.
>>>
>>> Another issue is the transaction error recovery during address device.
>>> xHCI specs say we should disable and re-enable the slot.
>>> xhci driver additionally frees and reallocates the vdev.
>>> We could probably avoid this and just re-initialize the contexts without
>>> reallocating vdev.  This being said I think it would be even better to
>>> not
>>> try to 'usb persist' sideband over a usb device reset.
>>>
>>> Might be best to unregister sideband in qualcomm usb audio driver
>>> completely in
>>> the drv->pre_reset, and re-register it back in drv->post_reset
>>>
>>> But to avoid this specific issue we should also set
>>> vdev->sideband->vdev to NULL
>>> in xhci_free_virt_device()
>>
>> Regarding the suggestion to set vdev->sideband->vdev = NULL within
>> xhci_free_virt_device() to avoid dangling pointers, I agree that this
>> effectively prevents the use after free during unregistration.
>>
>> But, I would like to highlight a critical point regarding the
>> interrupter lifecycle. Even if sb->vdev is set to NULL ,
>> __xhci_sideband_remove_interrupter() must still be invoked during the
>> unregistration sequence.
>>
>> If the interrupter removal is skipped because vdev=NULL , the secondary
>> interrupter resource is leaked. In our observations, this leads to a
>> failure during the subsequent device connection and registration
>> attempt, resulting in the error: "Failed to add secondary interrupter,
>> max interrupters".
>>
>> So it is essential that the cleanup path ensures the interrupter is
>> released regardless of whether the vdev is still alive.
>>
> 
> Thanks for the review and the detailed suggestions.
> 
> You're right, xhci_free_virt_device() is the right place to ensure
> no dangling pointers are left. I've updated the patch accordingly:
> 
> 1. In xhci_free_virt_device(), if vdev->sideband is still set at free
>     time, print a debug message and set vdev->sideband->vdev = NULL
>     before kfree(dev). This breaks the dangling pointer at the source.
> 
> 2. In xhci_sideband_unregister(), check sb->vdev before issuing stop
>     endpoint commands. If already NULL (cleared by
>     xhci_free_virt_device), skip endpoint cleanup but still remove the
>     interrupter and free the sideband instance. The interrupter and
>     sideband struct are host-level resources independent of vdev's
>     lifecycle, so they must be released unconditionally to avoid
>     leaks.
> 
> Regarding Selva's point on the interrupter lifecycle: I entirely
> agree. If the interrupter removal is skipped when vdev is NULL,
> the secondary interrupter leaks and causes "Failed to add secondary
> interrupter, max interrupters" on subsequent device connections.
> This is exactly why the updated patch ensures
> __xhci_sideband_remove_interrupter() is called regardless of whether
> vdev is still alive.
> 
> Regarding the USB device reset path: I agree that unregistering
> sideband in drv->pre_reset and re-registering in drv->post_reset
> would be the cleaner approach. I'll look into implementing this as
> a follow-up change in the qualcomm usb audio offload driver.
> 
> Regarding the vdev free+realloc during address device error recovery:
> while re-initializing contexts without reallocating vdev could reduce
> the risk of dangling pointers, this is a separate concern from the
> immediate fix and would require a thorough analysis of the slot
> lifecycle. I plan to investigate this as a separate effort.
> 
> Proposed changes below for review:
> 
>   drivers/usb/host/xhci-mem.c      |  8 ++++++++
>   drivers/usb/host/xhci-sideband.c | 25 ++++++++++++++++++-------
>   2 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index af8d4b74c4ba..afdcfb38b35f 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -922,6 +922,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
>   		dev->rhub_port->slot_id = 0;
>   	if (xhci->devs[slot_id] == dev)
>   		xhci->devs[slot_id] = NULL;
> +
> +	if (dev->sideband) {
> +		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
> +			  slot_id);
> +		dev->sideband->vdev = NULL;
> +		dev->sideband = NULL;
> +	}
> +
>   	kfree(dev);
>   }
>   
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index a5deeee4d5dc..6312c9e3af65 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -472,12 +472,22 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>   
>   	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]);
> +		/*
> +		 * If vdev is NULL, xhci_free_virt_device() has already
> +		 * cleared sb->vdev and freed vdev (e.g. on
> +		 * COMP_USB_TRANSACTION_ERROR during address device
> +		 * recovery). Skip endpoint cleanup as the xHC has already
> +		 * disabled the slot.
> +		 *
> +		 * The interrupter and sideband instance are host-level
> +		 * resources independent of vdev, so still remove and free
> +		 * them to avoid leaks.
> +		 */
> +		if (vdev) {
> +			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);
>   
> @@ -486,7 +496,8 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>   
>   	spin_lock_irq(&xhci->lock);
>   	sb->xhci = NULL;
> -	vdev->sideband = NULL;
> +	if (vdev)
> +		vdev->sideband = NULL;
>   	spin_unlock_irq(&xhci->lock);
>   
>   	kfree(sb);
> 
> Does this approach look good to you? If so I'll send a formal v2 patch.
> 

Looks good to me

Thanks
Mathias

Re: 答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Selvarasu Ganesan]

On 9/10/2026 5:41 PM, 胡连勤 wrote:
> Hi Mathias, Selva,
>
>>> I think we need to address this issue a lot earlier than in
>>> xhci_sideband_unregister()
>>>
>>> xhci_free_virt_device() shouldn't leave any dangling pointers, if
>>> vdev->sideband
>>> is still set at this point then something is wrong, and should as a
>>> final resort be
>>> fixed here. Print a debug message and set vdev->sideband->vdev = NULL
>>> before freeing vdev.
>>>
>>> Another issue is the transaction error recovery during address device.
>>> xHCI specs say we should disable and re-enable the slot.
>>> xhci driver additionally frees and reallocates the vdev.
>>> We could probably avoid this and just re-initialize the contexts without
>>> reallocating vdev.  This being said I think it would be even better to
>>> not
>>> try to 'usb persist' sideband over a usb device reset.
>>>
>>> Might be best to unregister sideband in qualcomm usb audio driver
>>> completely in
>>> the drv->pre_reset, and re-register it back in drv->post_reset
>>>
>>> But to avoid this specific issue we should also set
>>> vdev->sideband->vdev to NULL
>>> in xhci_free_virt_device()
>> Regarding the suggestion to set vdev->sideband->vdev = NULL within
>> xhci_free_virt_device() to avoid dangling pointers, I agree that this
>> effectively prevents the use after free during unregistration.
>>
>> But, I would like to highlight a critical point regarding the
>> interrupter lifecycle. Even if sb->vdev is set to NULL ,
>> __xhci_sideband_remove_interrupter() must still be invoked during the
>> unregistration sequence.
>>
>> If the interrupter removal is skipped because vdev=NULL , the secondary
>> interrupter resource is leaked. In our observations, this leads to a
>> failure during the subsequent device connection and registration
>> attempt, resulting in the error: "Failed to add secondary interrupter,
>> max interrupters".
>>
>> So it is essential that the cleanup path ensures the interrupter is
>> released regardless of whether the vdev is still alive.
>>
> Thanks for the review and the detailed suggestions.
>
> You're right, xhci_free_virt_device() is the right place to ensure
> no dangling pointers are left. I've updated the patch accordingly:
>
> 1. In xhci_free_virt_device(), if vdev->sideband is still set at free
>     time, print a debug message and set vdev->sideband->vdev = NULL
>     before kfree(dev). This breaks the dangling pointer at the source.
>
> 2. In xhci_sideband_unregister(), check sb->vdev before issuing stop
>     endpoint commands. If already NULL (cleared by
>     xhci_free_virt_device), skip endpoint cleanup but still remove the
>     interrupter and free the sideband instance. The interrupter and
>     sideband struct are host-level resources independent of vdev's
>     lifecycle, so they must be released unconditionally to avoid
>     leaks.
>
> Regarding Selva's point on the interrupter lifecycle: I entirely
> agree. If the interrupter removal is skipped when vdev is NULL,
> the secondary interrupter leaks and causes "Failed to add secondary
> interrupter, max interrupters" on subsequent device connections.
> This is exactly why the updated patch ensures
> __xhci_sideband_remove_interrupter() is called regardless of whether
> vdev is still alive.
>
> Regarding the USB device reset path: I agree that unregistering
> sideband in drv->pre_reset and re-registering in drv->post_reset
> would be the cleaner approach. I'll look into implementing this as
> a follow-up change in the qualcomm usb audio offload driver.
>
> Regarding the vdev free+realloc during address device error recovery:
> while re-initializing contexts without reallocating vdev could reduce
> the risk of dangling pointers, this is a separate concern from the
> immediate fix and would require a thorough analysis of the slot
> lifecycle. I plan to investigate this as a separate effort.
>
> Proposed changes below for review:
>
>   drivers/usb/host/xhci-mem.c      |  8 ++++++++
>   drivers/usb/host/xhci-sideband.c | 25 ++++++++++++++++++-------
>   2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index af8d4b74c4ba..afdcfb38b35f 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -922,6 +922,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
>   		dev->rhub_port->slot_id = 0;
>   	if (xhci->devs[slot_id] == dev)
>   		xhci->devs[slot_id] = NULL;
> +
> +	if (dev->sideband) {
> +		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
> +			  slot_id);
> +		dev->sideband->vdev = NULL;

Thanks for your updated patch.

Dont forget to add #include <linux/usb/xhci-sideband.h> in this 
xhci-mem.c file otherwise getting below error,

drivers/usb/host/xhci-mem.c:928:30: error: invalid use of undefined type 
‘struct xhci_sideband’
     928 |                 dev->sideband->vdev = NULL;


> +		dev->sideband = NULL;
> +	}
> +
>   	kfree(dev);
>   }
>   
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index a5deeee4d5dc..6312c9e3af65 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -472,12 +472,22 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>   
>   	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]);
> +		/*
> +		 * If vdev is NULL, xhci_free_virt_device() has already
> +		 * cleared sb->vdev and freed vdev (e.g. on
> +		 * COMP_USB_TRANSACTION_ERROR during address device
> +		 * recovery). Skip endpoint cleanup as the xHC has already
> +		 * disabled the slot.
> +		 *
> +		 * The interrupter and sideband instance are host-level
> +		 * resources independent of vdev, so still remove and free
> +		 * them to avoid leaks.
> +		 */
> +		if (vdev) {
> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
> +				if (sb->eps[i])
> +					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);

I have one query on  skip endpoint cleanup due to  vdev is NULL, in this 
case the pointers in sb->eps are not cleared. Since these pointers point 
into the virtual device eps, any subsequent call to sideband API 
functions like xhci_sideband_get_endpoint_buffer() that dereference 
sb->eps could result in a use after free if the virtual device has been 
freed. Is it possible?


Thanks,

Selva

> +		}
>   
>   		__xhci_sideband_remove_interrupter(sb);
>   
> @@ -486,7 +496,8 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>   
>   	spin_lock_irq(&xhci->lock);
>   	sb->xhci = NULL;
> -	vdev->sideband = NULL;
> +	if (vdev)
> +		vdev->sideband = NULL;
>   	spin_unlock_irq(&xhci->lock);
>   
>   	kfree(sb);
>
> Does this approach look good to you? If so I'll send a formal v2 patch.
>
> Thanks
> Lianqin

答复: 答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[胡连勤]

Hi Selva,

> >
> >   drivers/usb/host/xhci-mem.c      |  8 ++++++++
> >   drivers/usb/host/xhci-sideband.c | 25 ++++++++++++++++++-------
> >   2 files changed, 26 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> > index af8d4b74c4ba..afdcfb38b35f 100644
> > --- a/drivers/usb/host/xhci-mem.c
> > +++ b/drivers/usb/host/xhci-mem.c
> > @@ -922,6 +922,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
> >   		dev->rhub_port->slot_id = 0;
> >   	if (xhci->devs[slot_id] == dev)
> >   		xhci->devs[slot_id] = NULL;
> > +
> > +	if (dev->sideband) {
> > +		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
> > +			  slot_id);
> > +		dev->sideband->vdev = NULL;
> 
> Thanks for your updated patch.
> 
> Dont forget to add #include <linux/usb/xhci-sideband.h> in this
> xhci-mem.c file otherwise getting below error,
> 
> drivers/usb/host/xhci-mem.c:928:30: error: invalid use of undefined type
> ‘struct xhci_sideband’
>      928 |                 dev->sideband->vdev = NULL;
> 

Include the corresponding header file
#include <linux/dmapool.h>
 #include <linux/dma-mapping.h>
 #include <linux/bitfield.h>
+#include <linux/usb/xhci-sideband.h>
 

> > +		dev->sideband = NULL;
> > +	}
> > +
> >   	kfree(dev);
> >   }
> >
> > diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> > index a5deeee4d5dc..6312c9e3af65 100644
> > --- a/drivers/usb/host/xhci-sideband.c
> > +++ b/drivers/usb/host/xhci-sideband.c
> > @@ -472,12 +472,22 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
> >
> >   	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]);
> > +		/*
> > +		 * If vdev is NULL, xhci_free_virt_device() has already
> > +		 * cleared sb->vdev and freed vdev (e.g. on
> > +		 * COMP_USB_TRANSACTION_ERROR during address device
> > +		 * recovery). Skip endpoint cleanup as the xHC has already
> > +		 * disabled the slot.
> > +		 *
> > +		 * The interrupter and sideband instance are host-level
> > +		 * resources independent of vdev, so still remove and free
> > +		 * them to avoid leaks.
> > +		 */
> > +		if (vdev) {
> > +			for (i = 0; i < EP_CTX_PER_DEV; i++)
> > +				if (sb->eps[i])
> > +					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
> 
> I have one query on  skip endpoint cleanup due to  vdev is NULL, in this
> case the pointers in sb->eps are not cleared. Since these pointers point
> into the virtual device eps, any subsequent call to sideband API
> functions like xhci_sideband_get_endpoint_buffer() that dereference
> sb->eps could result in a use after free if the virtual device has been
> freed. Is it possible?

Yes, you're absolutely right. If sb->eps[] is not cleared when vdev
is NULL, subsequent calls to xhci_sideband_get_endpoint_buffer() or
similar API functions would dereference dangling pointers into the
freed vdev, resulting in use-after-free.

I added the else branch to clear sb->eps[]:

	} else {
		for (i = 0; i < EP_CTX_PER_DEV; i++)
			sb->eps[i] = NULL;
	}


The complete code modification is as follows:
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index af8d4b74c4ba..448d28aaff3e 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -15,6 +15,7 @@
 #include <linux/dmapool.h>
 #include <linux/dma-mapping.h>
 #include <linux/bitfield.h>
+#include <linux/usb/xhci-sideband.h>
 
 #include "xhci.h"
 #include "xhci-trace.h"
@@ -922,6 +923,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
 		dev->rhub_port->slot_id = 0;
 	if (xhci->devs[slot_id] == dev)
 		xhci->devs[slot_id] = NULL;
+
+	if (dev->sideband) {
+		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
+			 slot_id);
+		dev->sideband->vdev = NULL;
+		dev->sideband = NULL;
+	}
+
 	kfree(dev);
 }
 
diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index a5deeee4d5dc..f979ce517163 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -472,12 +472,25 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 
 	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]);
+		/*
+		 * If vdev is NULL, xhci_free_virt_device() has already
+		 * cleared sb->vdev and freed vdev (e.g. on
+		 * COMP_USB_TRANSACTION_ERROR during address device
+		 * recovery). Skip endpoint cleanup as the xHC has already
+		 * disabled the slot.
+		 *
+		 * The interrupter and sideband instance are host-level
+		 * resources independent of vdev, so still remove and free
+		 * them to avoid leaks.
+		 */
+		if (vdev) {
+			for (i = 0; i < EP_CTX_PER_DEV; i++)
+				if (sb->eps[i])
+					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
+		} else {
+			for (i = 0; i < EP_CTX_PER_DEV; i++)
+				sb->eps[i] = NULL;
+		}
 
 		__xhci_sideband_remove_interrupter(sb);
 
@@ -486,7 +499,8 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 
 	spin_lock_irq(&xhci->lock);
 	sb->xhci = NULL;
-	vdev->sideband = NULL;
+	if (vdev)
+		vdev->sideband = NULL;
 	spin_unlock_irq(&xhci->lock);
 
 	kfree(sb);

Thanks
Lianqin

Re: 答复: 答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Selvarasu Ganesan]

On 9/11/2026 12:59 PM, 胡连勤 wrote:
> Hi Selva,
>
>>>    drivers/usb/host/xhci-mem.c      |  8 ++++++++
>>>    drivers/usb/host/xhci-sideband.c | 25 ++++++++++++++++++-------
>>>    2 files changed, 26 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>>> index af8d4b74c4ba..afdcfb38b35f 100644
>>> --- a/drivers/usb/host/xhci-mem.c
>>> +++ b/drivers/usb/host/xhci-mem.c
>>> @@ -922,6 +922,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
>>>    		dev->rhub_port->slot_id = 0;
>>>    	if (xhci->devs[slot_id] == dev)
>>>    		xhci->devs[slot_id] = NULL;
>>> +
>>> +	if (dev->sideband) {
>>> +		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
>>> +			  slot_id);
>>> +		dev->sideband->vdev = NULL;
>> Thanks for your updated patch.
>>
>> Dont forget to add #include <linux/usb/xhci-sideband.h> in this
>> xhci-mem.c file otherwise getting below error,
>>
>> drivers/usb/host/xhci-mem.c:928:30: error: invalid use of undefined type
>> ‘struct xhci_sideband’
>>       928 |                 dev->sideband->vdev = NULL;
>>
> Include the corresponding header file
> #include <linux/dmapool.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/bitfield.h>
> +#include <linux/usb/xhci-sideband.h>
>   
>
>>> +		dev->sideband = NULL;
>>> +	}
>>> +
>>>    	kfree(dev);
>>>    }
>>>
>>> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
>>> index a5deeee4d5dc..6312c9e3af65 100644
>>> --- a/drivers/usb/host/xhci-sideband.c
>>> +++ b/drivers/usb/host/xhci-sideband.c
>>> @@ -472,12 +472,22 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>>>
>>>    	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]);
>>> +		/*
>>> +		 * If vdev is NULL, xhci_free_virt_device() has already
>>> +		 * cleared sb->vdev and freed vdev (e.g. on
>>> +		 * COMP_USB_TRANSACTION_ERROR during address device
>>> +		 * recovery). Skip endpoint cleanup as the xHC has already
>>> +		 * disabled the slot.
>>> +		 *
>>> +		 * The interrupter and sideband instance are host-level
>>> +		 * resources independent of vdev, so still remove and free
>>> +		 * them to avoid leaks.
>>> +		 */
>>> +		if (vdev) {
>>> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
>>> +				if (sb->eps[i])
>>> +					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
>> I have one query on  skip endpoint cleanup due to  vdev is NULL, in this
>> case the pointers in sb->eps are not cleared. Since these pointers point
>> into the virtual device eps, any subsequent call to sideband API
>> functions like xhci_sideband_get_endpoint_buffer() that dereference
>> sb->eps could result in a use after free if the virtual device has been
>> freed. Is it possible?
> Yes, you're absolutely right. If sb->eps[] is not cleared when vdev
> is NULL, subsequent calls to xhci_sideband_get_endpoint_buffer() or
> similar API functions would dereference dangling pointers into the
> freed vdev, resulting in use-after-free.
>
> I added the else branch to clear sb->eps[]:
>
> 	} else {
> 		for (i = 0; i < EP_CTX_PER_DEV; i++)
> 			sb->eps[i] = NULL;
> 	}
>
>
> The complete code modification is as follows:
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index af8d4b74c4ba..448d28aaff3e 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -15,6 +15,7 @@
>   #include <linux/dmapool.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/bitfield.h>
> +#include <linux/usb/xhci-sideband.h>
>   
>   #include "xhci.h"
>   #include "xhci-trace.h"
> @@ -922,6 +923,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
>   		dev->rhub_port->slot_id = 0;
>   	if (xhci->devs[slot_id] == dev)
>   		xhci->devs[slot_id] = NULL;
> +
> +	if (dev->sideband) {
> +		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
> +			 slot_id);
> +		dev->sideband->vdev = NULL;
> +		dev->sideband = NULL;
> +	}
> +
>   	kfree(dev);
>   }
>   
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index a5deeee4d5dc..f979ce517163 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -472,12 +472,25 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>   
>   	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]);
> +		/*
> +		 * If vdev is NULL, xhci_free_virt_device() has already
> +		 * cleared sb->vdev and freed vdev (e.g. on
> +		 * COMP_USB_TRANSACTION_ERROR during address device
> +		 * recovery). Skip endpoint cleanup as the xHC has already
> +		 * disabled the slot.
> +		 *
> +		 * The interrupter and sideband instance are host-level
> +		 * resources independent of vdev, so still remove and free
> +		 * them to avoid leaks.
> +		 */
> +		if (vdev) {
> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
> +				if (sb->eps[i])
> +					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
> +		} else {
> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
> +				sb->eps[i] = NULL;
> +		}
>   
>   		__xhci_sideband_remove_interrupter(sb);
>   
> @@ -486,7 +499,8 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>   
>   	spin_lock_irq(&xhci->lock);
>   	sb->xhci = NULL;
> -	vdev->sideband = NULL;
> +	if (vdev)
> +		vdev->sideband = NULL;
>   	spin_unlock_irq(&xhci->lock);
>   
>   	kfree(sb);

Looks good to me.

Thanks,
Selva


>
> Thanks
> Lianqin

Re: 答复: 答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Mathias Nyman]

On 9/11/26 11:41, Selvarasu Ganesan wrote:
> 
> On 9/11/2026 12:59 PM, 胡连勤 wrote:
>> Hi Selva,
>>
>>>>     drivers/usb/host/xhci-mem.c      |  8 ++++++++
>>>>     drivers/usb/host/xhci-sideband.c | 25 ++++++++++++++++++-------
>>>>     2 files changed, 26 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>>>> index af8d4b74c4ba..afdcfb38b35f 100644
>>>> --- a/drivers/usb/host/xhci-mem.c
>>>> +++ b/drivers/usb/host/xhci-mem.c
>>>> @@ -922,6 +922,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
>>>>     		dev->rhub_port->slot_id = 0;
>>>>     	if (xhci->devs[slot_id] == dev)
>>>>     		xhci->devs[slot_id] = NULL;
>>>> +
>>>> +	if (dev->sideband) {
>>>> +		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
>>>> +			  slot_id);
>>>> +		dev->sideband->vdev = NULL;
>>> Thanks for your updated patch.
>>>
>>> Dont forget to add #include <linux/usb/xhci-sideband.h> in this
>>> xhci-mem.c file otherwise getting below error,
>>>
>>> drivers/usb/host/xhci-mem.c:928:30: error: invalid use of undefined type
>>> ‘struct xhci_sideband’
>>>        928 |                 dev->sideband->vdev = NULL;
>>>
>> Include the corresponding header file
>> #include <linux/dmapool.h>
>>    #include <linux/dma-mapping.h>
>>    #include <linux/bitfield.h>
>> +#include <linux/usb/xhci-sideband.h>
>>    
>>
>>>> +		dev->sideband = NULL;
>>>> +	}
>>>> +
>>>>     	kfree(dev);
>>>>     }
>>>>
>>>> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
>>>> index a5deeee4d5dc..6312c9e3af65 100644
>>>> --- a/drivers/usb/host/xhci-sideband.c
>>>> +++ b/drivers/usb/host/xhci-sideband.c
>>>> @@ -472,12 +472,22 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>>>>
>>>>     	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]);
>>>> +		/*
>>>> +		 * If vdev is NULL, xhci_free_virt_device() has already
>>>> +		 * cleared sb->vdev and freed vdev (e.g. on
>>>> +		 * COMP_USB_TRANSACTION_ERROR during address device
>>>> +		 * recovery). Skip endpoint cleanup as the xHC has already
>>>> +		 * disabled the slot.
>>>> +		 *
>>>> +		 * The interrupter and sideband instance are host-level
>>>> +		 * resources independent of vdev, so still remove and free
>>>> +		 * them to avoid leaks.
>>>> +		 */
>>>> +		if (vdev) {
>>>> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
>>>> +				if (sb->eps[i])
>>>> +					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
>>> I have one query on  skip endpoint cleanup due to  vdev is NULL, in this
>>> case the pointers in sb->eps are not cleared. Since these pointers point
>>> into the virtual device eps, any subsequent call to sideband API
>>> functions like xhci_sideband_get_endpoint_buffer() that dereference
>>> sb->eps could result in a use after free if the virtual device has been
>>> freed. Is it possible?

Good point, endpoint use after free is a much bigger and earlier issue here.

The endpoint rings that audio driver is accessing via sideband are freed and
reallocated much earlier. Audio driver is unaware of this reset, and may still
try to access the freed ring buffers.
This is an issue long before xhci_sideband_unregister() is called.

usb_reset_and_verify_device()
   hub_port_init()  // resets port
   usb_hcd_alloc_bandwidth(udev, udev->actconfig, NULL, NULL);
     hcd->driver->drop_endpoint()  // for all endpoints, xhci tags ep to be dropped
     hcd->driver->add_endpoint()   // for active endpoints. xhci allocs new ring for ep
     hcd->driver->check_bandwidth(hcd, udev) // xhci frees old ring and takes new ring into use
     
So turns out setting vdev->sideband->vdev to NULL in xhci_free_virt_dev(), and
reacting to it in xhci_sideband_unregister() is too little too late.

I think wee need to look at using drv->pre_reset and drv->post_reset
to unregister and re-register sideband, or optionally to unbind and rebind
the whole interface.

Thanks
Mathias

答复: 答复: 答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[胡连勤]

Hi Mathias,

> >>>> +		if (vdev) {
> >>>> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
> >>>> +				if (sb->eps[i])
> >>>> +					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
> >>> I have one query on  skip endpoint cleanup due to  vdev is NULL, in this
> >>> case the pointers in sb->eps are not cleared. Since these pointers point
> >>> into the virtual device eps, any subsequent call to sideband API
> >>> functions like xhci_sideband_get_endpoint_buffer() that dereference
> >>> sb->eps could result in a use after free if the virtual device has been
> >>> freed. Is it possible?
> 
> Good point, endpoint use after free is a much bigger and earlier issue here.
> 
> The endpoint rings that audio driver is accessing via sideband are freed and
> reallocated much earlier. Audio driver is unaware of this reset, and may still
> try to access the freed ring buffers.
> This is an issue long before xhci_sideband_unregister() is called.
> 
> usb_reset_and_verify_device()
>    hub_port_init()  // resets port
>    usb_hcd_alloc_bandwidth(udev, udev->actconfig, NULL, NULL);
>      hcd->driver->drop_endpoint()  // for all endpoints, xhci tags ep to be dropped
>      hcd->driver->add_endpoint()   // for active endpoints. xhci allocs new ring for ep
>      hcd->driver->check_bandwidth(hcd, udev) // xhci frees old ring and takes new ring into use
> 
> So turns out setting vdev->sideband->vdev to NULL in xhci_free_virt_dev(), and
> reacting to it in xhci_sideband_unregister() is too little too late.
> 
> I think wee need to look at using drv->pre_reset and drv->post_reset
> to unregister and re-register sideband, or optionally to unbind and rebind
> the whole interface.
> 

Thanks for the detailed analysis. You're absolutely right - the v2
patch is "too little too late".

I verified that the USB audio driver (sound/usb/) does NOT implement
pre_reset/post_reset callbacks. So during usb_reset_device(), the
USB core will unbind and rebind the entire interface (hub.c line
6411-6417):

    if (drv->pre_reset && drv->post_reset)
        unbind = (drv->pre_reset)(cintf);
    else if (cintf->condition == USB_INTERFACE_BOUND)
        unbind = 1;                    // <-- audio driver hits this path
    if (unbind)
        usb_forced_unbind_intf(cintf);

This means the audio driver's disconnect() should be called before
the reset happens, which should trigger xhci_sideband_unregister()
before the rings are freed.

But the crash trace shows xhci_sideband_unregister() is called from
uaudio_disconnect() AFTER the device has already been freed. This
suggests the disconnect() -> xhci_sideband_unregister() path is not
completing before xhci frees the vdev.

Looking at the crash trace again:

  hub_event()
    xhci_setup_device()                  <-- COMP_USB_TRANSACTION_ERROR
    xhci_disable_and_free_slot()          <-- frees vdev here
      xhci_free_virt_device()
    ...
    usb_disconnect()                      <-- disconnect happens AFTER
      uaudio_disconnect()
        xhci_sideband_unregister()        <-- CRASH (vdev already freed)

The issue is that xhci_setup_device() failure triggers
xhci_disable_and_free_slot() directly, which frees vdev before
usb_disconnect() has a chance to call the driver's disconnect().

So the real question is: should xhci_setup_device() failure path
defer the vdev free until after usb_disconnect() has cleaned up
the sideband?

Or should we ensure that when a sideband client registers, the
audio driver's disconnect() properly calls xhci_sideband_unregister()
before returning, so that the sideband is always cleaned up before
xhci frees vdev?

I agree that drv->pre_reset/post_reset is the clean solution for
the usb_reset_device() path, but the crash happens in the
xhci_setup_device() failure path which doesn't go through
pre_reset/post_reset.

Would you like me to:

1. Keep v2 as a defensive fix (prevents crash), and work on a
   separate patch to add pre_reset/post_reset to the audio driver
   for the reset path?

2. Or focus on fixing the xhci_setup_device() failure path to
   ensure sideband is cleaned up before vdev is freed?

Thanks,
Lianqin

Re: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Michal Pecio]

On Fri, 11 Sep 2026 16:10:04 +0300, Mathias Nyman wrote:
> Good point, endpoint use after free is a much bigger and earlier
> issue here.
> 
> The endpoint rings that audio driver is accessing via sideband are
> freed and reallocated much earlier. Audio driver is unaware of this
> reset, and may still try to access the freed ring buffers.
> This is an issue long before xhci_sideband_unregister() is called.
> 
> usb_reset_and_verify_device()
>    hub_port_init()  // resets port
>    usb_hcd_alloc_bandwidth(udev, udev->actconfig, NULL, NULL);
>      hcd->driver->drop_endpoint()  // for all endpoints, xhci tags ep to be dropped
>      hcd->driver->add_endpoint()   // for active endpoints. xhci allocs new ring for ep
>      hcd->driver->check_bandwidth(hcd, udev) // xhci frees old ring and takes new ring into use
>      
> So turns out setting vdev->sideband->vdev to NULL in
> xhci_free_virt_dev(), and reacting to it in
> xhci_sideband_unregister() is too little too late.

To be exact, such reset of current configuration only happens after
successful hub_port_init(), which requires successful hub_port_reset(),
which at least attempts to call hcd->driver->reset_device(), which is
xhci_discover_or_reset_device().

This already deallocates transfer rings and includes a callback to
sideband client to synchronize. Current implementation in QC seems to
command the HW to stop using affected endpoint(s), so the most obvious
and blatant kind of UAF is meant not to happen.

Maybe this could be extended to unregister the sideband right there,
but not sure what happens if hub_port_init() fails without us knowing.

> I think wee need to look at using drv->pre_reset and drv->post_reset
> to unregister and re-register sideband, or optionally to unbind and
> rebind the whole interface.

That's another opportunity to get rid of sideband users.

It doesn't cover usb_reset_and_verify_device() called in reset-resume,
but clients should usb_offload_get() to prevent suspend.

It doesn't cover hub_port_reset() called by port_event() for SuperSpeed
devices, not sure what that is and whether it's dangerous. I noted that
the original patch talks about hub_event(), but maybe it's a mistake?

Regards,
Michal

答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[胡连勤]

Hi Michal,

> On Fri, 11 Sep 2026 16:10:04 +0300, Mathias Nyman wrote:
> > Good point, endpoint use after free is a much bigger and earlier
> > issue here.
> >
> > The endpoint rings that audio driver is accessing via sideband are
> > freed and reallocated much earlier. Audio driver is unaware of this
> > reset, and may still try to access the freed ring buffers.
> > This is an issue long before xhci_sideband_unregister() is called.
> >
> > usb_reset_and_verify_device()
> >    hub_port_init()  // resets port
> >    usb_hcd_alloc_bandwidth(udev, udev->actconfig, NULL, NULL);
> >      hcd->driver->drop_endpoint()  // for all endpoints, xhci tags ep to be dropped
> >      hcd->driver->add_endpoint()   // for active endpoints. xhci allocs new ring for ep
> >      hcd->driver->check_bandwidth(hcd, udev) // xhci frees old ring and takes new ring into use
> >
> > So turns out setting vdev->sideband->vdev to NULL in
> > xhci_free_virt_dev(), and reacting to it in
> > xhci_sideband_unregister() is too little too late.
> 
> To be exact, such reset of current configuration only happens after
> successful hub_port_init(), which requires successful hub_port_reset(),
> which at least attempts to call hcd->driver->reset_device(), which is
> xhci_discover_or_reset_device().
> 
> This already deallocates transfer rings and includes a callback to
> sideband client to synchronize. Current implementation in QC seems to
> command the HW to stop using affected endpoint(s), so the most obvious
> and blatant kind of UAF is meant not to happen.
> 
> Maybe this could be extended to unregister the sideband right there,
> but not sure what happens if hub_port_init() fails without us knowing.
> 
> > I think wee need to look at using drv->pre_reset and drv->post_reset
> > to unregister and re-register sideband, or optionally to unbind and
> > rebind the whole interface.
> 
> That's another opportunity to get rid of sideband users.
> 
> It doesn't cover usb_reset_and_verify_device() called in reset-resume,
> but clients should usb_offload_get() to prevent suspend.
> 
> It doesn't cover hub_port_reset() called by port_event() for SuperSpeed
> devices, not sure what that is and whether it's dangerous. I noted that
> the original patch talks about hub_event(), but maybe it's a mistake?
> 

Thanks for the analysis. A clarification on the hub_event() reference
in my patch:

The crash trace shows hub_event() at the top because that's the actual
crash call stack from the failing device. The full sequence is:

  hub_event()
    -> port_event()                           [hub.c:5966]
       -> usb_reset_device(udev)              [hub.c:5875]
          -> usb_reset_and_verify_device()    [hub.c:6183]
             -> hub_port_init()               [hub.c:6228]
                -> hcd->driver->address_device()  [hub.c:4781]
                   -> xhci_setup_device()     <-- COMP_USB_TRANSACTION_ERROR
                      -> xhci_disable_and_free_slot()  [xhci.c:4438]
                         -> xhci_free_virt_device()   <-- frees vdev here

This is the "do warm reset, full device" branch in port_event()
(hub.c:5875), which calls usb_reset_device() ― different from the
hub_port_reset(hub, port1, NULL, ...) call at hub.c:5865 which handles
the port-only warm reset case (no udev).

So hub_event() in the trace is correct ― it's the workqueue entry point
that dispatches to port_event().

Regarding your point about xhci_discover_or_reset_device() already
deallocating rings with a sideband callback ― to clarify, that's the
hcd->driver->reset_device() path called from hub_port_reset() at
hub.c:3188. My crash path goes through xhci_setup_device() instead,
which is called from hub_port_init() via hcd->driver->address_device()
and doesn't go through xhci_discover_or_reset_device(). So the sideband
callback you mentioned wouldn't apply to this particular path.

Lianqin
Thanks

Re: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Michal Pecio]

On Mon, 14 Sep 2026 07:04:17 +0000, 胡连勤 wrote:
> > It doesn't cover hub_port_reset() called by port_event() for
> > SuperSpeed devices, not sure what that is and whether it's
> > dangerous. I noted that the original patch talks about hub_event(),
> > but maybe it's a mistake? 
> 
> Thanks for the analysis. A clarification on the hub_event() reference
> in my patch:
> 
> The crash trace shows hub_event() at the top because that's the actual
> crash call stack from the failing device. The full sequence is:
> 
>   hub_event()
>     -> port_event()                           [hub.c:5966]
>        -> usb_reset_device(udev)              [hub.c:5875]
>           -> usb_reset_and_verify_device()    [hub.c:6183]
>              -> hub_port_init()               [hub.c:6228]
>                 -> hcd->driver->address_device()  [hub.c:4781]
>                    -> xhci_setup_device()     <-- COMP_USB_TRANSACTION_ERROR
>                       -> xhci_disable_and_free_slot()  [xhci.c:4438]
>                          -> xhci_free_virt_device()   <-- frees vdev here  

So not a mistake and this is indeed a dangerous case. And AFAICT, in
this path udev's pre_reset() routine isn't called and therefore can't
be used to fix your issue, unless USB core is patched to call it.

But xhci_discover_or_reset_device() is called: before hub_port_init()
calls problematic hub_enable_device() / hub_address_device() functions,
it calls hub_port_reset(), which calls hcd->driver->reset_device().

But I'm not entirely sure what happens if reset fails before this call
is made and then hub_port_init() jumps to re_enumerate. The function
bails out, but sooner or later somebody will try to free this device in
some manner, I suppose, so what happens then?

Regards,
Michal

答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[胡连勤]

Hi Michal,

> > Thanks for the analysis. A clarification on the hub_event() reference
> > in my patch:
> >
> > The crash trace shows hub_event() at the top because that's the actual
> > crash call stack from the failing device. The full sequence is:
> >
> >   hub_event()
> >     -> port_event()                           [hub.c:5966]
> >        -> usb_reset_device(udev)              [hub.c:5875]
> >           -> usb_reset_and_verify_device()    [hub.c:6183]
> >              -> hub_port_init()               [hub.c:6228]
> >                 -> hcd->driver->address_device()  [hub.c:4781]
> >                    -> xhci_setup_device()     <-- COMP_USB_TRANSACTION_ERROR
> >                       -> xhci_disable_and_free_slot()  [xhci.c:4438]
> >                          -> xhci_free_virt_device()   <-- frees vdev here
> 
> So not a mistake and this is indeed a dangerous case. And AFAICT, in
> this path udev's pre_reset() routine isn't called and therefore can't
> be used to fix your issue, unless USB core is patched to call it.
> 
Actually it is. The crash path goes through usb_reset_device()
(hub.c:5875), which calls drv->pre_reset() at hub.c:6412 before
usb_reset_and_verify_device(). So pre_reset/post_reset is viable —
if the sideband client is a USB interface driver.

> But xhci_discover_or_reset_device() is called: before hub_port_init()
> calls problematic hub_enable_device() / hub_address_device() functions,
> it calls hub_port_reset(), which calls hcd->driver->reset_device().
> 
> But I'm not entirely sure what happens if reset fails before this call
> is made and then hub_port_init() jumps to re_enumerate. The function
> bails out, but sooner or later somebody will try to free this device in
> some manner, I suppose, so what happens then?
> 
By the time we reach re_enumerate (hub.c:6235), xhci_setup_device()
has already freed vdev+rings via xhci_disable_and_free_slot()
(xhci.c:4438) without notifying sideband. The later xhci_free_dev()
is a no-op because xhci->devs[slot_id] is already NULL. So the
dangerous window is between xhci_discover_or_reset_device() (with
callback) and xhci_setup_device() failure (without callback).
Given pre_reset() is available, the proposed fix:

1. Sideband client implements pre_reset() to unregister and stop
    ring access before reset.
2. Add a sideband callback in xhci_free_virt_device() for defense
    in depth.

Does this approach look acceptable to you?

Regards,
Lianqin

Re: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Mathias Nyman]

On 9/14/26 12:09, Michal Pecio wrote:
> On Mon, 14 Sep 2026 07:04:17 +0000, 胡连勤 wrote:
>>> It doesn't cover hub_port_reset() called by port_event() for
>>> SuperSpeed devices, not sure what that is and whether it's
>>> dangerous. I noted that the original patch talks about hub_event(),
>>> but maybe it's a mistake?
>>
>> Thanks for the analysis. A clarification on the hub_event() reference
>> in my patch:
>>
>> The crash trace shows hub_event() at the top because that's the actual
>> crash call stack from the failing device. The full sequence is:
>>
>>    hub_event()
>>      -> port_event()                           [hub.c:5966]
>>         -> usb_reset_device(udev)              [hub.c:5875]
>>            -> usb_reset_and_verify_device()    [hub.c:6183]
>>               -> hub_port_init()               [hub.c:6228]
>>                  -> hcd->driver->address_device()  [hub.c:4781]
>>                     -> xhci_setup_device()     <-- COMP_USB_TRANSACTION_ERROR
>>                        -> xhci_disable_and_free_slot()  [xhci.c:4438]
>>                           -> xhci_free_virt_device()   <-- frees vdev here
> 
> So not a mistake and this is indeed a dangerous case. And AFAICT, in
> this path udev's pre_reset() routine isn't called and therefore can't
> be used to fix your issue, unless USB core is patched to call it.
> 
> But xhci_discover_or_reset_device() is called: before hub_port_init()
> calls problematic hub_enable_device() / hub_address_device() functions,
> it calls hub_port_reset(), which calls hcd->driver->reset_device().

To me it looks like both drv->pre_reset and xhci_discover_or_reset_device()
are called in this path.

hub_event()
   port_event()
     usb_reset_device(udev)
       if (config) //and for each interface in this config: , for each interface)
         if (cintf->dev.driver) {
           drv = to_usb_driver(cintf->dev.driver);
           if (drv->pre_reset && drv->post_reset)
           unbind = (drv->pre_reset)(cintf);


Any chance you could trace the whole call path in more details and see exactly which path
is staken before the crash.

port_event() should only call usb_reset_device() for usb3 devices with link stuck in
ss.inactive for longer than ~100ms.  Is this really a usb3 audio device?

Thanks
Mathias

答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[胡连勤]

Hi Mathias,

> >
> > But xhci_discover_or_reset_device() is called: before hub_port_init()
> > calls problematic hub_enable_device() / hub_address_device() functions,
> > it calls hub_port_reset(), which calls hcd->driver->reset_device().
> 
> To me it looks like both drv->pre_reset and xhci_discover_or_reset_device()
> are called in this path.
> 
Your code tracing is correct. drv->pre_reset() IS called at
hub.c:6412 before usb_reset_and_verify_device(), and
xhci_discover_or_reset_device() IS called via hub_port_reset() →
hcd->driver->reset_device() inside hub_port_init().

However, this path is not the actual crash path. I apologize —
my earlier call chain referencing usb_reset_device() was an
assumption, not from the actual crash dump.


> hub_event()
>    port_event()
>      usb_reset_device(udev)
>        if (config) //and for each interface in this config: , for each interface)
>          if (cintf->dev.driver) {
>            drv = to_usb_driver(cintf->dev.driver);
>            if (drv->pre_reset && drv->post_reset)
>            unbind = (drv->pre_reset)(cintf);
> 
> 
> Any chance you could trace the whole call path in more details and see exactly which path
> is staken before the crash.
> 
> port_event() should only call usb_reset_device() for usb3 devices with link stuck in
> ss.inactive for longer than ~100ms.  Is this really a usb3 audio device?
>
No, it is not. The device is a full-speed Apple EarPods
(USB 1.1):

  usb 1-1: new full-speed USB device number 2 using xhci-hcd
  usb 1-1: Product: EarPods
  usb 1-1: New USB device found, idVendor=05ac, idProduct=110b

So the port_event() → usb_reset_device() warm-reset path for
SS.Inactive does not apply here.
The actual crash path is through usb_disconnect(), not
usb_reset_device(). The full sequence from dmesg + crash trace:

1. xhci_setup_device() gets COMP_USB_TRANSACTION_ERROR
("Device not responding to setup address")

2. xhci_disable_and_free_slot() → xhci_free_virt_device()
frees the entire vdev struct (subsequently SLAB-poisoned),
but does NOT clear sb->vdev

3. Later: usb_disconnect() → usb_audio_disconnect()
→ uaudio_disconnect() → xhci_sideband_unregister()
→ xhci_stop_endpoint_sync() → xhci_get_ep_ctx()
← CRASH: dereferences vdev->out_ctx, reads SLAB poison
value 0xdead000000000122
(0x100 = SLAB_POISON base, 0x22 = out_ctx field offset)

Crash call trace:
  xhci_get_ep_ctx+0x0/0x38
  xhci_sideband_unregister+0x68/0xf0
  uaudio_disconnect+0x70/0x144
  usb_audio_disconnect+0x7c/0x268
  usb_unbind_interface+0x13c/0x340
  device_release_driver_internal+0x1c4/0x2bc
  device_release_driver+0x18/0x28
  bus_remove_device+0x158/0x170
  device_del+0x1c8/0x320
  usb_disable_device+0x84/0x190
  usb_disconnect+0xe8/0x338
  hub_event+0xbd8/0x19ac

So both drv->pre_reset and xhci_discover_or_reset_device() are
called as you traced, but they are on the reset path — the crash
happens on the disconnect path.

The root cause is that xhci_free_virt_device() frees vdev without
clearing the sideband's dangling pointer. The fix (patch
2482d78a7813) clears sb->vdev = NULL in xhci_free_virt_device()
before freeing vdev, so xhci_sideband_unregister() can detect the
already-freed vdev and skip endpoint cleanup.

Regards,
Lianqin

Re: 答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Mathias Nyman]

On 9/14/26 15:24, 胡连勤 wrote:
> Hi Michal,
> 
>>> Thanks for the analysis. A clarification on the hub_event() reference
>>> in my patch:
>>>
>>> The crash trace shows hub_event() at the top because that's the actual
>>> crash call stack from the failing device. The full sequence is:
>>>
>>>    hub_event()
>>>      -> port_event()                           [hub.c:5966]
>>>         -> usb_reset_device(udev)              [hub.c:5875]
>>>            -> usb_reset_and_verify_device()    [hub.c:6183]
>>>               -> hub_port_init()               [hub.c:6228]
>>>                  -> hcd->driver->address_device()  [hub.c:4781]
>>>                     -> xhci_setup_device()     <-- COMP_USB_TRANSACTION_ERROR
>>>                        -> xhci_disable_and_free_slot()  [xhci.c:4438]
>>>                           -> xhci_free_virt_device()   <-- frees vdev here
>>
>> So not a mistake and this is indeed a dangerous case. And AFAICT, in
>> this path udev's pre_reset() routine isn't called and therefore can't
>> be used to fix your issue, unless USB core is patched to call it.
>>
> Actually it is. The crash path goes through usb_reset_device()
> (hub.c:5875), which calls drv->pre_reset() at hub.c:6412 before
> usb_reset_and_verify_device(). So pre_reset/post_reset is viable —
> if the sideband client is a USB interface driver.
> 
>> But xhci_discover_or_reset_device() is called: before hub_port_init()
>> calls problematic hub_enable_device() / hub_address_device() functions,
>> it calls hub_port_reset(), which calls hcd->driver->reset_device().
>>
>> But I'm not entirely sure what happens if reset fails before this call
>> is made and then hub_port_init() jumps to re_enumerate. The function
>> bails out, but sooner or later somebody will try to free this device in
>> some manner, I suppose, so what happens then?
>>
> By the time we reach re_enumerate (hub.c:6235), xhci_setup_device()
> has already freed vdev+rings via xhci_disable_and_free_slot()
> (xhci.c:4438) without notifying sideband. The later xhci_free_dev()
> is a no-op because xhci->devs[slot_id] is already NULL. So the
> dangerous window is between xhci_discover_or_reset_device() (with
> callback) and xhci_setup_device() failure (without callback).
> Given pre_reset() is available, the proposed fix:
> 
> 1. Sideband client implements pre_reset() to unregister and stop
>      ring access before reset.

Sounds good, call xhci_sideband_remove_endpoint() for every offloaded endpoint.
If possible then maybe even unregister sideband for this device completely here.


> 2. Add a sideband callback in xhci_free_virt_device() for defense
>      in depth.

Selvarasu Ganesan pointed out that xhci 'core' in fact doesn't include
xhci-sideband.h yet. If possible I'd like to keep it that way.

Setting xhci->sideband->vdev to NULL, or calling a callback here changes this
and is the first time we then intertwine xhci core with sideband.

Long term solution is to not reallocate vdev just because we try to disable and
re-enable the slot to recover from a failed address device command.
Usb core doesn't free and reallocate udev during device reset either.

Niklas just started looking at decoupling vdev allocation and initalization.
Meanwhile we could try a bandaid like:

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a9e47e178c28..0b5152a1a301 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4435,11 +4435,15 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
  		dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
  
  		mutex_unlock(&xhci->mutex);
-		ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
-		if (!ret) {
-			if (xhci_alloc_dev(hcd, udev) == 1)
-				xhci_setup_addressable_virt_dev(xhci, udev);
+
+		if (!virt_dev->sideband) {
+			ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
+			if (!ret) {
+				if (xhci_alloc_dev(hcd, udev) == 1)
+					xhci_setup_addressable_virt_dev(xhci, udev);
+			}
  		}
+
  		kfree(command->completion);
  		kfree(command);
  		return -EPROTO;

Does this work in your case?
Can you see any negative side-effects with this solution like never re-enumerating and
recovering after a failed address device command?

Thanks
Mathias

Re: 答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Mathias Nyman]

On 9/14/26 16:00, 胡连勤 wrote:
> Hi Mathias,
> 
>>>
>>> But xhci_discover_or_reset_device() is called: before hub_port_init()
>>> calls problematic hub_enable_device() / hub_address_device() functions,
>>> it calls hub_port_reset(), which calls hcd->driver->reset_device().
>>
>> To me it looks like both drv->pre_reset and xhci_discover_or_reset_device()
>> are called in this path.
>>
> Your code tracing is correct. drv->pre_reset() IS called at
> hub.c:6412 before usb_reset_and_verify_device(), and
> xhci_discover_or_reset_device() IS called via hub_port_reset() →
> hcd->driver->reset_device() inside hub_port_init().
> 
> However, this path is not the actual crash path. I apologize —
> my earlier call chain referencing usb_reset_device() was an
> assumption, not from the actual crash dump.
> 
> 
>> hub_event()
>>     port_event()
>>       usb_reset_device(udev)
>>         if (config) //and for each interface in this config: , for each interface)
>>           if (cintf->dev.driver) {
>>             drv = to_usb_driver(cintf->dev.driver);
>>             if (drv->pre_reset && drv->post_reset)
>>             unbind = (drv->pre_reset)(cintf);
>>
>>
>> Any chance you could trace the whole call path in more details and see exactly which path
>> is staken before the crash.
>>
>> port_event() should only call usb_reset_device() for usb3 devices with link stuck in
>> ss.inactive for longer than ~100ms.  Is this really a usb3 audio device?
>>
> No, it is not. The device is a full-speed Apple EarPods
> (USB 1.1):
> 
>    usb 1-1: new full-speed USB device number 2 using xhci-hcd
>    usb 1-1: Product: EarPods
>    usb 1-1: New USB device found, idVendor=05ac, idProduct=110b
> 
> So the port_event() → usb_reset_device() warm-reset path for
> SS.Inactive does not apply here.
> The actual crash path is through usb_disconnect(), not
> usb_reset_device(). The full sequence from dmesg + crash trace:

Thanks for clarifying, the crash part is now clear.

What is still unclear is the parts before this.
How do we end up in a situation where we are resetting and addressing
a device with sideband still registered.

What codepath ends up calling the futile

hub_port_init()
   ->hub_address_device()

that ends up freeing and reallocating the xhci vdev?

Is there a way (like drv->pre_reset call) among that path to
inform audio driver to unregister sideband?

Thanks
Mathias

Re: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Michal Pecio]

On Mon, 14 Sep 2026 13:00:40 +0000, 胡连勤 wrote:
> > > But xhci_discover_or_reset_device() is called: before
> > > hub_port_init() calls problematic hub_enable_device() /
> > > hub_address_device() functions, it calls hub_port_reset(),
> > > which calls hcd->driver->reset_device().  
> > 
> > To me it looks like both drv->pre_reset and
> > xhci_discover_or_reset_device() are called in this path.

Yes, you are right. I mistakenly looked at the "warm reset, port only"
case but there seems to be no possibility of falling into this path.

BTW, SuperSpeed audio devices do exist. Basically, any audio function
in a larger device which needs the bandwidth. Similar thing with HID.

> Your code tracing is correct. drv->pre_reset() IS called at
> hub.c:6412 before usb_reset_and_verify_device(), and
> xhci_discover_or_reset_device() IS called via hub_port_reset() →
> hcd->driver->reset_device() inside hub_port_init().
> 
> However, this path is not the actual crash path. I apologize —
> my earlier call chain referencing usb_reset_device() was an
> assumption, not from the actual crash dump.

Note that xhci_setup_device() is involved, which is called from
hub_enable_device() and hub_set_address(), and these are called
from hub_port_init(), which is used by hub_port_connect() and
usb_reset_and_verify_device(). So something happens there.

Is this reproducible? Then add to xhci_free_virt_device():

	if (dev->sideband)
		dump_stack()

and we will know how it happens.

Regards,
Michal

Re: 答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Mathias Nyman]

>> 2. Add a sideband callback in xhci_free_virt_device() for defense
>>      in depth.
> 
> Selvarasu Ganesan pointed out that xhci 'core' in fact doesn't include
> xhci-sideband.h yet. If possible I'd like to keep it that way.
> 
> Setting xhci->sideband->vdev to NULL, or calling a callback here changes this
> and is the first time we then intertwine xhci core with sideband.
> 

Disregard the above, I forgot about the xhci_sideband_notify_ep_ring_free() call
in xhci_discover_or_reset_device().

Rest of comment is still valid.

Thanks
Mathias

Re: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Michal Pecio]

On Mon, 14 Sep 2026 17:06:35 +0300, Mathias Nyman wrote:
> > Selvarasu Ganesan pointed out that xhci 'core' in fact doesn't
> > include xhci-sideband.h yet. If possible I'd like to keep it that
> > way.
> > 
> > Setting xhci->sideband->vdev to NULL, or calling a callback here
> > changes this and is the first time we then intertwine xhci core
> > with sideband. 
> 
> Disregard the above, I forgot about the
> xhci_sideband_notify_ep_ring_free() call in
> xhci_discover_or_reset_device().

You aren't entirely wrong, xhci_sideband_notify_ep_ring_free() isn't
useful to sideband clients so it could be declared in "xhci.h" instead
of <xhci-sideband.h>. Then xhci.c won't need the public API header.

This function is also exported unnecessarily. Another unused export is
xhci_stop_endpoint_sync(), because sideband hasn't become new module.

Regards,
Michal