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

[Michal Pecio <michal.pecio@gmail.com>]

On Fri, 11 Sep 2026 10:22:37 +0000, 胡连勤 wrote:
> 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?

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.

> 
>   hub_event()
>     xhci_setup_device()                  <-- COMP_USB_TRANSACTION_ERROR
>     xhci_disable_and_free_slot()
>       xhci_free_virt_device()
>         kfree(out_ctx), kfree(vdev)
>         xhci->devs[slot_id] = NULL
>     ...
>     usb_disconnect()
>       uaudio_disconnect()
>         xhci_sideband_unregister()
>           xhci_stop_endpoint_sync()
>             xhci_get_ep_ctx()            <-- CRASH (deref freed out_ctx)
> 
> Unable to handle kernel paging request at virtual address dead000000000122
> Call trace:
>  xhci_get_ep_ctx+0x0/0x38
>  xhci_sideband_unregister+0x68/0xf0
>  uaudio_disconnect+0x70/0x144
>  usb_audio_disconnect+0x7c/0x268
>  usb_unbind_interface+0x13c/0x340
>  device_release_driver_internal+0x1c4/0x2bc
>  device_release_driver+0x18/0x28
>  bus_remove_device+0x158/0x170
>  device_del+0x1c8/0x320
>  usb_disable_device+0x84/0x190
>  usb_disconnect+0xe8/0x338
>  hub_event+0xbd8/0x19ac
>  process_scheduled_works+0x200/0x9d8
>  worker_thread+0x154/0x3b0
>  kthread+0x11c/0x1a0
> 
> Fix this by clearing any remaining sideband pointer in
> xhci_free_virt_device() before freeing vdev. If vdev->sideband is
> still set, set vdev->sideband->vdev = NULL to break the dangling
> pointer at the source.
> 
> Additionally, in xhci_sideband_unregister(), check sb->vdev before
> issuing stop endpoint commands. If vdev is already NULL (cleared by
> xhci_free_virt_device), skip endpoint cleanup as the xHC has already
> disabled the slot, but still remove the interrupter and free the
> sideband instance to avoid leaks.
> 
> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lianqin Hu <hulianqin@vivo.com>
> ---
> 
> Changes in v2:
>  - Move fix to xhci_free_virt_device() per maintainer suggestion.
>  - Use xhci_dbg per maintainer suggestion.
>  - Ensure interrupter cleanup is unconditional when vdev is NULL.
>  - Clear dangling sb->eps[] when vdev is NULL (per Selva).
>  - Update patch commit message.
>  - Link to v1: https://lore.kernel.org/all/TYUPR06MB6217000B59003EDF233D7246D2B22@TYUPR06MB6217.apcprd06.prod.outlook.com/
> 
>  drivers/usb/host/xhci-mem.c      |  9 +++++++++
>  drivers/usb/host/xhci-sideband.c | 28 +++++++++++++++++++++-------
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index af8d4b74c4ba..448d28aaff3e 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -15,6 +15,7 @@
>  #include <linux/dmapool.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/bitfield.h>
> +#include <linux/usb/xhci-sideband.h>
>  
>  #include "xhci.h"
>  #include "xhci-trace.h"
> @@ -922,6 +923,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
>  		dev->rhub_port->slot_id = 0;
>  	if (xhci->devs[slot_id] == dev)
>  		xhci->devs[slot_id] = NULL;
> +
> +	if (dev->sideband) {

Could be:

  if (IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND) && dev->sideband)

> +		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
> +			 slot_id);
> +		dev->sideband->vdev = NULL;
> +		dev->sideband = NULL;
> +	}
> +
>  	kfree(dev);
>  }
>  
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index a5deeee4d5dc..f979ce517163 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -472,12 +472,25 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>  
>  	scoped_guard(mutex, &sb->mutex) {
>  		vdev = sb->vdev;
> -		if (!vdev)
> -			return;
> -
> -		for (i = 0; i < EP_CTX_PER_DEV; i++)
> -			if (sb->eps[i])
> -				__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
> +		/*
> +		 * If vdev is NULL, xhci_free_virt_device() has already
> +		 * cleared sb->vdev and freed vdev (e.g. on
> +		 * COMP_USB_TRANSACTION_ERROR during address device
> +		 * recovery). Skip endpoint cleanup as the xHC has already
> +		 * disabled the slot.
> +		 *
> +		 * The interrupter and sideband instance are host-level
> +		 * resources independent of vdev, so still remove and free
> +		 * them to avoid leaks.
> +		 */
> +		if (vdev) {
> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
> +				if (sb->eps[i])
> +					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
> +		} else {
> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
> +				sb->eps[i] = NULL;
> +		}
>  
>  		__xhci_sideband_remove_interrupter(sb);
>  
> @@ -486,7 +499,8 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>  
>  	spin_lock_irq(&xhci->lock);
>  	sb->xhci = NULL;
> -	vdev->sideband = NULL;
> +	if (vdev)
> +		vdev->sideband = NULL;
>  	spin_unlock_irq(&xhci->lock);
>  
>  	kfree(sb);
> -- 
> 2.48.1
>