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

[胡连勤]

Hi Mathias,

> > 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.
> 
> 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.

Regards,
Lianqin