[PATCH] usb: xhci: Fix sleep in atomic context in xhci_free_streams()

[PATCH] usb: xhci: Fix sleep in atomic context in xhci_free_streams()

[胡连勤]

When a USB device with active stream endpoints is disconnected,
xhci_free_streams() is called from the hub_event workqueue to
free the stream resources while holding xhci->lock with irqs
disabled.

It calls xhci_free_stream_info(), which invokes
xhci_free_stream_ctx(), which in turn calls dma_free_coherent()
for large stream context arrays.

dma_free_coherent() can sleep (e.g. via vunmap), triggering
a BUG when called from atomic context.

Call trace:
 dma_free_attrs+0x174/0x220
 xhci_free_stream_info+0xd0/0x11c
 xhci_free_streams+0x278/0x37c
 usb_free_streams+0x98/0xc0
 usb_unbind_interface+0x1b8/0x2f8
 device_release_driver_internal+0x1d4/0x2cc
 device_release_driver+0x18/0x28
 bus_remove_device+0x160/0x1a4
 device_del+0x1ec/0x350
 usb_disable_device+0x98/0x214
 usb_disconnect+0xf0/0x35c
 hub_event+0xab4/0x19ec
 process_one_work+0x278/0x63c

Fix this by saving the stream_info pointers and clearing the
ep references under the lock, then calling xhci_free_stream_info()
outside the lock where sleeping is allowed.

Fixes: 8df75f42f8e6 ("USB: xhci: Add memory allocation for USB3 bulk streams.")
Cc: stable@vger.kernel.org
Signed-off-by: Lianqin Hu <hulianqin@vivo.com>
---
 drivers/usb/host/xhci.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a54f5b57f205..c9af508edcb9 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3788,6 +3788,7 @@ static int xhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
 	struct xhci_virt_device *vdev;
 	struct xhci_command *command;
 	struct xhci_input_control_ctx *ctrl_ctx;
+	struct xhci_stream_info *stream_info[EP_CTX_PER_DEV];
 	unsigned int ep_index;
 	unsigned long flags;
 	u32 changed_ep_bitmask;
@@ -3848,10 +3849,14 @@ static int xhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
 	if (ret < 0)
 		return ret;
 
