Re: [PATCH v9 14/14] ALSA: virtio: rebase ordinary capture accounting on RESET

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] ALSA: virtio: virtsnd_pcm_indirect_reset() schedules work during device reset
- [High] ALSA: virtio: paused virtsnd_pcm_indirect_reset() ignores unconsumed hw_ptr
- [High] ALSA: virtio: virtsnd_pcm_msg_complete() clears XRUN latched by indirect reset
- [Medium] ALSA: virtio: snd_pcm_lib_reset() ACK refill failure bypasses indirect reset
--

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

ALSA: virtio: rebase ordinary capture accounting on RESET

This patch adds virtsnd_pcm_indirect_reset() and selects
virtsnd_pcm_capture_indirect_ops for non-staged capture streams to keep
indirect capture accounting synchronized with the ALSA core during
SNDRV_PCM_IOCTL_RESET. When refilling RX buffers fails after
snd_pcm_lib_reset() mutates hw_ptr, it latches XRUN and schedules
elapsed_period so the core can finish rebasing appl_ptr before reporting
the error.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index c3d272163907d..ba255803c6577 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -1693,6 +1697,62 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
>  	return virtsnd_pcm_sync_stop_locked(substream);
>  }
>  
> +/*
> + * The PCM lock covers indirect accounting and core completion. Once lib_reset
> + * changes hw_ptr, complete the core rebase even if refilling fails. The latched
> + * XRUN rejects subsequent pointer/ACK operations; the worker publishes XRUN to
> + * paused streams too. RESET success therefore does not imply stream recovery.
> + */
> +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;

[Severity: Medium]
What happens if RX virtqueue refilling fails inside virtsnd_pcm_cp_ack()
while snd_pcm_lib_reset() is updating the pointer on a running stream?

When snd_pcm_running(substream) is true, the call chain is:

  virtsnd_pcm_indirect_reset()
    -> snd_pcm_lib_reset()
      -> snd_pcm_update_hw_ptr()
        -> virtsnd_pcm_cp_pointer()
          -> snd_pcm_indirect_capture_pointer()
            -> virtsnd_pcm_cp_ack()

In include/sound/pcm-indirect.h:snd_pcm_indirect_capture_pointer(), rec
fields are mutated before invoking substream->ops->ack():

	rec->hw_io = ptr;
	rec->hw_ready += bytes;
	qsize = rec->hw_queue_size ? rec->hw_queue_size : rec->hw_buffer_size;
	if (rec->hw_ready > qsize)
		return SNDRV_PCM_POS_XRUN;
	rec->sw_io += bytes;
	if (rec->sw_io >= rec->sw_buffer_size)
		rec->sw_io -= rec->sw_buffer_size;
	if (substream->ops->ack) {
		err = substream->ops->ack(substream);
		if (err == -EPIPE)
			return SNDRV_PCM_POS_XRUN;
	}

Whenever rec->hw_ready > 0 and rec->sw_ready < rec->sw_buffer_size,
virtsnd_pcm_cp_ack() runs snd_pcm_indirect_capture_transfer() and attempts
to refill the RX virtqueue before snd_pcm_lib_reset() returns.

If virtsnd_pcm_msg_send() fails during that ack callback,
snd_pcm_indirect_capture_pointer() returns SNDRV_PCM_POS_XRUN,
snd_pcm_update_hw_ptr0() transitions runtime->state to
SNDRV_PCM_STATE_XRUN and returns -EPIPE, and snd_pcm_lib_reset() executes:

	if (snd_pcm_running(substream) && snd_pcm_update_hw_ptr(substream) >= 0)
		runtime->status->hw_ptr %= runtime->buffer_size;
	else {
		runtime->status->hw_ptr = 0;
		runtime->hw_ptr_wrap = 0;
	}

virtsnd_pcm_indirect_reset() then sees !snd_pcm_running(substream) and
runtime->state != SNDRV_PCM_STATE_PAUSED and returns 0 early, so
snd_pcm_do_reset() sets runtime->control->appl_ptr = 0 while
vss->pcm_indirect retains its partially updated hw_io, hw_ready, and sw_io
values.

Should virtsnd_pcm_cp_ack() be suppressed during snd_pcm_lib_reset() (as
staged capture does with vss->capture_reset_observing) so that refilling
only happens once in virtsnd_pcm_indirect_reset() after hw_ptr is rebased?

> +
> +	guard(spinlock_irqsave)(&queue->lock);
> +	guard(spinlock)(&vss->lock);
> +	if (vss->resetting || !vss->prepared || vss->stopped) {
> +		vss->xfer_xrun = true;
> +		virtsnd_pcm_schedule_period_elapsed(vss);
> +		return 0;
> +	}

[Severity: Critical]
Can scheduling vss->elapsed_period here when vss->resetting is true cause
a use-after-free on substream->runtime during substream close?

Unlike virtsnd_pcm_reset(), which returns 0 immediately when
vss->resetting is true:

	if (vss->resetting)
		return 0;

virtsnd_pcm_indirect_reset() calls virtsnd_pcm_schedule_period_elapsed(vss)
when vss->resetting is true (which can happen after virtsnd_freeze() /
virtsnd_reset_vqs() has already set vss->resetting = true and finished its
own cancel_work_sync(&vss->elapsed_period)).

If userspace then closes the substream while vss->resetting is still true,
virtsnd_pcm_sync_stop_locked() returns -ECANCELED before reaching
cancel_work_sync(&vss->elapsed_period):

	scoped_guard(spinlock_irqsave, &vss->lock) {
		if (vss->resetting)
			return -ECANCELED;
		...
	}
	cancel_work_sync(&vss->elapsed_period);

