Re: [RFT PATCH v2] xhci: sideband: Fix race condition in sideband unregister

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Oct 28, 2025 at 06:51:53PM +0200, Mathias Nyman wrote:
> Uttkarsh Aggarwal observed a kernel panic during sideband un-register
> and found it was caused by a race condition between sideband unregister,
> and creating sideband interrupters.
> The issue occurrs when thread T1 runs uaudio_disconnect() and released
> sb->xhci via sideband_unregister, while thread T2 simultaneously accessed
> the now-NULL sb->xhci in xhci_sideband_create_interrupter() resulting in
> a crash.
> 
> Ensure new endpoints or interrupter can't be added to a sidenband after
> xhci_sideband_unregister() cleared the existing ones, and unlocked the
> sideband mutex.
> Reorganize code so that mutex is only taken and released once in
> xhci_sideband_unregister(), and clear sb->vdev while mutex is taken.
> 
> Use mutex guards to reduce human unlock errors in code
> 
> Refuse to add endpoints or interrupter if sb->vdev is not set.
> sb->vdev is set when sideband is created and registered.
> 
> Reported-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@oss.qualcomm.com>
> Closes: https://lore.kernel.org/linux-usb/20251028080043.27760-1-uttkarsh.aggarwal@oss.qualcomm.com
> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
> 
> v2:
>   use guard() and fix missing mutex_unlock as recommended by greg k-h 
> 
> ---
>  drivers/usb/host/xhci-sideband.c | 97 +++++++++++++++++---------------
>  1 file changed, 53 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index e771a476fef2..2daa0ba7ad9a 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -86,6 +86,22 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
>  	sb->eps[ep->ep_index] = NULL;
>  }
>  
> +static void
> +__xhci_sideband_remove_interrupter(struct xhci_sideband *sb)

This function must be called with the mutex locked, so shouldn't you
document that here so that the compiler can catch it if we mess up in
the future and forget to grab it?

> +{
> +	struct usb_device *udev;
> +
> +	if (!sb->ir)
> +		return;
> +
> +	xhci_remove_secondary_interrupter(xhci_to_hcd(sb->xhci), sb->ir);
> +	sb->ir = NULL;
> +	udev = sb->vdev->udev;
> +
> +	if (udev->state != USB_STATE_NOTATTACHED)
> +		usb_offload_put(udev);
> +}
> +
>  /* sideband api functions */
>  
>  /**
> @@ -131,14 +147,17 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
>  	struct xhci_virt_ep *ep;
>  	unsigned int ep_index;
>  
> -	mutex_lock(&sb->mutex);
> +	guard(mutex)(&sb->mutex);
> +
> +	if (!sb->vdev)
> +		return -ENODEV;
> +
>  	ep_index = xhci_get_endpoint_index(&host_ep->desc);
>  	ep = &sb->vdev->eps[ep_index];
>  
> -	if (ep->ep_state & EP_HAS_STREAMS) {
> -		mutex_unlock(&sb->mutex);
> +	if (ep->ep_state & EP_HAS_STREAMS)
>  		return -EINVAL;
> -	}
> +

Very minor nit, just delete the extra line, like you did in the rest of
the diff below here, otherwise you have 2 blank lines.

thanks,

greg k-h