Re: [PATCH v7 16/16] ALSA: virtio: serialize capture lifecycle and reset state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] ALSA: virtio: checking vss->suspended in sync_stop wedges suspended streams
- [High] ALSA: virtio: split reset_epoch update and missing checks corrupt pause state
- [High] ALSA: virtio: race and missing RX refill in virtsnd_pcm_ioctl() on RESET
- [Medium] ALSA: virtio: missing failure handling and state check in pause retry path
- [Medium] ALSA: virtio: missing XRUN wakeup on staged native-pause release failure
- [Medium] ALSA: virtio: race between snd_pcm_drop() and virtsnd_pcm_prepare()
--

commit 24d284e1ac3e3d50dab01d4de1927240e8a33acb
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: serialize capture lifecycle and reset state

This patch serializes PCM lifecycle callbacks with a per-substream mutex,
rebases staged capture counters on RESET, and adds reset epoch and suspend
checks to control and sync-stop paths.

> diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
> index 852cc27ac4a45..2fd8595046563 100644
> --- a/sound/virtio/virtio_card.c
> +++ b/sound/virtio/virtio_card.c
> @@ -354,6 +354,7 @@ static void virtsnd_reset_vqs(struct virtio_snd *snd)
>  
>  		guard(spinlock_irqsave)(&queue->lock);
>  		guard(spinlock)(&vss->lock);
> +		vss->reset_epoch++;
>  		vss->prepared = false;
>  		vss->xfer_enabled = false;
>  		vss->suspended = true;

[Severity: High]
Can incrementing vss->reset_epoch in the first loop of virtsnd_reset_vqs()
while resetting vss->pause_restart_state in the fourth loop corrupt the
pause-restart state machine if a control operation runs concurrently?

In virtsnd_reset_vqs(), the first loop increments vss->reset_epoch and
clears vss->prepared, while the fourth loop later resets
vss->pause_restart_state:

sound/virtio/virtio_card.c:virtsnd_reset_vqs() {
	...
	for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) {
		cancel_work_sync(&vss->elapsed_period);
		guard(spinlock_irqsave)(&vss->lock);
		vss->stopped = false;
		vss->xfer_xrun = false;
		vss->pause_restart_state = VIRTSND_PCM_PAUSE_RESTART_NONE;
	}
}

If virtsnd_pcm_pause_restart_send() enters between the first and fourth
loops, it samples the already-incremented vss->reset_epoch and transitions
vss->pause_restart_state to submitted_state. After the fourth loop resets
vss->pause_restart_state to VIRTSND_PCM_PAUSE_RESTART_NONE, the post-send
check in virtsnd_pcm_pause_restart_send() sees vss->reset_epoch == epoch
and vss->pause_restart_state != submitted_state, poisoning
vss->pause_restart_state to VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED
across virtsnd_restore().

Similarly, in virtsnd_pcm_pause_release_flush(), waking from
wait_event_timeout(vss->msg_empty, ...) when virtsnd_reset_vqs() cancels
pending RX buffers does not check vss->reset_epoch: if it runs before the
fourth loop it transitions to VIRTSND_PCM_PAUSE_RESTART_RELEASED and
returns 0 across a reset, and if it runs after the fourth loop it sees
vss->pause_restart_state == VIRTSND_PCM_PAUSE_RESTART_NONE and returns
-ETIMEDOUT instead of -ECANCELED. In virtsnd_pcm_pause_restart_start(),
the two locked blocks after virtsnd_pcm_pause_prepare() also omit
vss->reset_epoch checks, allowing vss->prepared = true to be set after the
first loop of virtsnd_reset_vqs() or returning -EBADFD instead of
-ECANCELED after the fourth loop.

[ ... ]

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 0685a001cf95d..573fab6525a61 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -667,6 +724,14 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
>  	return rc;
>  }
>  
> +static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
> +{
> +	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +
> +	guard(mutex)(&vss->lifecycle_lock);
> +	return virtsnd_pcm_prepare_locked(substream);
> +}

[Severity: Medium]
Can a concurrent snd_pcm_drop() and snd_pcm_prepare() still race despite
vss->lifecycle_lock?

In sound/core/pcm_native.c, snd_pcm_drop() stops the stream (setting
vss->stopped = true and runtime->stop_operating = true), drops the stream
lock, and calls snd_pcm_sync_stop() without holding runtime->buffer_mutex.
snd_pcm_sync_stop() clears runtime->stop_operating = false before calling
substream->ops->sync_stop(substream) (virtsnd_pcm_sync_stop()), which only
then acquires vss->lifecycle_lock.

