Re: [PATCH v5 11/11] ALSA: virtio-snd: fix dsnoop capture with staged buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: virtio-snd: un-armed paused stop on read XRUN in virtsnd_pcm_cp_ack()
--

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

ALSA: virtio-snd: fix dsnoop capture with staged buffers

This commit adds free-running capture mode support for staged capture buffers
when stop_threshold equals the boundary, allowing dsnoop clients to consume
the mmap ring without triggering capacity rejections. It also handles transitions
back to bounded capture and arms synchronous stop on paused XRUN paths.

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 663f7695b5be8..e7258bce2e6f0 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
[ ... ]
> @@ -508,24 +511,54 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
>  	return 0;
>  }
>  
[ ... ]
> +/* Caller holds vss->lock. */
> +int virtsnd_pcm_capture_refresh_mode_locked(struct virtio_pcm_substream *vss,
> +					    bool *free_running)
> +{
> +	struct snd_pcm_runtime *runtime;
> +	snd_pcm_uframes_t stop_threshold;
> +	u64 outstanding;
> +	bool new_free_running;
> +
> +	if (!vss->capture_staging || !vss->runtime_active || !vss->substream)
> +		return -EBADFD;
> +
> +	runtime = vss->substream->runtime;
> +	if (!runtime || !vss->buffer_bytes || !vss->capture_boundary)
> +		return -EBADFD;
> +
> +	/* Validate the currently committed policy before observing a transition. */
> +	if (!virtsnd_pcm_capture_counters_valid(vss))
> +		return -EIO;
> +
> +	stop_threshold = READ_ONCE(runtime->stop_threshold);
> +	new_free_running = stop_threshold == vss->capture_boundary;
> +	if (new_free_running != vss->capture_free_running) {
> +		outstanding = vss->capture_copied_total -
> +			      vss->capture_acked_total;
> +		if (new_free_running) {
> +			vss->capture_ready = 0;
> +		} else {
> +			if (outstanding >= vss->buffer_bytes)
> +				return -EPIPE;
> +			vss->capture_ready = outstanding;
> +		}
> +		vss->capture_free_running = new_free_running;
> +	}
> +
> +	*free_running = new_free_running;
> +	return 0;
> +}
[ ... ]
> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index d50924b792f65..cbf4e479d295f 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
[ ... ]
> @@ -1314,8 +1348,14 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
>  	struct virtsnd_pcm_transfer transfer = {};
>  	int rc;
>  
> -	if (vss->capture_staging)
> -		return virtsnd_pcm_capture_staged_ack(vss, runtime);
> +	if (vss->capture_staging) {
> +		rc = virtsnd_pcm_capture_staged_ack(vss, runtime);
> +		if (rc == -EPIPE) {
> +			guard(spinlock_irqsave)(&vss->lock);
> +			virtsnd_pcm_capture_arm_paused_stop(vss);
> +		}
> +		return rc;
> +	}

[Severity: Medium]
Can a paused staged-capture stream still enter SNDRV_PCM_STATE_XRUN during a
partial read without arming vss->stopped and runtime->stop_operating?

If a staged-capture stream is paused in SNDRV_PCM_STATE_PAUSED with unread
frames in the ring (where outstanding < vss->buffer_bytes), and userspace
lowers runtime->stop_threshold via snd_pcm_sw_params() from boundary to a
bounded threshold smaller than the frames that will remain after a partial
read, snd_pcm_sw_params() does not call snd_pcm_update_state() because
snd_pcm_running(substream) is false while paused.

When userspace then performs a partial read through __snd_pcm_lib_xfer() that
leaves remaining avail >= runtime->stop_threshold:

  __snd_pcm_lib_xfer()
    pcm_lib_apply_appl_ptr()
      virtsnd_pcm_cp_ack()
        virtsnd_pcm_capture_staged_ack()
          virtsnd_pcm_capture_refresh_mode_locked()

virtsnd_pcm_capture_refresh_mode_locked() in sound/virtio/virtio_pcm_msg.c
only checks outstanding >= vss->buffer_bytes when transitioning from
free-running to bounded mode, so virtsnd_pcm_capture_staged_ack() succeeds
with rc == 0. Because virtsnd_pcm_cp_ack() in sound/virtio/virtio_pcm_ops.c
only calls virtsnd_pcm_capture_arm_paused_stop(vss) when rc == -EPIPE, it
returns 0 without setting vss->stopped or runtime->stop_operating.

Immediately afterward at the end of __snd_pcm_lib_xfer() in
sound/core/pcm_lib.c:

  _end_unlock:
    runtime->twake = 0;
    if (xfer > 0 && err >= 0)
        snd_pcm_update_state(substream, runtime);

snd_pcm_update_state() sees avail >= runtime->stop_threshold and calls
__snd_pcm_xrun() -> snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN). Because
snd_pcm_running() is false in SNDRV_PCM_STATE_PAUSED, snd_pcm_do_stop() skips
trigger(STOP) and does not set runtime->stop_operating = true.

On the subsequent recovery call to snd_pcm_prepare(), snd_pcm_sync_stop() is
skipped and virtsnd_pcm_prepare() in sound/virtio/virtio_pcm_ops.c fails with
-EBADFD on vss->stopped (in pause-restart mode) or
virtsnd_pcm_msg_pending_num(vss) (in native pause mode):

  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)) {
          dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n",
                  vss->sid);
          return -EBADFD;
      }

Should virtsnd_pcm_cp_ack() also check whether a paused stream has remaining
capture avail >= runtime->stop_threshold (or check stop_threshold in
virtsnd_pcm_capture_staged_ack()) so virtsnd_pcm_capture_arm_paused_stop() is
armed before snd_pcm_update_state() moves the paused stream to
SNDRV_PCM_STATE_XRUN?

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