答复: [PATCH v2] usb: xhci: clear dangling sideband pointer in xhci_free_virt_device()

[胡连勤]

Hi Michal,

Thanks for the detailed review.

> > xhci_free_virt_device() must not leave any dangling pointers.
> > If vdev->sideband is still set at this point then something is
> > wrong, e.g. the sideband client did not unregister before the
> > virtual device was freed. This can happen when
> > xhci_setup_device() gets COMP_USB_TRANSACTION_ERROR (device not
> > responding to setup address during bus reset recovery), causing
> > xhci_disable_and_free_slot() -> xhci_free_virt_device() to free
> > vdev before the sideband client has a chance to unregister.
> 
> When and how is the sideband client driver supposed to learn that
> its device has been reset?
> 
> There is some code in xhci_discover_or_reset_device() which sends
> notification that endpoints have been removed. Does it run before
> or after vdev can potentially be freed?

You're absolutely right. Looking at the code:

xhci_discover_or_reset_device() notifies sideband BEFORE freeing:
  line 4074: xhci_sideband_notify_ep_ring_free(ep->sideband, i);
  line 4077: xhci_free_endpoint_ring(xhci, virt_dev, i);

So the sideband client gets a chance to release its ring references
before they are freed. This is the BEFORE case.

But xhci_setup_device() on COMP_USB_TRANSACTION_ERROR takes a different
path (line 4434-4445):
  - xhci_disable_and_free_slot()
    - xhci_free_virt_device()
      - kfree(out_ctx), kfree(vdev)
  - No sideband notification at all

This is the AFTER case you described - vdev is freed without notifying
the sideband client, breaking the notification mechanism itself.

> 
> BEFORE: it seems we had an opportunity to get rid of the sideband
> completely before running into trouble here.
> 
> AFTER: freeing vdev will break those notifications, is it a bug?
> 
> The suggestion by Mathias that sideband should be fully destroyed
> by the client *before* USB core begins reset doesn't look bad.

Agreed. The root cause is that the sideband client is unaware of the
device reset/free in the COMP_USB_TRANSACTION_ERROR path.

This v2 patch is a defensive fix to prevent the immediate crash
(NULL pointer dereference in xhci_sideband_unregister()). It doesn't
address the root cause.

For the complete fix, I think we have two options:

1. In xhci_setup_device() COMP_USB_TRANSACTION_ERROR path, add
   sideband notification before calling xhci_disable_and_free_slot().
   Unlike xhci_discover_or_reset_device() which iterates endpoints
   and calls xhci_sideband_notify_ep_ring_free() before freeing
   rings (line 4062-4077), the TRANSACTION_ERROR path skips this
   step entirely and goes straight to xhci_free_virt_device().

2. As Mathias suggested, use drv->pre_reset/post_reset to unregister
   and re-register sideband around device reset, so the client
   driver handles the lifecycle explicitly.

I'll prepare a follow-up patch for the root cause fix. Would you
or Mathias have a preference between option 1 and 2?

Thanks,
Lianqin