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

[Mathias Nyman]

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@xxxxxxxxxxxxxxx
> Signed-off-by: Lianqin Hu <hulianqin@xxxxxxxx>

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()

Thanks
Mathias