答复: [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