If a concurrent snd_pcm_prepare() runs after snd_pcm_drop() clears
runtime->stop_operating = false in snd_pcm_sync_stop() but before
virtsnd_pcm_sync_stop() acquires vss->lifecycle_lock, the prepare path's
own snd_pcm_sync_stop() sees runtime->stop_operating == false and skips
calling virtsnd_pcm_sync_stop(). If virtsnd_pcm_prepare() then acquires
vss->lifecycle_lock first, virtsnd_pcm_prepare_locked() does not call
virtsnd_pcm_sync_stop_locked() (unlike virtsnd_pcm_hw_params()), so it
observes vss->stopped == true and fails with -EBADFD.

Conversely, if the delayed virtsnd_pcm_sync_stop() from snd_pcm_drop()
acquires vss->lifecycle_lock after a concurrent prepare completes,
virtsnd_pcm_sync_stop_locked() now includes vss->prepared in need_release
(and virtsnd_pcm_pause_restart_sync_stop() releases
VIRTSND_PCM_PAUSE_RESTART_PREPARED), so it sends VIRTIO_SND_R_PCM_RELEASE
and clears vss->prepared = false on the newly prepared stream.

[ ... ]

> @@ -771,20 +849,31 @@ static int virtsnd_pcm_pause_restart_pause(struct virtio_pcm_substream *vss)
>  
>  	scoped_guard(spinlock_irqsave, &vss->lock) {
>  		state = vss->pause_restart_state;
> -		if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASED)
> +		if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASED) {
> +			if (vss->xfer_xrun)
> +				virtsnd_pcm_schedule_period_elapsed(vss);
>  			return vss->xfer_xrun ? -EPIPE : 0;
> +		}
>  		vss->xfer_enabled = false;
>  		vss->stopped = true;
>  	}
>  
>  	if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT) {
> -		rc = virtsnd_pcm_pause_restart_sync_stop(vss->substream);
> -		if (!rc) {
> -			guard(spinlock_irqsave)(&vss->lock);
> -			vss->stopped = true;
> -			if (vss->xfer_xrun)
> -				rc = -EPIPE;
> +		unsigned int js = msecs_to_jiffies(virtsnd_msg_timeout_ms);
> +
> +		/* Trigger holds the PCM lock: never cancel the period worker here. */
> +		if (!wait_event_timeout(vss->msg_empty,
> +				       !virtsnd_pcm_msg_pending_num(vss), js))
> +			return -ETIMEDOUT;
> +		scoped_guard(spinlock_irqsave, &vss->lock) {
> +			if (vss->suspended)
> +				return -ECANCELED;
> +			vss->prepared = false;
> +			vss->pause_restart_state = VIRTSND_PCM_PAUSE_RESTART_RELEASED;
> +			rc = vss->xfer_xrun ? -EPIPE : 0;
>  		}
> +		if (rc)
> +			virtsnd_pcm_pause_restart_failed(vss);
>  		return rc;
>  	}

[Severity: Medium]
In the VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT branch of
virtsnd_pcm_pause_restart_pause(), does returning -ETIMEDOUT directly on
wait_event_timeout() failure skip calling virtsnd_pcm_pause_restart_failed()?

Previously this path called virtsnd_pcm_pause_restart_sync_stop(), which
invoked virtsnd_pcm_pause_restart_fail(vss, false) on timeout to set
vss->xfer_xrun = true, schedule vss->elapsed_period, and set
substream->runtime->stop_operating = true. In the new open-coded path,
timing out leaves vss->stopped = true while runtime->stop_operating and
vss->xfer_xrun remain false.

Also, when wait_event_timeout() returns non-zero, should the locked block
re-verify that vss->msg_count == 0 and vss->pause_restart_state ==
VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT before setting
VIRTSND_PCM_PAUSE_RESTART_RELEASED, matching
virtsnd_pcm_pause_release_flush() and virtsnd_pcm_pause_restart_sync_stop()?

[ ... ]

> @@ -945,10 +1039,12 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
>  			return -ENOMEM;
>  		}
>  
> -		rc = virtsnd_ctl_msg_send_sync(snd, msg);
> -		if (rc)
> +		rc = virtsnd_pcm_send_sync(vss, msg);
> +		if (rc && rc != -ECANCELED)
>  			virtsnd_pcm_start_failed(vss);
>  
> +		if (!rc && vss->capture_staging)
> +			virtsnd_pcm_schedule_period_elapsed(vss);
>  		return rc;

