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

[Mathias Nyman]

On 10/2/26 13:12, 胡连勤 wrote:
> Hi Mathias,
>
> > > Sounds good, call xhci_sideband_remove_endpoint() for every offloaded endpoint.
> > > If possible then maybe even unregister sideband for this device completely here.
> > >
> > >
> > > > 2. Add a sideband callback in xhci_free_virt_device() for defense
> > > >       in depth.
> > >
> > > Selvarasu Ganesan pointed out that xhci 'core' in fact doesn't include
> > > xhci-sideband.h yet. If possible I'd like to keep it that way.
> > >
> > > Setting xhci->sideband->vdev to NULL, or calling a callback here changes this
> > > and is the first time we then intertwine xhci core with sideband.
> > >
> > > Long term solution is to not reallocate vdev just because we try to disable and
> > > re-enable the slot to recover from a failed address device command.
> > > Usb core doesn't free and reallocate udev during device reset either.
> > >
> > > Niklas just started looking at decoupling vdev allocation and initalization.
> > > Meanwhile we could try a bandaid like:
> > >
> > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > index a9e47e178c28..0b5152a1a301 100644
> > > --- a/drivers/usb/host/xhci.c
> > > +++ b/drivers/usb/host/xhci.c
> > > @@ -4435,11 +4435,15 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> > >    		dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
> > >
> > >    		mutex_unlock(&xhci->mutex);
> > > -		ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
> > > -		if (!ret) {
> > > -			if (xhci_alloc_dev(hcd, udev) == 1)
> > > -				xhci_setup_addressable_virt_dev(xhci, udev);
> > > +
> > > +		if (!virt_dev->sideband) {
> > > +			ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
> > > +			if (!ret) {
> > > +				if (xhci_alloc_dev(hcd, udev) == 1)
> > > +					xhci_setup_addressable_virt_dev(xhci, udev);
> > > +			}
> > >    		}
> > > +
> > >    		kfree(command->completion);
> > >    		kfree(command);
> > >    		return -EPROTO;
> > >
> > > Does this work in your case?
> > > Can you see any negative side-effects with this solution like never re-enumerating and
> > > recovering after a failed address device command?
> >
> > Thanks for the bandaid. Based on my analysis of the crash path, it
> > should work — skipping xhci_disable_and_free_slot() when sideband
> > is set keeps vdev valid, so the subsequent
> > xhci_sideband_unregister() in the disconnect path won't
> > dereference freed memory. The eventual xhci_free_dev() →
> > xhci_free_virt_device() still cleans up correctly since vdev
> > remains intact.
> >
> > I don't see obvious negative side-effects. The slot stays enabled
> > for retries, but xhci_setup_device() handles the re-address case
> > at xhci.c:4391. For truly broken devices, re_enumerate →
> > disconnect still works since vdev is valid.
> >
> > I'll apply your patch and test it with the crash scenario. Will
> > report back with the results.
> Thank you for the bandaid patch. I tested it and confirmed it resolves
> the crash. The use-after-free no longer occurs when sideband is set.
>
> While discussing this with Wesley Cheng (Qualcomm), he noticed that
> there's another path that could potentially hit the same issue. The
> key difference is how pre_reset() gets invoked:
>
> - usb_reset_device() calls usb_pre_reset(), which invokes the interface
>    driver's pre_reset() callback. If pre_reset() returns 1, the
>    interface is force-unbound, which triggers the class driver's
>    disconnect handler (e.g., uaudio_disconnect() ->
>    xhci_sideband_unregister()). This unregisters the sideband and
>    releases its references to vdev/out_ctx BEFORE xhci_setup_device()
>    is called. So when COMP_USB_TRANSACTION_ERROR later frees
>    vdev/out_ctx, there are no stale sideband references -- no crash.
>
> - finish_port_resume() calls usb_reset_and_verify_device(), which does
>    NOT call usb_pre_reset(). The interface remains bound and the
>    sideband is still registered, still holding references to
>    vdev/out_ctx. If xhci_setup_device() then hits
>    COMP_USB_TRANSACTION_ERROR and frees vdev/out_ctx, the subsequent
>    usb_disconnect() -> uaudio_disconnect() ->
>    xhci_sideband_unregister() -> xhci_stop_endpoint_sync() ->
>    xhci_get_ep_ctx() tries to dereference the already-freed out_ctx --
>    crash.
>
> Wesley suggested an alternative condition that would cover both paths:
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index a9e47e178c28..ff7c29b798fd 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4435,10 +4435,12 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
>                  dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
>   
>                  mutex_unlock(&xhci->mutex);
> -               ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
> -               if (!ret) {
> -                       if (xhci_alloc_dev(hcd, udev) == 1)
> -                               xhci_setup_addressable_virt_dev(xhci, udev);
> +               if (!udev->reset_resume && !udev->reset_in_progress) {
> +                       ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
> +                       if (!ret) {
> +                               if (xhci_alloc_dev(hcd, udev) == 1)
> +                                       xhci_setup_addressable_virt_dev(xhci, udev);
> +                       }
>                  }
>                  kfree(command->completion);
>                  kfree(command);
>
> Instead of checking !virt_dev->sideband, this checks
> !udev->reset_resume && !udev->reset_in_progress, which covers both
> usb_reset_device() and finish_port_resume() paths for all devices.
>
> Both approaches work in our testing. We'd appreciate your guidance on
> which approach to take as the final fix.

I'd probably still go with !virt_dev->sideband

!udev->reset_resume && !udev->reset_in_progress will prevent recovery of 'address
device' failure of ordinary, non-sideband usb devices that need to be reset at resume.

Thanks
Mathias