Re: [PATCH v12 01/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: race on stopped and stop_operating in virtsnd_pcm_pb_ack()
--

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

ALSA: virtio: Propagate PCM send failures through ACK

This commit updates virtsnd_pcm_msg_send() to roll back the message length
and notify the virtqueue for previously queued messages when adding a message
fails. It also propagates transfer errors through virtsnd_pcm_pb_ack() and
virtsnd_pcm_cp_ack() so ALSA can handle them as XRUNs.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 1091100a4c982..a40ea394bf174 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
> @@ -466,36 +466,100 @@ virtsnd_pcm_cp_pointer(struct snd_pcm_substream *substream)
>  		vss->hw_ptr);
>  }
>  
> +struct virtsnd_pcm_transfer {
> +	struct snd_pcm_indirect indirect;
> +	int error;
> +};
> +
>  static void virtsnd_pcm_trans_copy(struct snd_pcm_substream *substream,
>  				   struct snd_pcm_indirect *rec, size_t bytes)
>  {
>  	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +	struct virtsnd_pcm_transfer *transfer =
> +		container_of(rec, struct virtsnd_pcm_transfer, indirect);
> +
> +	if (transfer->error)
> +		return;
>  
> -	virtsnd_pcm_msg_send(vss, rec->sw_data, bytes);
> +	transfer->error = virtsnd_pcm_msg_send(vss, rec->sw_data, bytes);
> +}
> +
> +/* 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;
>  
> -	guard(spinlock_irqsave)(&queue->lock);
> -	guard(spinlock)(&vss->lock);
> +	scoped_guard(spinlock_irqsave, &queue->lock) {
> +		guard(spinlock)(&vss->lock);
> +		transfer.indirect = vss->pcm_indirect;
> +		if (vss->stopped) {
> +			rc = -EPIPE;
> +		} else {
> +			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;
> +	}

[Severity: Medium]
Can virtsnd_pcm_pb_ack() and virtsnd_pcm_cp_ack() race with
snd_pcm_sync_stop() and virtsnd_pcm_sync_stop() on vss->stopped and
substream->runtime->stop_operating?

Because virtio-snd PCM devices are nonatomic (vpcm->pcm->nonatomic = true),
snd_pcm_sync_stop() is invoked without holding the PCM stream lock
(substream->self_group.mutex), such as from snd_pcm_drop() after dropping
pcm_stream_lock_irq() or from snd_pcm_do_prepare() under
runtime->buffer_mutex. Meanwhile, a concurrent SNDRV_PCM_IOCTL_SYNC_PTR ioctl
in snd_pcm_sync_ptr() acquires only pcm_stream_lock_irq() and calls:

snd_pcm_sync_ptr()
  pcm_lib_apply_appl_ptr()
    virtsnd_pcm_pb_ack() / virtsnd_pcm_cp_ack()

In sound/virtio/virtio_pcm_ops.c, virtsnd_pcm_sync_stop() reads and clears
vss->stopped without holding vss->lock or the PCM stream lock, after
snd_pcm_sync_stop() clears substream->runtime->stop_operating:

static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
{
	...
	cancel_work_sync(&vss->elapsed_period);

	if (!vss->stopped)
		return 0;
	...
	vss->stopped = false;

	return 0;
}

If a concurrent ACK runs while vss->stopped is true, virtsnd_pcm_pb_ack() or
virtsnd_pcm_cp_ack() sets rc = -EPIPE and calls virtsnd_pcm_transfer_failed(),
which writes vss->stopped = true and substream->runtime->stop_operating = true.
That races with virtsnd_pcm_sync_stop() and re-arms stop_operating after
snd_pcm_sync_stop() cleared it, while returning -EPIPE so
pcm_lib_apply_appl_ptr() invokes __snd_pcm_xrun() even when snd_pcm_drop() had
placed the stream in SNDRV_PCM_STATE_SETUP.

Conversely, virtsnd_pcm_pb_ack() and virtsnd_pcm_cp_ack() only check
vss->stopped before transferring data. Once virtsnd_pcm_sync_stop() clears
vss->stopped = false after releasing the stream in SNDRV_PCM_STATE_SETUP, a
subsequent SNDRV_PCM_IOCTL_SYNC_PTR ioctl sees vss->stopped == false and
enqueues virtio I/O messages on a released stream.

Later commits in this series ("ALSA: virtio: Recover PCM state after a failed
start" and "ALSA: virtio: add staged capture and serialize PCM lifecycle
accounting") partially address this by guarding vss->stopped with vss->lock
and gating ACK on vss->prepared and vss->preparing, but do not completely
resolve the race. In virtsnd_pcm_sync_stop_locked() (and
virtsnd_pcm_pause_restart_sync_stop()) as well as virtsnd_pcm_prepare_locked(),
vss->prepared remains true while vss->stopped is true across sleeping teardown
calls (cancel_work_sync(), virtsnd_pcm_stop_device(), and
virtsnd_pcm_send_sync() for VIRTIO_SND_R_PCM_RELEASE), and
virtsnd_pcm_pb_ack() does not check runtime->state. A concurrent
SNDRV_PCM_IOCTL_SYNC_PTR during that window still sees vss->prepared == true
and vss->stopped == true, calls virtsnd_pcm_transfer_failed() to set
runtime->stop_operating = true after snd_pcm_sync_stop() cleared it, and
returns -EPIPE to trigger __snd_pcm_xrun().

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