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

[胡连勤]

Hi Michal,

> > Thanks for the analysis. A clarification on the hub_event() reference
> > in my patch:
> >
> > The crash trace shows hub_event() at the top because that's the actual
> > crash call stack from the failing device. The full sequence is:
> >
> >   hub_event()
> >     -> port_event()                           [hub.c:5966]
> >        -> usb_reset_device(udev)              [hub.c:5875]
> >           -> usb_reset_and_verify_device()    [hub.c:6183]
> >              -> hub_port_init()               [hub.c:6228]
> >                 -> hcd->driver->address_device()  [hub.c:4781]
> >                    -> xhci_setup_device()     <-- COMP_USB_TRANSACTION_ERROR
> >                       -> xhci_disable_and_free_slot()  [xhci.c:4438]
> >                          -> xhci_free_virt_device()   <-- frees vdev here
> 
> So not a mistake and this is indeed a dangerous case. And AFAICT, in
> this path udev's pre_reset() routine isn't called and therefore can't
> be used to fix your issue, unless USB core is patched to call it.
> 
Actually it is. The crash path goes through usb_reset_device()
(hub.c:5875), which calls drv->pre_reset() at hub.c:6412 before
usb_reset_and_verify_device(). So pre_reset/post_reset is viable —
if the sideband client is a USB interface driver.

> But xhci_discover_or_reset_device() is called: before hub_port_init()
> calls problematic hub_enable_device() / hub_address_device() functions,
> it calls hub_port_reset(), which calls hcd->driver->reset_device().
> 
> But I'm not entirely sure what happens if reset fails before this call
> is made and then hub_port_init() jumps to re_enumerate. The function
> bails out, but sooner or later somebody will try to free this device in
> some manner, I suppose, so what happens then?
> 
By the time we reach re_enumerate (hub.c:6235), xhci_setup_device()
has already freed vdev+rings via xhci_disable_and_free_slot()
(xhci.c:4438) without notifying sideband. The later xhci_free_dev()
is a no-op because xhci->devs[slot_id] is already NULL. So the
dangerous window is between xhci_discover_or_reset_device() (with
callback) and xhci_setup_device() failure (without callback).
Given pre_reset() is available, the proposed fix:

1. Sideband client implements pre_reset() to unregister and stop
    ring access before reset.
2. Add a sideband callback in xhci_free_virt_device() for defense
    in depth.

Does this approach look acceptable to you?

Regards,
Lianqin