+	/* dma_free_coherent() called by xhci_free_stream_info() may sleep,
+	 * so save stream_info pointers and clear references under lock,
+	 * then free the memory outside lock.
+	 */
 	spin_lock_irqsave(&xhci->lock, flags);
 	for (i = 0; i < num_eps; i++) {
 		ep_index = xhci_get_endpoint_index(&eps[i]->desc);
-		xhci_free_stream_info(xhci, vdev->eps[ep_index].stream_info);
+		stream_info[i] = vdev->eps[ep_index].stream_info;
 		vdev->eps[ep_index].stream_info = NULL;
 		/* FIXME Unset maxPstreams in endpoint context and
 		 * update deq ptr to point to normal string ring.
@@ -3861,6 +3866,9 @@ static int xhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
 	}
 	spin_unlock_irqrestore(&xhci->lock, flags);
 
+	for (i = 0; i < num_eps; i++)
+		xhci_free_stream_info(xhci, stream_info[i]);
+
 	return 0;
 }
 
-- 
2.48.1

Re: [PATCH] usb: xhci: Fix sleep in atomic context in xhci_free_streams()

[Michal Pecio]

On Thu, 11 Jun 2026 06:44:14 +0000, 胡连勤 wrote:
> When a USB device with active stream endpoints is disconnected,
> xhci_free_streams() is called from the hub_event workqueue to
> free the stream resources while holding xhci->lock with irqs
> disabled. 

Pedantry: it (currently) holds the lock while freeing, but it isn't
called with the lock held, nor for the explicit purpose of freeing
things with the lock held. Not sure how to parse this sentence?

> It calls xhci_free_stream_info(), which invokes
> xhci_free_stream_ctx(), which in turn calls dma_free_coherent()
> for large stream context arrays.
> 
> dma_free_coherent() can sleep (e.g. via vunmap), triggering
> a BUG when called from atomic context.
> 
> Call trace:
>  dma_free_attrs+0x174/0x220
>  xhci_free_stream_info+0xd0/0x11c
>  xhci_free_streams+0x278/0x37c
>  usb_free_streams+0x98/0xc0
>  usb_unbind_interface+0x1b8/0x2f8
>  device_release_driver_internal+0x1d4/0x2cc
>  device_release_driver+0x18/0x28
>  bus_remove_device+0x160/0x1a4
>  device_del+0x1ec/0x350
>  usb_disable_device+0x98/0x214
>  usb_disconnect+0xf0/0x35c
>  hub_event+0xab4/0x19ec
>  process_one_work+0x278/0x63c
> 
> Fix this by saving the stream_info pointers and clearing the
> ep references under the lock, then calling xhci_free_stream_info()
> outside the lock where sleeping is allowed.

I wonder if this copy is necessary or if it would suffice to start with
calling xhci_free_stream_info() unlocked (EP_GETTING_NO_STREAMS should
ensure that nobody submits new URBs and hopefully core won't attempt to
re-enable streams concurrently) and then grab the lock only to update
vdev->eps stream_info and ep_state fields.

Regards,
Michal

答复: [PATCH] usb: xhci: Fix sleep in atomic context in xhci_free_streams()

[胡连勤]

Hi Michal Pecio:

> > When a USB device with active stream endpoints is disconnected,
> > xhci_free_streams() is called from the hub_event workqueue to
> > free the stream resources while holding xhci->lock with irqs
> > disabled.
> 
> Pedantry: it (currently) holds the lock while freeing, but it isn't
> called with the lock held, nor for the explicit purpose of freeing
> things with the lock held. Not sure how to parse this sentence?

The above description has a problem; please update the description information:
When a USB device with active stream endpoints is disconnected,
xhci_free_streams() is called from the hub_event workqueue to
free the stream resources.  It acquires xhci->lock with irqs
disabled, but then calls xhci_free_stream_info() under the lock.

> > It calls xhci_free_stream_info(), which invokes
> > xhci_free_stream_ctx(), which in turn calls dma_free_coherent()
> > for large stream context arrays.
> >
> > dma_free_coherent() can sleep (e.g. via vunmap), triggering
> > a BUG when called from atomic context.
> >
> > Call trace:
> >  dma_free_attrs+0x174/0x220
> >  xhci_free_stream_info+0xd0/0x11c
> >  xhci_free_streams+0x278/0x37c
> >  usb_free_streams+0x98/0xc0
> >  usb_unbind_interface+0x1b8/0x2f8
> >  device_release_driver_internal+0x1d4/0x2cc
> >  device_release_driver+0x18/0x28
> >  bus_remove_device+0x160/0x1a4
> >  device_del+0x1ec/0x350
> >  usb_disable_device+0x98/0x214
> >  usb_disconnect+0xf0/0x35c
> >  hub_event+0xab4/0x19ec
> >  process_one_work+0x278/0x63c
> >
> > Fix this by saving the stream_info pointers and clearing the
> > ep references under the lock, then calling xhci_free_stream_info()
> > outside the lock where sleeping is allowed.
> 
> I wonder if this copy is necessary or if it would suffice to start with
> calling xhci_free_stream_info() unlocked (EP_GETTING_NO_STREAMS should
> ensure that nobody submits new URBs and hopefully core won't attempt to
> re-enable streams concurrently) and then grab the lock only to update
> vdev->eps stream_info and ep_state fields.

I considered this approach, but there is
a use-after-free window between freeing stream_info and clearing
the pointer.
The xHCI interrupt handler (xhci_irq()) holds xhci->lock while
processing events, and several event handlers access
ep->stream_info under the lock — e.g.
ring_doorbell_for_active_rings() iterates
stream_info->stream_rings[], and xhci_handle_cmd_set_deq()
accesses stream_info->stream_ctx_array[].
If we free stream_info first (outside the lock), then acquire the
lock to clear the pointer, there is a window where the interrupt
handler can still see the old (dangling) pointer and dereference
freed memory. So we need to clear the reference under the lock first,
then free the memory outside.

Lianqin

Re: 答复: [PATCH] usb: xhci: Fix sleep in atomic context in xhci_free_streams()

[Mathias Nyman]

On 6/11/26 11:33, 胡连勤 wrote:
> Hi Michal Pecio:
> 
>>> When a USB device with active stream endpoints is disconnected,
>>> xhci_free_streams() is called from the hub_event workqueue to
>>> free the stream resources while holding xhci->lock with irqs
>>> disabled.
>>
>> Pedantry: it (currently) holds the lock while freeing, but it isn't
>> called with the lock held, nor for the explicit purpose of freeing
>> things with the lock held. Not sure how to parse this sentence?
> 
> The above description has a problem; please update the description information:
> When a USB device with active stream endpoints is disconnected,
> xhci_free_streams() is called from the hub_event workqueue to
> free the stream resources.  It acquires xhci->lock with irqs
> disabled, but then calls xhci_free_stream_info() under the lock.
> 
>>> It calls xhci_free_stream_info(), which invokes
>>> xhci_free_stream_ctx(), which in turn calls dma_free_coherent()
>>> for large stream context arrays.
>>>
>>> dma_free_coherent() can sleep (e.g. via vunmap), triggering
>>> a BUG when called from atomic context.
>>>
>>> Call trace:
>>>   dma_free_attrs+0x174/0x220
>>>   xhci_free_stream_info+0xd0/0x11c
>>>   xhci_free_streams+0x278/0x37c
>>>   usb_free_streams+0x98/0xc0
>>>   usb_unbind_interface+0x1b8/0x2f8
>>>   device_release_driver_internal+0x1d4/0x2cc
>>>   device_release_driver+0x18/0x28
>>>   bus_remove_device+0x160/0x1a4
>>>   device_del+0x1ec/0x350
>>>   usb_disable_device+0x98/0x214
>>>   usb_disconnect+0xf0/0x35c
>>>   hub_event+0xab4/0x19ec
>>>   process_one_work+0x278/0x63c
>>>
>>> Fix this by saving the stream_info pointers and clearing the
>>> ep references under the lock, then calling xhci_free_stream_info()
>>> outside the lock where sleeping is allowed.
>>
>> I wonder if this copy is necessary or if it would suffice to start with
>> calling xhci_free_stream_info() unlocked (EP_GETTING_NO_STREAMS should
>> ensure that nobody submits new URBs and hopefully core won't attempt to
>> re-enable streams concurrently) and then grab the lock only to update
>> vdev->eps stream_info and ep_state fields.
> 
> I considered this approach, but there is
> a use-after-free window between freeing stream_info and clearing
> the pointer.
> The xHCI interrupt handler (xhci_irq()) holds xhci->lock while
> processing events, and several event handlers access
> ep->stream_info under the lock — e.g.
> ring_doorbell_for_active_rings() iterates
> stream_info->stream_rings[], and xhci_handle_cmd_set_deq()
> accesses stream_info->stream_ctx_array[].
> If we free stream_info first (outside the lock), then acquire the
> lock to clear the pointer, there is a window where the interrupt
> handler can still see the old (dangling) pointer and dereference
> freed memory. So we need to clear the reference under the lock first,
> then free the memory outside.

Good point, but the need for this reveals another bug.

This means xhci_free_streams() can queue a configure endpoint command
to remove streams, and xhci_irq() can ring the doorbell of a stream at the
same time xHC controller is processing the command to remove the stream


CPU0				CPU1				xHC controller

xhci_free_streams()
spin_lock()
state|= EP_GETTING_NO_STREAMS;					process old cmd/xfer
spin_unlock							write old event
xhci_configure_endpoint()					trigger interrupt
				xhci_irq()
				spin_lock()
				handling old event
				ring_db_for_active_rings()	process configure_endpoint cmd	 				

Looks like we assume streams are functional by checking if ep->stream_info
exists in many places where we should check ep_state stream flags instead.

We should for example prevent ringing stream doorbell if ep_state has
EP_GETTING_NO_STREAM flag set.

So I think this is a good targeted patch for this specific issue.
We should start fixing the other issues and hopefully end up where
Michal suggested.

Thanks
Mathias

答复: 答复: [PATCH] usb: xhci: Fix sleep in atomic context in xhci_free_streams()

[胡连勤]

Hi Mathias, Micha

> >>> When a USB device with active stream endpoints is disconnected,
> >>> xhci_free_streams() is called from the hub_event workqueue to
> >>> free the stream resources while holding xhci->lock with irqs
> >>> disabled.
> >>
> >> Pedantry: it (currently) holds the lock while freeing, but it isn't
> >> called with the lock held, nor for the explicit purpose of freeing
> >> things with the lock held. Not sure how to parse this sentence?
> >
> > The above description has a problem; please update the description information:
> > When a USB device with active stream endpoints is disconnected,
> > xhci_free_streams() is called from the hub_event workqueue to
> > free the stream resources.  It acquires xhci->lock with irqs
> > disabled, but then calls xhci_free_stream_info() under the lock.
> >
> >>> It calls xhci_free_stream_info(), which invokes
> >>> xhci_free_stream_ctx(), which in turn calls dma_free_coherent()
> >>> for large stream context arrays.
> >>>
> >>> dma_free_coherent() can sleep (e.g. via vunmap), triggering
> >>> a BUG when called from atomic context.
> >>>
> >>> Call trace:
> >>>   dma_free_attrs+0x174/0x220
> >>>   xhci_free_stream_info+0xd0/0x11c
> >>>   xhci_free_streams+0x278/0x37c
> >>>   usb_free_streams+0x98/0xc0
> >>>   usb_unbind_interface+0x1b8/0x2f8
> >>>   device_release_driver_internal+0x1d4/0x2cc
> >>>   device_release_driver+0x18/0x28
> >>>   bus_remove_device+0x160/0x1a4
> >>>   device_del+0x1ec/0x350
> >>>   usb_disable_device+0x98/0x214
> >>>   usb_disconnect+0xf0/0x35c
> >>>   hub_event+0xab4/0x19ec
> >>>   process_one_work+0x278/0x63c
> >>>
> >>> Fix this by saving the stream_info pointers and clearing the
> >>> ep references under the lock, then calling xhci_free_stream_info()
> >>> outside the lock where sleeping is allowed.
> >>
> >> I wonder if this copy is necessary or if it would suffice to start with
> >> calling xhci_free_stream_info() unlocked (EP_GETTING_NO_STREAMS should
> >> ensure that nobody submits new URBs and hopefully core won't attempt to
> >> re-enable streams concurrently) and then grab the lock only to update
> >> vdev->eps stream_info and ep_state fields.
> >
> > I considered this approach, but there is
> > a use-after-free window between freeing stream_info and clearing
> > the pointer.
> > The xHCI interrupt handler (xhci_irq()) holds xhci->lock while
> > processing events, and several event handlers access
> > ep->stream_info under the lock — e.g.
> > ring_doorbell_for_active_rings() iterates
> > stream_info->stream_rings[], and xhci_handle_cmd_set_deq()
> > accesses stream_info->stream_ctx_array[].
> > If we free stream_info first (outside the lock), then acquire the
> > lock to clear the pointer, there is a window where the interrupt
> > handler can still see the old (dangling) pointer and dereference
> > freed memory. So we need to clear the reference under the lock first,
> > then free the memory outside.
> 
> Good point, but the need for this reveals another bug.
> 
> This means xhci_free_streams() can queue a configure endpoint command
> to remove streams, and xhci_irq() can ring the doorbell of a stream at the
> same time xHC controller is processing the command to remove the stream
> 
> 
> CPU0				CPU1				xHC controller
> 
> xhci_free_streams()
> spin_lock()
> state|= EP_GETTING_NO_STREAMS;					process old cmd/xfer
> spin_unlock							write old event
> xhci_configure_endpoint()					trigger interrupt
> 				xhci_irq()
> 				spin_lock()
> 				handling old event
> 				ring_db_for_active_rings()	process configure_endpoint cmd
> 
> Looks like we assume streams are functional by checking if ep->stream_info
> exists in many places where we should check ep_state stream flags instead.
> 
> We should for example prevent ringing stream doorbell if ep_state has
> EP_GETTING_NO_STREAM flag set.
> 
> So I think this is a good targeted patch for this specific issue.
> We should start fixing the other issues and hopefully end up where
> Michal suggested.

Thanks for the review and the insight about the deeper issue.
I agree that many places check ep->stream_info instead of
ep_state stream flags, which could lead to the race you
described.

I'll modify the patch description for this patch and release version v2 later.

Thanks
Lianqin

Freeing bulk streams concurretnly with URB execution

[Michal Pecio]

On Thu, 11 Jun 2026 20:53:40 +0300, Mathias Nyman wrote:
> On 6/11/26 11:33, 胡连勤 wrote:
> > If we free stream_info first (outside the lock), then acquire the
> > lock to clear the pointer, there is a window where the interrupt
> > handler can still see the old (dangling) pointer and dereference
> > freed memory. So we need to clear the reference under the lock
> > first, then free the memory outside.
> 
> Good point, but the need for this reveals another bug.
> 
> This means xhci_free_streams() can queue a configure endpoint command
> to remove streams, and xhci_irq() can ring the doorbell of a stream
> at the same time xHC controller is processing the command to remove
> the stream

Yes, nothing seems to prevent that if there are URBs on the endpoint.
Without URBs the doorbell won't be rung, though UAF would still occur
with my proposal in absence of improved guards against it.

8df75f42f8e6 added the GETTING_(NO)_STREAMS flags to block new URBs
on transitioning endpoints and prevented enabling streams if URBs are
already pending, but not disabling streams.

So question is, can usbcore / uas actually try to disable streams with
pending URBs, or submit new URBs concurrently?

If so then we probably need to fail the attempt or flush the URBs, and
I suspect that storage (Cc) may not like the former option.

Otherwise, the URBs not only get in the way of cleaning things up, but
it's not even clear how they would complete. The HW won't execute them
anymore, probably someone will have to unlink, relying on the hack for
removing URBs from nonexistent rings and logging a dev_err().

On the upside, the above suggests that it's not a common occurrence if
nobody complained in 15 years...

> So I think this is a good targeted patch for this specific issue.
> We should start fixing the other issues and hopefully end up where
> Michal suggested.

Indeed, I see no way to simplify this patch without much work.

Regards,
Michal