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

[Selvarasu Ganesan <selvarasu.g@samsung.com>]

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