Re: [PATCH v14 04/12] ALSA: virtio: Propagate PCM send failures through ACK

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: virtio: ack callbacks re-arm stop_operating when stopped with xfer_xrun
--

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

ALSA: virtio: Propagate PCM send failures through ACK

This commit propagates message submission errors from virtsnd_pcm_msg_send()
through the indirect PCM transfer and ACK callbacks, arming stop_operating and
returning -EPIPE on transfer failure. It also adds prepared and preparing state
tracking to gate ACK callbacks across substream teardown, prepare, and reset.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 1091100a4c982..629c738873c9e 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
[ ... ]
> @@ -461,41 +530,165 @@ virtsnd_pcm_cp_pointer(struct snd_pcm_substream *substream)
[ ... ]
> +/* Caller holds vss->lock. */
> +static void virtsnd_pcm_transfer_failed(struct virtio_pcm_substream *vss)
> +{
> +	vss->xfer_enabled = false;
> +	vss->stopped = true;
> +	vss->substream->runtime->stop_operating = true;
> +}
[ ... ]
>  static int virtsnd_pcm_pb_ack(struct snd_pcm_substream *substream)
>  {
>  	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
>  	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +	struct virtsnd_pcm_transfer transfer = {};
> +	int rc;
> +
> +	rc = virtsnd_pcm_ack_state(substream);
> +	if (rc)
> +		return rc;
>  
> -	guard(spinlock_irqsave)(&queue->lock);
> -	guard(spinlock)(&vss->lock);
> +	scoped_guard(spinlock_irqsave, &queue->lock) {
> +		guard(spinlock)(&vss->lock);
> +		if (vss->preparing || !vss->prepared)
> +			return -EBADFD;
> +		if (vss->stopped && !vss->xfer_xrun)
> +			return -EBADFD;
> +		if (vss->xfer_xrun) {
> +			rc = -EPIPE;
> +		} else {
> +			transfer.indirect = vss->pcm_indirect;
> +			rc = snd_pcm_indirect_playback_transfer(substream,
> +							       &transfer.indirect,
> +							       virtsnd_pcm_trans_copy);
> +			if (!rc)
> +				rc = transfer.error;
> +		}
> +		if (rc)
> +			virtsnd_pcm_transfer_failed(vss);
> +		else
> +			vss->pcm_indirect = transfer.indirect;
> +	}
[ ... ]
>  static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
>  {
>  	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
>  	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +	struct virtsnd_pcm_transfer transfer = {};
> +	int rc;
> +
> +	rc = virtsnd_pcm_ack_state(substream);
> +	if (rc)
> +		return rc;
>  
> -	guard(spinlock_irqsave)(&queue->lock);
> -	guard(spinlock)(&vss->lock);
> +	scoped_guard(spinlock_irqsave, &queue->lock) {
> +		guard(spinlock)(&vss->lock);
> +		if (vss->preparing || !vss->prepared)
> +			return -EBADFD;
> +		if (vss->stopped && !vss->xfer_xrun &&
> +		    substream->runtime->state != SNDRV_PCM_STATE_DRAINING)
> +			return -EBADFD;
> +		if (vss->xfer_xrun) {
> +			rc = -EPIPE;
> +		} else {
> +			transfer.indirect = vss->pcm_indirect;
> +			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);

[Severity: Medium]
As reported by Sashiko on v13 [1], in virtsnd_pcm_pb_ack() and
virtsnd_pcm_cp_ack(), can a substream with both vss->stopped and
vss->xfer_xrun set bypass the stopped check during teardown or prepare,
re-arm runtime->stop_operating, and trigger a spurious transition to
SNDRV_PCM_STATE_XRUN?

Both virtsnd_pcm_pb_ack() and virtsnd_pcm_cp_ack() gate the vss->stopped
check on !vss->xfer_xrun:

	if (vss->stopped && !vss->xfer_xrun)
		return -EBADFD;
	if (vss->xfer_xrun) {
		rc = -EPIPE;
	}

When the device reports VIRTIO_SND_EVT_PCM_XRUN, virtsnd_pcm_event() sets
vss->xfer_xrun = true, and neither virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_STOP)
nor virtsnd_pcm_sync_stop() clears vss->xfer_xrun.

If a linked substream is in SNDRV_PCM_STATE_PREPARED (for example, after
snd_pcm_undo_start() stopped it following an XRUN event) or
SNDRV_PCM_STATE_DRAINING (for a capture stream stopped by snd_pcm_drain()),
and snd_pcm_prepare() runs:

snd_pcm_do_prepare(s)
  snd_pcm_sync_stop(s, true)
    s->runtime->stop_operating = false;
    virtsnd_pcm_sync_stop(s)
      virtsnd_pcm_block_ack(s)

Before virtsnd_pcm_block_ack() acquires the PCM stream lock, a concurrent
SNDRV_PCM_IOCTL_SYNC_PTR on s can run pcm_lib_apply_appl_ptr() and invoke
virtsnd_pcm_pb_ack() or virtsnd_pcm_cp_ack(). Because vss->stopped and
vss->xfer_xrun are both true, the ACK callback skips the -EBADFD return,
calls virtsnd_pcm_transfer_failed() to set s->runtime->stop_operating = true
and vss->stopped = true again, and returns -EPIPE so pcm_lib_apply_appl_ptr()
calls __snd_pcm_xrun(s).

If a later linked substream in the group fails prepare, earlier substreams in
the group are left in SNDRV_PCM_STATE_XRUN. If prepare succeeds,
s->runtime->stop_operating remains true on the prepared stream, and a
subsequent snd_pcm_suspend_all() while s is still in SNDRV_PCM_STATE_PREPARED
will call virtsnd_pcm_sync_stop() -> virtsnd_pcm_block_ack() with
vss->stopped == false, leaving vss->preparing = true.

In the later commit in this series ("ALSA: virtio: add staged capture and
serialize PCM lifecycle accounting"), snd_pcm_do_prepare() still calls
snd_pcm_sync_stop() before substream->ops->prepare(), virtsnd_pcm_sync_stop()
returns 0 for SNDRV_PCM_STATE_PREPARED (and virtsnd_pcm_sync_stop_locked()
restores vss->preparing = false before returning while leaving
vss->xfer_xrun = true), virtsnd_pcm_ack_state() drops the
buffer_accessing < 0 check, and virtsnd_pcm_pb_ack_locked() and
virtsnd_pcm_cp_ack_locked() check vss->xfer_xrun before
!vss->prepared || vss->stopped, so this race window remains.

Should virtsnd_pcm_pb_ack() and virtsnd_pcm_cp_ack() check vss->stopped and
return -EBADFD before checking vss->xfer_xrun, or avoid calling
virtsnd_pcm_transfer_failed() when vss->stopped is already set?

[1] https://lore.kernel.org/r/20261103080821.16983-2-mynameisjune111@gmail.com

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