Re: [PATCH v6 13/14] ALSA: virtio: fence staged capture state and recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] ALSA: virtio: deadlock and broken retry in virtsnd_pcm_pause_restart_pause()
- [High] ALSA: virtio: XRUN wakeup suppressed in virtsnd_pcm_pause_restart_fail()
- [High] ALSA: virtio: premature paused stop arming in virtsnd_pcm_cp_ack()
- [High] ALSA: virtio: post-reset substream state clobbered after virtsnd_reset_vqs()
- [High] ALSA: virtio: desynchronized prepared and stopped state on prepare and stop
- [High] ALSA: virtio: missing paused stop arming in non-staged virtsnd_pcm_cp_ack()
--

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

ALSA: virtio: fence staged capture state and recovery

This commit tracks device preparation under the substream lock, fences
virtqueue submissions and state transitions across pause, reset, and open
failure paths, and updates staged capture ACK and XRUN recovery handling.

> diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
> index df7091b74a521..852cc27ac4a45 100644
> --- a/sound/virtio/virtio_card.c
> +++ b/sound/virtio/virtio_card.c
> @@ -348,6 +348,16 @@ static void virtsnd_reset_vqs(struct virtio_snd *snd)
>  	struct virtio_device *vdev = snd->vdev;
>  	unsigned int i;
>  
> +	for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) {
> +		struct virtio_pcm_substream *vss = &snd->substreams[i];
> +		struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +
> +		guard(spinlock_irqsave)(&queue->lock);
> +		guard(spinlock)(&vss->lock);
> +		vss->prepared = false;
> +		vss->xfer_enabled = false;
> +		vss->suspended = true;
> +	}
>  	virtio_reset_device(vdev);
>  
>  	/* Deleting a virtqueue does not unmap its outstanding descriptors. */
> @@ -364,9 +374,20 @@ static void virtsnd_reset_vqs(struct virtio_snd *snd)
>  		spin_unlock_irqrestore(&queue->lock, flags);
>  	}
>  
> -	vdev->config->del_vqs(vdev);
> -	for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i)
> +	for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i) {
> +		guard(spinlock_irqsave)(&snd->queues[i].lock);
>  		snd->queues[i].vqueue = NULL;
> +	}
> +	vdev->config->del_vqs(vdev);
> +	for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) {
> +		struct virtio_pcm_substream *vss = &snd->substreams[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;
> +	}
>  }

[Severity: High]
Can resetting vss->stopped, vss->xfer_xrun, and vss->pause_restart_state
inside virtsnd_reset_vqs() get clobbered by in-flight control message
senders that are only woken up afterwards?

In virtsnd_freeze() (and virtsnd_remove()), the teardown sequence runs:

  virtsnd_ctl_msg_cancel_all(snd);
  virtsnd_reset_vqs(snd);
  virtsnd_ctl_msg_cleanup(snd);

virtsnd_ctl_msg_cancel_all() marks pending control messages with
msg->cancelled = true, but it does not complete msg->notify. The threads
waiting in virtsnd_ctl_msg_send_sync() are only woken later when
virtsnd_ctl_msg_cleanup() calls virtsnd_ctl_msg_complete().

When a thread blocked in virtsnd_pcm_pause_restart_send() wakes up after
virtsnd_reset_vqs() has already reset the substream state,
virtsnd_ctl_msg_uncertain(msg, rc) returns true and vss->pause_restart_state
(now VIRTSND_PCM_PAUSE_RESTART_NONE) no longer matches submitted_state:

  scoped_guard(spinlock_irqsave, &vss->lock) {
          if ((rc && uncertain) || vss->pause_restart_state != submitted_state) {
                  vss->pause_restart_state =
                          VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED;
                  if (!rc)
                          rc = -EIO;
          } ...
  }

The woken caller then runs virtsnd_pcm_pause_restart_fail() (or
virtsnd_pcm_start_failed() in non-pause-restart mode), overwriting
vss->pause_restart_state with VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED and
setting vss->stopped = true and vss->xfer_xrun = true after the reset has
already completed, leaving the substream wedged after virtsnd_restore().

