Linux USB
 help / color / mirror / Atom feed
From: Michal Pecio <michal.pecio@gmail.com>
To: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: 胡连勤 <hulianqin@vivo.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Oliver Neukum" <oneukum@suse.com>,
	"Alan Stern" <stern@rowland.harvard.edu>
Subject: Freeing bulk streams concurretnly with URB execution
Date: Fri, 12 Jun 2026 13:27:40 +0200	[thread overview]
Message-ID: <20260612132740.00afd726.michal.pecio@gmail.com> (raw)
In-Reply-To: <10322326-6b5a-4f6e-8c8b-e915363137ee@linux.intel.com>

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

      parent reply	other threads:[~2026-06-12 11:27 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-11  6:44 [PATCH] usb: xhci: Fix sleep in atomic context in xhci_free_streams() 胡连勤
2026-06-11  7:45 ` Michal Pecio
2026-06-11  8:33   ` 答复: " 胡连勤
2026-06-11 17:53     ` Mathias Nyman
2026-06-12  2:14       ` 答复: " 胡连勤
2026-06-12 11:27       ` Michal Pecio [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260612132740.00afd726.michal.pecio@gmail.com \
    --to=michal.pecio@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hulianqin@vivo.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@linux.intel.com \
    --cc=oneukum@suse.com \
    --cc=stern@rowland.harvard.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox