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

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

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