[ ... ]

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

[ ... ]

> @@ -223,7 +227,8 @@ virtsnd_pcm_pause_restart_fail(struct virtio_pcm_substream *vss,
>  	bool notify_xrun;
>  
>  	scoped_guard(spinlock_irqsave, &vss->lock) {
> -		notify_xrun = !vss->xfer_xrun;
> +		notify_xrun = vss->runtime_active && vss->xfer_enabled &&
> +			      !vss->xfer_xrun;
>  		vss->xfer_enabled = false;
>  		vss->stopped = true;
>  		vss->xfer_xrun = true;

[Severity: High]
Does requiring vss->xfer_enabled in virtsnd_pcm_pause_restart_fail()
suppress XRUN notifications when SNDRV_PCM_TRIGGER_PAUSE_RELEASE or
SNDRV_PCM_TRIGGER_PAUSE_PUSH fails?

When a stream is paused, vss->xfer_enabled is false. During
SNDRV_PCM_TRIGGER_PAUSE_RELEASE in virtsnd_pcm_pause_restart_start(),
vss->xfer_enabled remains false until virtsnd_pcm_pause_prepare() and
virtsnd_pcm_capture_send() both succeed:

  if (resume) {
          rc = virtsnd_pcm_pause_prepare(vss);
          if (rc) {
                  virtsnd_pcm_pause_restart_failed(vss);
                  return rc;
          }
          ...
  }
  scoped_guard(spinlock_irqsave, &queue->lock) {
          guard(spinlock)(&vss->lock);
          ...
          rc = virtsnd_pcm_capture_send(vss);
          if (!rc)
                  vss->xfer_enabled = true;
  }
  if (rc) {
          virtsnd_pcm_pause_restart_failed(vss);
          return rc;
  }

If either step fails, virtsnd_pcm_pause_restart_fail() sees
vss->xfer_enabled == false and skips virtsnd_pcm_schedule_period_elapsed().
Because snd_pcm_action_single() runs snd_pcm_undo_pause() (where
vss->xfer_xrun is already true) and skips snd_pcm_post_pause(), the stream
stays in SNDRV_PCM_STATE_PAUSED with vss->xfer_xrun latched to true and
virtsnd_pcm_period_elapsed() is never scheduled to transition it to
SNDRV_PCM_STATE_XRUN and wake blocked readers.

Similarly, in virtsnd_pcm_pause_restart_pause(), vss->xfer_enabled is
cleared to false at function entry before VIRTIO_SND_R_PCM_STOP or
virtsnd_pcm_pause_release_flush() runs, so any failure in
virtsnd_pcm_pause_restart_pause() also sees vss->xfer_enabled == false and
never schedules virtsnd_pcm_period_elapsed().

[ ... ]

> @@ -599,8 +614,6 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
>  			return -EBADFD;
>  		}
>  
> -		vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
> -		vss->hw_ptr = 0;
>  	} else {
>  		struct snd_pcm_runtime *runtime = substream->runtime;
>  		unsigned int buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
> @@ -616,31 +629,30 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
>  		}
>  	}
>  
> -	/* ALSA prepare resets its pointers even after a suspended stream. */
> -	if (vss->capture_staging)
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		vss->prepared = false;
> +		vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
>  		vss->hw_ptr = 0;
> -
> -	if (!pause_restart)
> -		vss->xfer_xrun = false;
> -	vss->suspended = false;
> -	vss->msg_count = 0;
> -	virtsnd_pcm_msg_reset(vss);
> -	if (vss->capture_staging) {
> -		scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (!pause_restart)
> +			vss->xfer_xrun = false;
> +		vss->suspended = false;
> +		vss->msg_count = 0;
> +		virtsnd_pcm_msg_reset(vss);
> +		memset(&vss->pcm_indirect, 0, sizeof(vss->pcm_indirect));
> +		vss->pcm_indirect.sw_buffer_size = vss->buffer_bytes;
> +		vss->pcm_indirect.hw_buffer_size = vss->buffer_bytes;
> +		if (vss->capture_staging) {
>  			vss->capture_boundary = substream->runtime->boundary;
>  			rc = virtsnd_pcm_capture_refresh_mode_locked(vss,
> -								     &free_running);
> +							     &free_running);
> +			if (rc)
> +				return rc;
>  		}
> -		if (rc)
> -			return rc;
>  	}
>  
> -	memset(&vss->pcm_indirect, 0, sizeof(vss->pcm_indirect));
> -	vss->pcm_indirect.sw_buffer_size =
> -		vss->pcm_indirect.hw_buffer_size =
> -		snd_pcm_lib_buffer_bytes(substream);
> -
>  	rc = virtsnd_pcm_dev_prepare(vss);
> +	scoped_guard(spinlock_irqsave, &vss->lock)
> +		vss->prepared = !rc;
>  	if (pause_restart) {
>  		if (rc) {
>  			virtsnd_pcm_pause_restart_fail(vss, false);
> @@ -759,13 +771,23 @@ 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_RELEASE_SENT ||
> -		    state == VIRTSND_PCM_PAUSE_RESTART_RELEASED)
> -			return 0;
> +		if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASED)
> +			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;
> +		}
> +		return rc;
> +	}

