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

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

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