In addition, both virtsnd_pcm_hw_free() and virtsnd_pcm_close() only call
cancel_work_sync(&vss->elapsed_period) when vss->capture_staging is true:

	if (vss->capture_staging) {
		scoped_guard(spinlock_irqsave, &vss->lock) {
			vss->runtime_active = false;
			vss->xfer_enabled = false;
		}
		cancel_work_sync(&vss->elapsed_period);
	}

When snd_pcm_release_substream() subsequently calls
snd_pcm_detach_substream(), runtime is freed without holding
pcm_stream_lock, while virtsnd_pcm_period_elapsed() may still be queued or
running and dereferencing substream->runtime in
sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed().

Also, if this branch is taken when vss->resetting is false and
!vss->prepared && !vss->stopped, virtsnd_pcm_transfer_failed(vss) is not
called, leaving vss->stopped false so a subsequent virtsnd_pcm_prepare()
skips virtsnd_pcm_sync_stop_locked() without canceling the scheduled work.

> +
> +	/* 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;

[Severity: High]
When runtime->state == SNDRV_PCM_STATE_PAUSED, does using
vss->pcm_indirect.sw_io and vss->pcm_indirect.hw_ready directly miss RX
completions that already advanced vss->hw_ptr?

For ordinary capture (!vss->capture_staging), RX completions in
sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_msg_complete() advance
vss->hw_ptr and clear msg->length = 0 under vss->lock without updating
vss->pcm_indirect:

		vss->hw_ptr += data_bytes;
	}

	if (vss->hw_ptr >= vss->buffer_bytes)
		vss->hw_ptr -= vss->buffer_bytes;

	msg->length = 0;

vss->pcm_indirect (hw_io, sw_io, and hw_ready) is only updated from
vss->hw_ptr when virtsnd_pcm_cp_pointer() calls
snd_pcm_indirect_capture_pointer().

When a stream is paused via snd_pcm_pause(), the core does not call
snd_pcm_update_hw_ptr(). Any RX completion whose elapsed_period work runs
after runtime->state becomes SNDRV_PCM_STATE_PAUSED exits early in
sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed():

	if (!snd_pcm_running(substream)) {
		...
		return;
	}

And any RX completion arriving during SNDRV_PCM_TRIGGER_PAUSE_PUSH while
virtsnd_pcm_stop_device() waits for VIRTIO_SND_R_PCM_STOP also advances
vss->hw_ptr without updating vss->pcm_indirect.

When virtsnd_pcm_indirect_reset() then runs in SNDRV_PCM_STATE_PAUSED:
1. snd_pcm_lib_reset() skips snd_pcm_update_hw_ptr() because
   snd_pcm_running(substream) is false, and sets runtime->status->hw_ptr = 0.
2. virtsnd_pcm_indirect_reset() sets runtime->status->hw_ptr from the stale
   vss->pcm_indirect.sw_io and runs snd_pcm_indirect_capture_transfer()
   without folding the unconsumed delta (vss->hw_ptr - vss->pcm_indirect.hw_io)
   into hw_io, sw_io, and hw_ready.

As a result, completed RX messages between hw_io and vss->hw_ptr are not
counted in hw_ready and are not re-submitted to the RX virtqueue,
hw_ptr/appl_ptr are rebased to the stale sw_io instead of vss->hw_ptr, and
after resume the next virtsnd_pcm_cp_pointer() call sees those pre-RESET
bytes as newly captured data. (Note also that if the earlier
vss->resetting || !vss->prepared || vss->stopped check returns 0 while
paused, runtime->status->hw_ptr is left at 0 from snd_pcm_lib_reset()
rather than restored from sw_io.)

> +	/* 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);
> +		vss->xfer_xrun = true;
> +		virtsnd_pcm_schedule_period_elapsed(vss);
> +		return 0;
> +	}

[Severity: High]
Can an in-flight RX completion in virtsnd_pcm_msg_complete() clear
vss->xfer_xrun before virtsnd_pcm_period_elapsed() or subsequent pointer
checks observe the latched XRUN?

When snd_pcm_indirect_capture_transfer() fails here (or on the earlier
error path), virtsnd_pcm_transfer_failed(vss) sets vss->xfer_enabled = false
and vss->stopped = true, but it does not stop the VirtIO device or flush RX
buffers already queued in the virtqueue (including any messages queued and
kicked by earlier iterations of virtsnd_pcm_trans_copy() before a later
period failed).

In sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_msg_complete(), any completing
non-staged message unconditionally clears vss->xfer_xrun under vss->lock:

	msg->length = 0;

	vss->xfer_xrun = false;
	vss->msg_count--;

If an in-flight RX message completes before the scheduled
virtsnd_pcm_period_elapsed() worker runs, vss->xfer_xrun is reset to false.
In sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed():

		scoped_guard(spinlock_irqsave, &vss->lock) {
			runtime = substream->runtime;
			paused_xrun = runtime &&
				      runtime->state == SNDRV_PCM_STATE_PAUSED &&
				      vss->xfer_xrun;
			if (paused_xrun)
				virtsnd_pcm_capture_arm_paused_stop(vss);
		}

paused_xrun then evaluates to false, so a paused stream never transitions
to SNDRV_PCM_STATE_XRUN. Because RESET rebased appl_ptr = hw_ptr
(avail == 0), subsequent reads on the paused stream never invoke pointer()
or ack(), leaving nonblocking reads returning -EAGAIN indefinitely and
blocking reads hanging instead of returning -EPIPE. Furthermore, because
this error return skips updating vss->pcm_indirect, vss->pcm_indirect is
left out of sync with the rebased runtime->control->appl_ptr.

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