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

Hi Mathias, Selva,

quoted

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