[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