[Severity: High]
Won't calling virtsnd_pcm_pause_restart_sync_stop() from inside
virtsnd_pcm_pause_restart_pause() deadlock on the PCM stream lock when
vss->elapsed_period work is pending or running?

Because vpcm->pcm->nonatomic is true, the ALSA PCM stream lock is a mutex
(substream->self_group.mutex), and ALSA holds it across substream->ops->trigger():

  CPU 0                                  CPU 1 (workqueue)
  snd_pcm_pause_lock_irq()
    pcm_stream_lock_irq()
    snd_pcm_action_single()
      snd_pcm_do_pause()
        virtsnd_pcm_trigger(PAUSE_PUSH)
          virtsnd_pcm_pause_restart_pause()
            virtsnd_pcm_pause_restart_sync_stop()
              cancel_work_sync(&vss->elapsed_period)
                                         virtsnd_pcm_period_elapsed()
                                           guard(pcm_stream_lock_irqsave)(substream)

virtsnd_pcm_period_elapsed() blocks waiting for pcm_stream_lock while
virtsnd_pcm_pause_restart_sync_stop() waits for virtsnd_pcm_period_elapsed()
to finish.

In addition, VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT is reached when an
earlier virtsnd_pcm_pause_release_flush() timed out, which called
virtsnd_pcm_pause_restart_failed() and set vss->xfer_xrun = true while
ALSA left runtime->state in SNDRV_PCM_STATE_RUNNING. When a subsequent
SNDRV_PCM_TRIGGER_PAUSE_PUSH retry drains the queue and returns -EPIPE
because vss->xfer_xrun is true, snd_pcm_action_single() runs
snd_pcm_undo_pause() and skips snd_pcm_post_pause(), keeping runtime->state
in SNDRV_PCM_STATE_RUNNING where snd_pcm_prepare() is rejected with -EBUSY.

