Re: [PATCH] usb: gadget: u_audio: read period size while holding stream lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] usb: gadget: u_audio: race with close in snd_pcm_period_elapsed() after unlock
--

commit a295a8414332708b19b9379c3e997a95e5d868c8
Author: Yaohan Li <shunsheng1218@gmail.com>

usb: gadget: u_audio: read period size while holding stream lock

This commit moves the snd_pcm_lib_period_bytes() call in
u_audio_iso_complete() inside the substream lock critical section to avoid
reading substream->runtime after unlocking.

> diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
> index f0ee83abcebc7..7f378858146a1 100644
> --- a/drivers/usb/gadget/function/u_audio.c
> +++ b/drivers/usb/gadget/function/u_audio.c
> @@ -156,6 +156,7 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
>  	struct uac_rtd_params *prm = req->context;
>  	struct snd_uac_chip *uac = prm->uac;
>  	unsigned int frames, p_pktsize;
> +	unsigned int period_bytes;
>  	unsigned long long pitched_rate_mil, p_pktsize_residue_mil,
>  			residue_frames_mil, div_result;
>  
> @@ -267,9 +268,10 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
>  	/* update hw_ptr after data is copied to memory */
>  	prm->hw_ptr = (hw_ptr + req->actual) % runtime->dma_bytes;
>  	hw_ptr = prm->hw_ptr;
> +	period_bytes = snd_pcm_lib_period_bytes(substream);
>  	snd_pcm_stream_unlock(substream);
>  
> -	if ((hw_ptr % snd_pcm_lib_period_bytes(substream)) < req->actual)
> +	if ((hw_ptr % period_bytes) < req->actual)
>  		snd_pcm_period_elapsed(substream);

[Severity: Critical]
Can snd_pcm_period_elapsed() still race with a concurrent close once
u_audio_iso_complete() calls snd_pcm_stream_unlock(substream)?

Because uac_pcm_ops does not implement .sync_stop and card->sync_irq is not
set, snd_pcm_sync_stop() is a no-op. As soon as u_audio_iso_complete() drops
the stream lock, a concurrent close in
snd_pcm_release()->snd_pcm_release_substream() can complete snd_pcm_drop(),
run do_hw_free(), and enter snd_pcm_detach_substream(), which executes
without holding pcm_stream_lock:

sound/core/pcm.c:snd_pcm_detach_substream() {
	...
	if (substream->timer) {
		scoped_guard(spinlock_irq, &substream->timer->lock)
			substream->runtime = NULL;
	} else {
		substream->runtime = NULL;
	}
	mutex_destroy(&runtime->buffer_mutex);
	snd_fasync_free(runtime->fasync);
	kfree(runtime);
	...
}

When (hw_ptr % period_bytes) < req->actual is true, u_audio_iso_complete()
then calls snd_pcm_period_elapsed()->snd_pcm_period_elapsed_under_stream_lock():

sound/core/pcm_lib.c:snd_pcm_period_elapsed_under_stream_lock() {
	struct snd_pcm_runtime *runtime;

	if (PCM_RUNTIME_CHECK(substream))
		return;
	runtime = substream->runtime;

	if (!snd_pcm_running(substream) ||
	    snd_pcm_update_hw_ptr0(substream, 1) < 0)
		goto _end;
	...
 _end:
	snd_kill_fasync(runtime->fasync, SIGIO, POLL_IN);
}

If snd_pcm_detach_substream() has already cleared substream->runtime,
PCM_RUNTIME_CHECK(substream) triggers a kernel warning via snd_BUG_ON(). If
snd_pcm_detach_substream() clears substream->runtime and frees runtime right
after PCM_RUNTIME_CHECK(substream) passes, snd_pcm_running(substream)
dereferences a NULL or freed substream->runtime, and snd_kill_fasync() reads
runtime->fasync from the freed runtime struct.

Would calling snd_pcm_period_elapsed_under_stream_lock(substream) before
snd_pcm_stream_unlock(substream) avoid dropping and reacquiring the stream
lock across the period check?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179378417544.69464.16164899430939223277@gmail.com?part=1