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

[Mathias Nyman <mathias.nyman@linux.intel.com>]

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