Re: [PATCH v5 02/11] ALSA: virtio: Recover PCM state after a failed start

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: virtio: unrecoverable stream state after failed suspend sync_stop()
- [Medium] ALSA: virtio: unprepared stream left startable if virtsnd_pcm_prepare() fails
--

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

ALSA: virtio: Recover PCM state after a failed start

This commit recovers virtio PCM substream state when a start trigger fails by
disabling transfers, marking the stream as stopped under vss->lock, and
arming runtime->stop_operating so virtsnd_pcm_sync_stop() releases pending
I/O before subsequent prepare, hw_params, or start operations.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index a40ea394bf174..970499fe419b4 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -115,7 +124,8 @@ static int virtsnd_pcm_open(struct snd_pcm_substream *substream)
>  	snd_pcm_hw_constraint_integer(substream->runtime,
>  				      SNDRV_PCM_HW_PARAM_PERIODS);
>  
> -	vss->stopped = !!virtsnd_pcm_msg_pending_num(vss);
> +	scoped_guard(spinlock_irqsave, &vss->lock)
> +		vss->stopped |= !!vss->msg_count;
>  	vss->suspended = false;
>  
>  	/*

[ ... ]

> @@ -273,6 +295,16 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
>  	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
>  	struct virtio_device *vdev = vss->snd->vdev;
>  	struct virtio_snd_msg *msg;
> +	bool stopped;
> +
> +	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;
> +	}
>  
>  	if (!vss->suspended) {
>  		if (virtsnd_pcm_msg_pending_num(vss)) {

[Severity: High]
Can a failed virtsnd_pcm_sync_stop() during system suspend leave the PCM
substream in a permanently unrecoverable state after resume?

During system suspend, do_pcm_suspend() -> snd_pcm_suspend_all() runs:

  snd_pcm_do_suspend()
    virtsnd_pcm_trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND)
      vss->suspended = true;
      vss->stopped = true;
  snd_pcm_sync_stop(substream, false)
    virtsnd_pcm_sync_stop(substream)

and ignores the return value of snd_pcm_sync_stop(). In
virtsnd_pcm_sync_stop(), allocating or sending VIRTIO_SND_R_PCM_RELEASE or
waiting in wait_event_interruptible_timeout() for vss->msg_empty can fail or
be interrupted (for example, returning -ERESTARTSYS when a signal or the
freezer is pending, or -ETIMEDOUT while vss->msg_count > 0). With this
change, virtsnd_pcm_sync_stop() leaves vss->stopped = true and re-arms
substream->runtime->stop_operating = true.

Subsequently, virtsnd_freeze() resets the device and deletes the virtqueues
without clearing vss->stopped or vss->msg_count:

  virtsnd_freeze()
    virtio_reset_device(vdev);
    vdev->config->del_vqs(vdev);

which returns all device-side streams to their initial unconfigured state.

Previously, post-resume snd_pcm_prepare() could recover because
runtime->stop_operating had been cleared by snd_pcm_sync_stop(),
virtsnd_pcm_prepare() did not check vss->stopped, and the vss->suspended
branch skipped the pending-message check, re-sent
VIRTIO_SND_R_PCM_SET_PARAMS to the reset device, and reset
vss->msg_count = 0.

With this patch, when userspace calls snd_pcm_prepare() after resume:

1. Because runtime->stop_operating was re-armed, snd_pcm_do_prepare() calls
   virtsnd_pcm_sync_stop() first. Seeing vss->stopped == true,
   virtsnd_pcm_sync_stop() sends VIRTIO_SND_R_PCM_RELEASE to a freshly reset,
   unconfigured stream before VIRTIO_SND_R_PCM_SET_PARAMS has been sent, and
   waits on vss->msg_empty for a stale vss->msg_count on the recreated empty
   virtqueue, timing out with -ETIMEDOUT.
2. Even if virtsnd_pcm_prepare() is reached, checking if (stopped) before
   if (!vss->suspended) returns -EBADFD immediately and makes the
   vss->suspended recovery path unreachable.
3. Reconfiguring via virtsnd_pcm_hw_params() is also rejected by the new
   stopped and msg_count checks, and closing and reopening the substream via
   virtsnd_pcm_open() preserves the stale vss->stopped = true (due to
   vss->stopped |= !!vss->msg_count), clears vss->suspended = false, and
   calls virtsnd_pcm_sync_stop() again.

Later commits in this series ("ALSA: virtio: Retain queued messages through
device reset" and "ALSA: virtio: wait through signals for PCM teardown")
detach unused buffers in virtsnd_freeze() and make the control message wait
non-interruptible, but wait_event_interruptible_timeout() in
virtsnd_pcm_sync_stop() remains interruptible and virtsnd_freeze() still does
not clear vss->stopped when virtio_reset_device() resets the device-side
stream state. If virtsnd_pcm_sync_stop() fails during suspend, post-resume
sync_stop() will still send VIRTIO_SND_R_PCM_RELEASE to an unconfigured
stream before SET_PARAMS and block prepare(), hw_params(), and open() if the
reset device rejects RELEASE in its initial state.

[ ... ]

> @@ -393,21 +434,31 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
>  	struct virtio_snd *snd = vss->snd;
>  	struct virtio_snd_msg *msg;
>  	unsigned int js = msecs_to_jiffies(virtsnd_msg_timeout_ms);
> +	bool need_release;
>  	int rc;
>  
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		vss->xfer_enabled = false;
> +		need_release = vss->stopped || vss->msg_count;
> +		if (need_release)
> +			vss->stopped = true;
> +	}
>  	cancel_work_sync(&vss->elapsed_period);
> -
> -	if (!vss->stopped)
> +	if (!need_release)
>  		return 0;
>  
>  	msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_RELEASE,
>  					GFP_KERNEL);
> -	if (!msg)
> +	if (!msg) {
> +		substream->runtime->stop_operating = true;
>  		return -ENOMEM;
> +	}
>  
>  	rc = virtsnd_ctl_msg_send_sync(snd, msg);
> -	if (rc)
> +	if (rc) {
> +		substream->runtime->stop_operating = true;
>  		return rc;
> +	}
>  
>  	/*
>  	 * The spec states that upon receipt of the RELEASE command "the device
> @@ -421,11 +472,13 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
>  	if (rc <= 0) {
>  		dev_warn(&snd->vdev->dev, "SID %u: failed to flush I/O queue\n",
>  			 vss->sid);
> +		substream->runtime->stop_operating = true;
>  
>  		return !rc ? -ETIMEDOUT : rc;
>  	}
>  
> -	vss->stopped = false;
> +	scoped_guard(spinlock_irqsave, &vss->lock)
> +		vss->stopped = false;
>  
>  	return 0;
>  }

[Severity: Medium]
What happens if virtsnd_pcm_sync_stop() succeeds after a failed start, but
the subsequent VIRTIO_SND_R_PCM_PREPARE command in virtsnd_pcm_prepare()
fails?

In sound/core/pcm_native.c:snd_pcm_do_start():

	err = substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_START);
	/* XRUN happened during the start */
	if (err == -EPIPE)
		__snd_pcm_set_state(substream->runtime, SNDRV_PCM_STATE_XRUN);
	return err;

When virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_START) fails with an error other
than -EPIPE (such as -ENOMEM from virtsnd_pcm_ctl_msg_alloc(), or an error
from virtsnd_pcm_msg_send() or virtsnd_ctl_msg_send_sync()),
substream->runtime->state remains SNDRV_PCM_STATE_PREPARED while
virtsnd_pcm_start_failed() sets vss->stopped = true and
runtime->stop_operating = true.

When userspace then calls snd_pcm_prepare() to recover:

1. snd_pcm_do_prepare() calls snd_pcm_sync_stop(substream, true), which
   clears runtime->stop_operating = false and runs virtsnd_pcm_sync_stop().
2. virtsnd_pcm_sync_stop() sends VIRTIO_SND_R_PCM_RELEASE to the device
   (moving the device stream out of the prepared state into the released
   state), waits for queued messages to drain, and clears
   vss->stopped = false.
3. snd_pcm_do_prepare() then calls virtsnd_pcm_prepare(), which allocates and
   sends VIRTIO_SND_R_PCM_PREPARE:

	msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_PREPARE,
					GFP_KERNEL);
	if (!msg)
		return -ENOMEM;

	return virtsnd_ctl_msg_send_sync(vss->snd, msg);

If allocating or sending VIRTIO_SND_R_PCM_PREPARE fails here,
snd_pcm_do_prepare() returns the error without changing
substream->runtime->state. At that point, the device stream is in the
released (unprepared) state, but substream->runtime->state is still
SNDRV_PCM_STATE_PREPARED, runtime->stop_operating is false, and vss->stopped
is false.

Wouldn't a subsequent SNDRV_PCM_IOCTL_START, write(), or
SNDRV_PCM_IOCTL_SYNC_PTR bypass both ALSA's state check and the vss->stopped
check in virtsnd_pcm_trigger(), queueing I/O messages and sending
VIRTIO_SND_R_PCM_START to an unprepared device stream?

Note that a later commit in this series ("ALSA: virtio: stage VZ capture and
restart paused streams") handles virtsnd_pcm_dev_prepare() failure when
pause_restart is true, but leaves the standard (!pause_restart) path
unhandled.

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