[Severity: Medium]
When SNDRV_PCM_TRIGGER_PAUSE_RELEASE fails for a staged capture stream in
native-pause mode (vss->capture_staging &&
!virtsnd_pcm_pause_restart_enabled(vss)), does calling
virtsnd_pcm_start_failed() leave paused capture readers sleeping without
an XRUN notification?

When virtsnd_pcm_capture_send() fails earlier in virtsnd_pcm_trigger() on
SNDRV_PCM_TRIGGER_PAUSE_RELEASE, the error path sets vss->xfer_xrun = true
and calls virtsnd_pcm_schedule_period_elapsed(vss):

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trigger() {
	...
			if (rc) {
				vss->stopped = true;
				substream->runtime->stop_operating = true;
				if (vss->capture_staging &&
				    vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
				    command == SNDRV_PCM_TRIGGER_PAUSE_RELEASE) {
					/* Pause undo leaves ALSA paused; wake its readers. */
					vss->xfer_xrun = true;
					virtsnd_pcm_schedule_period_elapsed(vss);
				}
				return rc;
			}
}

However, if SNDRV_PCM_TRIGGER_PAUSE_RELEASE fails either at the initial
(vss->stopped || !vss->prepared) check or when allocating/sending
VIRTIO_SND_R_PCM_START via virtsnd_pcm_start_failed(), vss->xfer_xrun is
never set and vss->elapsed_period is not scheduled, so ALSA's pause undo
leaves runtime->state in SNDRV_PCM_STATE_PAUSED without transitioning the
stream to XRUN.

[ ... ]

> @@ -1135,8 +1244,12 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
>  		return !rc ? -ETIMEDOUT : rc;
>  	}
>  
> -	scoped_guard(spinlock_irqsave, &vss->lock)
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (vss->suspended)
> +			return -ECANCELED;
> +		vss->prepared = false;
>  		vss->stopped = false;
> +	}
>  
>  	return 0;
>  }

[Severity: High]
Won't returning -ECANCELED when vss->suspended is true in
virtsnd_pcm_sync_stop_locked() and virtsnd_pcm_pause_restart_sync_stop()
permanently wedge the substream after SNDRV_PCM_TRIGGER_SUSPEND?

During system suspend, snd_pcm_suspend_all() triggers
SNDRV_PCM_TRIGGER_SUSPEND, which in virtsnd_pcm_trigger() and
virtsnd_pcm_pause_restart_stop(vss, true) sets both vss->suspended = true
and vss->stopped = true. When virtsnd_pcm_sync_stop_locked() (or
virtsnd_pcm_pause_restart_sync_stop()) then runs, it sends
VIRTIO_SND_R_PCM_RELEASE and flushes the queue, but returns -ECANCELED
because vss->suspended is true, leaving vss->stopped = true and
vss->prepared = true.

