Re: [PATCH v8 14/14] ALSA: virtio: synchronize indirect capture positions on reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: virtio: uncompleted hw_ptr rebase on virtsnd_pcm_indirect_reset() error
--

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

ALSA: virtio: synchronize indirect capture positions on reset

This commit adds a dedicated reset operation for indirect capture streams
to synchronize private indirect capture accounting with the core PCM
pointer rebase on reset. It discards unread data, returns the freed RX
buffer space to the virtqueue, and updates the indirect application
pointer to match the rebased hardware pointer.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index ba14f395eef8a..e4a7a4da4d3a8 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -1598,6 +1602,54 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
>  	return virtsnd_pcm_sync_stop_locked(substream);
>  }
>  
> +/* The PCM lock covers both indirect accounting and core completion. */
> +static int virtsnd_pcm_indirect_reset(struct snd_pcm_substream *substream)
> +{
> +	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +	struct virtsnd_pcm_transfer transfer = {};
> +	int rc;
> +
> +	rc = snd_pcm_lib_reset(substream);
> +	if (rc)
> +		return rc;
> +	if (!snd_pcm_running(substream) &&
> +	    runtime->state != SNDRV_PCM_STATE_PAUSED)
> +		return 0;
> +
> +	guard(spinlock_irqsave)(&queue->lock);
> +	guard(spinlock)(&vss->lock);
> +	if (vss->resetting)
> +		return -ECANCELED;
> +	if (!vss->prepared || vss->stopped)
> +		return -EBADFD;
> +
> +	/* Pausing does not move the device's fixed DMA buffer positions. */
> +	if (runtime->state == SNDRV_PCM_STATE_PAUSED)
> +		runtime->status->hw_ptr =
> +			bytes_to_frames(runtime, vss->pcm_indirect.sw_io);
> +
> +	transfer.indirect = vss->pcm_indirect;
> +	transfer.indirect.sw_ready = 0;
> +	/* Consume no application delta while returning discarded RX space. */
> +	transfer.indirect.appl_ptr = runtime->control->appl_ptr;
> +	rc = snd_pcm_indirect_capture_transfer(substream, &transfer.indirect,
> +					       virtsnd_pcm_trans_copy);
> +	if (!rc)
> +		rc = transfer.error;
> +	if (rc) {
> +		virtsnd_pcm_transfer_failed(vss);
> +		return rc;
> +	}

[Severity: High]
What happens to the PCM ring buffer accounting if virtsnd_pcm_indirect_reset()
returns an error after snd_pcm_lib_reset() has already rebased
runtime->status->hw_ptr?

On an indirect capture substream in SNDRV_PCM_STATE_RUNNING or
SNDRV_PCM_STATE_PAUSED that has wrapped past runtime->buffer_size (so
runtime->hw_ptr_base >= runtime->buffer_size and runtime->control->appl_ptr >=
runtime->buffer_size), virtsnd_pcm_indirect_reset() first calls
snd_pcm_lib_reset() (and in the paused state sets runtime->status->hw_ptr from
vss->pcm_indirect.sw_io), which rebases runtime->status->hw_ptr into
[0, runtime->buffer_size).

If virtsnd_pcm_indirect_reset() then returns -ECANCELED, -EBADFD, or
transfer.error, snd_pcm_do_reset() in sound/core/pcm_native.c aborts early
without completing the rest of the rebase:

sound/core/pcm_native.c:snd_pcm_do_reset() {
	...
	err = substream->ops->reset(substream);
	if (err < 0)
		return err;
	runtime->hw_ptr_base = 0;
	runtime->hw_ptr_interrupt = runtime->status->hw_ptr -
		runtime->status->hw_ptr % runtime->period_size;
	runtime->silence_start = runtime->status->hw_ptr;
	runtime->silence_filled = 0;
	runtime->control->appl_ptr = runtime->status->hw_ptr;
	...
}

This leaves runtime->status->hw_ptr rebased below runtime->control->appl_ptr
while runtime->hw_ptr_base and runtime->control->appl_ptr remain at their
pre-reset values, and runtime->state stays in SNDRV_PCM_STATE_RUNNING or
SNDRV_PCM_STATE_PAUSED. Although virtsnd_pcm_transfer_failed() sets
runtime->stop_operating = true on transfer errors, stop_operating is only
acted on by snd_pcm_sync_stop() and is not checked by pcm_accessible_state()
or __snd_pcm_lib_xfer().

In include/sound/pcm.h:snd_pcm_capture_avail():

	snd_pcm_sframes_t avail = runtime->status->hw_ptr - runtime->control->appl_ptr;
	if (avail < 0)
		avail += runtime->boundary;
	return avail;

avail underflows and wraps by runtime->boundary. Wouldn't a subsequent read
via sound/core/pcm_lib.c:__snd_pcm_lib_xfer() see this wrapped avail value and
copy stale DMA ring buffer frames to userspace via writer() before
pcm_accessible_state() or pcm_lib_apply_appl_ptr() stops the transfer?

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