>  	if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED ||
>  	    virtsnd_pcm_pause_restart_transitioning(state)) {
>  		virtsnd_pcm_pause_restart_fail(vss, true);
> @@ -813,10 +835,9 @@ static int virtsnd_pcm_pause_restart_stop(struct virtio_pcm_substream *vss,
>  	enum virtio_pcm_pause_restart_state state;
>  	int rc;
>  
> -	if (suspend)
> -		vss->suspended = true;
> -
>  	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		if (suspend)
> +			vss->suspended = true;
>  		vss->xfer_enabled = false;
>  		vss->stopped = true;
>  		state = vss->pause_restart_state;
> @@ -887,7 +908,7 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
>  
>  		scoped_guard(spinlock_irqsave, &queue->lock) {
>  			guard(spinlock)(&vss->lock);
> -			if (vss->stopped) {
> +			if (vss->stopped || !vss->prepared) {
>  				substream->runtime->stop_operating = true;
>  				return -EBADFD;
>  			}
> @@ -920,17 +941,18 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
>  		msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_START,
>  						GFP_KERNEL);
>  		if (!msg) {
> -			virtsnd_pcm_start_failed(vss, false);
> +			virtsnd_pcm_start_failed(vss);
>  			return -ENOMEM;
>  		}
>  
>  		rc = virtsnd_ctl_msg_send_sync(snd, msg);
>  		if (rc)
> -			virtsnd_pcm_start_failed(vss, false);
> +			virtsnd_pcm_start_failed(vss);
>  
>  		return rc;
>  	case SNDRV_PCM_TRIGGER_SUSPEND:
> -		vss->suspended = true;
> +		scoped_guard(spinlock_irqsave, &vss->lock)
> +			vss->suspended = true;
>  		fallthrough;

[Severity: High]
Are vss->prepared, vss->stopped, and vss->suspended kept consistent across
early prepare failures, sync_stop releases, and unprepared stop triggers?

1. In virtsnd_pcm_prepare(), vss->prepared = false is only set after the
early return checks for virtsnd_pcm_msg_pending_num(vss) and
virtsnd_pcm_dev_set_params(). If a stream is already in
SNDRV_PCM_STATE_PREPARED (with vss->prepared == true) and a re-prepare fails
in virtsnd_pcm_dev_set_params() or virtsnd_pcm_msg_pending_num(),
snd_pcm_do_prepare() leaves runtime->state as SNDRV_PCM_STATE_PREPARED and
virtsnd_pcm_prepare() returns with vss->prepared still true, allowing
subsequent START or ACK calls to queue transfers after a failed PREPARE.

2. In virtsnd_pcm_sync_stop() and virtsnd_pcm_pause_restart_sync_stop(),
sending VIRTIO_SND_R_PCM_RELEASE clears vss->stopped = false without
setting vss->prepared = false. After SNDRV_PCM_IOCTL_DROP releases the
stream on the device and moves runtime->state to SNDRV_PCM_STATE_SETUP,
vss->prepared remains true and vss->stopped is false, so a subsequent
SNDRV_PCM_IOCTL_SYNC_PTR (snd_pcm_sync_ptr() -> pcm_lib_apply_appl_ptr() ->
substream->ops->ack()) passes the !vss->prepared and vss->stopped checks in
virtsnd_pcm_pb_ack() and virtsnd_pcm_cp_ack() and queues descriptors on a
released stream.

3. Conversely, when !vss->prepared (such as after virtsnd_reset_vqs() sets
vss->prepared = false and vss->stopped = false), if SNDRV_PCM_TRIGGER_START
returns -EBADFD, snd_pcm_action_single() calls snd_pcm_undo_start() ->
virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_STOP). Because the
SNDRV_PCM_TRIGGER_STOP case does not check vss->prepared, it sets
vss->stopped = true and sends VIRTIO_SND_R_PCM_STOP to an unprepared
stream, and the next virtsnd_pcm_sync_stop() sends VIRTIO_SND_R_PCM_RELEASE
to the unconfigured stream, which fails and leaves vss->stopped latched to
true.

4. Also, vss->suspended = false in virtsnd_pcm_open() and the
if (!vss->suspended) check in virtsnd_pcm_prepare() still access
vss->suspended without holding vss->lock.

[ ... ]

> @@ -1352,7 +1376,9 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
>  
>  	if (vss->capture_staging) {
>  		rc = virtsnd_pcm_capture_staged_ack(vss, runtime);
> -		if (rc == -EPIPE) {
> +		if (rc == -EPIPE ||
> +		    (!rc && runtime->state == SNDRV_PCM_STATE_PAUSED &&
> +		     snd_pcm_capture_avail(runtime) >= runtime->stop_threshold)) {
>  			guard(spinlock_irqsave)(&vss->lock);
>  			virtsnd_pcm_capture_arm_paused_stop(vss);
>  		}

[Severity: High]
Can arming virtsnd_pcm_capture_arm_paused_stop() inside virtsnd_pcm_cp_ack()
when rc == 0 leave vss->stopped = true while the ALSA substream remains in
SNDRV_PCM_STATE_PAUSED?

In __snd_pcm_lib_xfer(), a single read that wraps around the ring buffer
boundary is split across two loop iterations. pcm_lib_apply_appl_ptr() ->
substream->ops->ack() is called after each contiguous chunk, whereas
snd_pcm_update_state() is only called once after the entire transfer loop
finishes.

If snd_pcm_capture_avail(runtime) >= runtime->stop_threshold after the first
chunk at the wrap boundary, but avail drops below runtime->stop_threshold
after the second chunk completes the read, virtsnd_pcm_cp_ack() calls
virtsnd_pcm_capture_arm_paused_stop(vss) on the first chunk (setting
vss->stopped = true and vss->xfer_enabled = false), while
snd_pcm_update_state() sees avail < runtime->stop_threshold and keeps
runtime->state in SNDRV_PCM_STATE_PAUSED. Calling snd_pcm_forward(),
snd_pcm_rewind(), or snd_pcm_sync_ptr() while paused also invokes
pcm_lib_apply_appl_ptr() -> substream->ops->ack() without calling
snd_pcm_update_state().

When userspace subsequently unpauses the stream with
SNDRV_PCM_TRIGGER_PAUSE_RELEASE in native pause mode, virtsnd_pcm_trigger()
sees vss->stopped == true and fails with -EBADFD.

>  		return rc;
>  	}
>  
>  	scoped_guard(spinlock_irqsave, &queue->lock) {
>  		guard(spinlock)(&vss->lock);
> +		if (!vss->prepared)
> +			return -EBADFD;
>  		transfer.indirect = vss->pcm_indirect;
>  		if (vss->stopped && runtime->state != SNDRV_PCM_STATE_DRAINING) {
>  			rc = -EPIPE;

[Severity: High]
What happens in the non-staged capture path (!vss->capture_staging) when a
paused read crosses runtime->stop_threshold and snd_pcm_update_state()
moves the substream to SNDRV_PCM_STATE_XRUN?

Because virtsnd_pcm_capture_arm_paused_stop() is only called in the
vss->capture_staging branch of virtsnd_pcm_cp_ack(), a paused non-staged
capture stream that enters SNDRV_PCM_STATE_XRUN from snd_pcm_update_state()
after a partial read (or when virtsnd_pcm_cp_ack() returns -EPIPE) never
sets vss->stopped = true or runtime->stop_operating = true, since
snd_pcm_do_stop() is a no-op when !snd_pcm_running(substream).

As a result, the queued RX descriptors remain pending in the virtqueue and
subsequent snd_pcm_prepare() calls skip virtsnd_pcm_sync_stop() and fail
with -EBADFD at virtsnd_pcm_msg_pending_num(vss). Furthermore, on
!CONFIG_MMU, because virtsnd_pcm_msg_complete() now checks
vss->prepared && !vss->stopped instead of vss->xfer_enabled:

  #ifndef CONFIG_MMU
  	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
  	    vss->prepared && !vss->stopped &&
  	    written_bytes > sizeof(msg->status)) {
  ...
  		memcpy(runtime->dma_area + msg->data_offset,
  		       vss->owned_dma_area + msg->data_offset, data_bytes);
  	}
  #endif

if userspace calls snd_pcm_drop() followed by snd_pcm_hw_params(),
snd_pcm_lib_malloc_pages() can free and reallocate runtime->dma_area before
virtsnd_pcm_hw_params() clears vss->prepared, allowing a concurrent RX
completion in virtsnd_pcm_msg_complete() to copy into freed
runtime->dma_area.

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