Re: [PATCH 2/9] ALSA: usb-audio: Fix possible race at sync of urb completions

[Takashi Iwai <tiwai@suse.de>]

On Tue, 22 Apr 2025 12:29:31 +0200,
Hillf Danton wrote:
> 
> On Tue, 22 Apr 2025 09:03:20 +0200 Takashi Iwai wrote:
> > On Tue, 22 Apr 2025 01:38:59 +0200, Hillf Danton wrote:
> > > On Mon, 21 Apr 2025 16:36:30 +0200 Takashi Iwai wrote:
> > > > On Mon, 21 Apr 2025 12:43:42 +0200, Hillf Danton wrote:
> > > > > I misread "Which reads and writes are you trying to solve?" though I
> > > > > showed the read/write, but it is a bad case particulay with UAF.
> > > > > 
> > > > > Could you tell us what will happen if the race is not fixed? Could ep
> > > > > be freed with in-flight urbs for example?
> > > > 
> > > > Before the patch, wait_clear_urbs() might return earlier than actually
> > > > all pending eps are finished, so it can be UAF.
> > > > 
> > > Got it.
> > > 
> > > > > Is it still race if the wait loop in wait_clear_urbs() ends before the
> > > > > urb complete callbace completes, given the last sentence in your commit
> > > > > message? If nope, igore my noise please.
> > > > 
> > > > Well, your concern about the missing barrier -- that would
> > > > wait_clear_urbs() missing the refcount decrement, hence it would be
> > > > rather to make the return delayed.  So it shouldn't lead to further
> > > > UAF, but at most it might lead to an unnecessary delay.
> > > > 
> > > > That said, I'm willing to take a fix even for a theoretical issue if
> > > > it clarifies what it really fixes.  But scratching a random surface
> > > > isn't what we want.
> > > > 
> > > Thank you for shedding light on the race, given
> > > a) the mb in 26fbe9772b8c ("USB: core: Fix hang in usb_kill_urb by adding memory barriers")
> > > b) the urb complete callback is invoked in giveback, see __usb_hcd_giveback_urb()
> > > 
> > > use the urb routines instead to close the race.
> > 
> > I'm afraid that it can break things as of this form; the stopped
> > stream might be restarted without reinitializing URBs.  That is, this
> > isn't called only from disconnect or close, but also just for stopping
> > the stream in the middle, too.
> > 
> I'd like to test the change proposed locally, so please tippoint me to
> the test programs that could trigger the break.

Well, you need to follow the code logic.  The function isn't called
only from release / disconnect, but also from the code pattern:
 trigger(STOP) -> prepare -> sync_stop
and the URBs aren't released / re-initialized in this case.

For the problem you raised, I suppose it's better to stick with your
first approach with the manual barriers.  But you'd need to describe
exactly what it does and why it needed as a proper patch.


thanks,

Takashi