Because this patch also removes vss->suspended = false from
virtsnd_pcm_open(), vss->suspended is now only cleared in
virtsnd_pcm_prepare_locked() after the vss->stopped check:

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_prepare_locked() {
	...
	scoped_guard(spinlock_irqsave, &vss->lock)
		stopped = vss->stopped;
	if (stopped) {
		dev_err(&vdev->dev, "SID %u: stream release failed\n",
			vss->sid);
		substream->runtime->stop_operating = true;
		return -EBADFD;
	}
	...
	scoped_guard(spinlock_irqsave, &vss->lock) {
		...
		vss->suspended = false;
}

Once vss->stopped and vss->suspended are both true:
- virtsnd_pcm_prepare_locked() sees vss->stopped == true, sets
  runtime->stop_operating = true, and returns -EBADFD before reaching
  vss->suspended = false.
- virtsnd_pcm_hw_params() calls virtsnd_pcm_sync_stop_locked(), which sees
  need_release (vss->stopped is true) and vss->suspended == true and fails
  with -ECANCELED.
- Closing and reopening the PCM device calls virtsnd_pcm_open(), which no
  longer clears vss->suspended and calls virtsnd_pcm_sync_stop(), failing
  open with -ECANCELED.

In addition, virtsnd_pcm_pause_restart_fail() and
virtsnd_pcm_start_failed() now return early when vss->suspended is true.
In virtsnd_pcm_prepare_locked(), the else branch of if (!suspended) calls
virtsnd_pcm_dev_set_params() while vss->suspended is still true; if
virtsnd_pcm_dev_set_params() fails, calling
virtsnd_pcm_pause_restart_fail(vss, false) is a no-op that skips setting
vss->stopped, vss->xfer_xrun, and runtime->stop_operating. Likewise, after
virtsnd_freeze() and virtsnd_restore(), vss->suspended remains true until
prepare succeeds, so a failed resume via SNDRV_PCM_TRIGGER_PAUSE_RELEASE
causes virtsnd_pcm_pause_restart_fail() to return early without setting
vss->xfer_xrun, scheduling vss->elapsed_period, or setting
runtime->stop_operating.

[ ... ]

> @@ -1415,12 +1531,52 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
>  	return 0;
>  }
>  
> +static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
> +{
> +	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +
> +	guard(mutex)(&vss->lifecycle_lock);
> +	return virtsnd_pcm_sync_stop_locked(substream);
> +}
> +
> +static int virtsnd_pcm_ioctl(struct snd_pcm_substream *substream,
> +			    unsigned int cmd, void *arg)
> +{
> +	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	size_t position;
> +	size_t period;
> +	int rc;
> +
> +	if (cmd != SNDRV_PCM_IOCTL1_RESET || !vss->capture_staging)
> +		return snd_pcm_lib_ioctl(substream, cmd, arg);
> +
> +	rc = snd_pcm_lib_ioctl(substream, cmd, arg);
> +	if (rc)
> +		return rc;
> +	guard(pcm_stream_lock_irqsave)(substream);
> +	runtime->status->hw_ptr %= runtime->buffer_size;
> +	position = frames_to_bytes(runtime, runtime->status->hw_ptr);
> +	period = snd_pcm_lib_period_bytes(substream);
> +	guard(spinlock_irqsave)(&vss->lock);
> +	vss->hw_ptr = position;
> +	vss->capture_copied_total = position;
> +	vss->capture_acked_total = position;
> +	vss->capture_pointer_seen_total = position;
> +	vss->capture_ready = 0;
> +	vss->capture_notified_periods = period ? position / period : 0;
> +	vss->capture_callback_snapshot = 0;
> +	vss->capture_callback_snapshot_active = false;
> +	vss->pcm_indirect.appl_ptr = runtime->status->hw_ptr;
> +	return 0;
> +}

[Severity: High]
What prevents virtsnd_pcm_ioctl() on SNDRV_PCM_IOCTL1_RESET from racing
with RX completions and virtsnd_pcm_period_elapsed() on a running staged
capture stream, or leaving RX buffers unqueued after clearing
vss->capture_ready?

When SNDRV_PCM_IOCTL_RESET is called while the stream is in
SNDRV_PCM_STATE_RUNNING:

1. snd_pcm_lib_ioctl() -> snd_pcm_lib_ioctl_reset() acquires the PCM
   stream lock, updates runtime->status->hw_ptr via
   virtsnd_pcm_cp_pointer(), reduces runtime->status->hw_ptr modulo
   runtime->buffer_size, and drops the PCM stream lock. Before
   virtsnd_pcm_ioctl() acquires vss->lock, virtsnd_pcm_capture_complete()
   can run in RX interrupt context: it checks ring capacity against the
   pre-reset vss->capture_ready (which can trigger a false ring_full XRUN)
   and advances vss->hw_ptr and vss->capture_copied_total as new audio is
   copied. virtsnd_pcm_ioctl() then overwrites vss->hw_ptr and
   vss->capture_copied_total with the older position sampled before
   vss->lock was taken, rewinding vss->hw_ptr over newly captured audio.

2. Between snd_pcm_lib_ioctl_reset() dropping the stream lock,
   virtsnd_pcm_ioctl() returning, and snd_pcm_do_reset() /
   snd_pcm_post_reset() re-acquiring the stream lock to zero
   runtime->hw_ptr_base and set runtime->control->appl_ptr =
   runtime->status->hw_ptr, virtsnd_pcm_period_elapsed() can run on
   system_highpri_wq. If virtsnd_pcm_period_elapsed() advances
   runtime->status->hw_ptr before snd_pcm_post_reset() sets
   runtime->control->appl_ptr = runtime->status->hw_ptr,
   runtime->control->appl_ptr is moved ahead of vss->pcm_indirect.appl_ptr
   and vss->capture_acked_total without an ACK callback, leaving
   vss->capture_ready inflated and causing subsequent ACKs or completions
   to fail with -EPIPE.

3. In bounded capture mode (!vss->capture_free_running),
   virtsnd_pcm_capture_enqueue() throttles queued RX descriptors based on
   vss->buffer_bytes - vss->capture_ready. After virtsnd_pcm_ioctl() resets
   vss->capture_ready = 0 on a running stream, nothing calls
   virtsnd_pcm_capture_send(vss) under queue->lock and vss->lock to refill
   held-back RX requests.

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