Re: [PATCH v13 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 with sync_stop in virtsnd_pcm_pb_ack()
--

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

ALSA: virtio: Propagate PCM send failures through ACK

This commit restores message length and notifies the virtqueue on partial
send failures in virtsnd_pcm_msg_send(), and propagates those errors through
the PCM ACK callbacks as XRUNs while rejecting ACK calls from invalid PCM
states.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 1091100a4c982..67460bd2a61ca 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
> @@ -466,36 +466,124 @@ 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;
> +}
>  
> -	virtsnd_pcm_msg_send(vss, rec->sw_data, bytes);
> +/* The caller holds the PCM lock, also used by DROP and state transitions. */
> +static int virtsnd_pcm_ack_state(struct snd_pcm_substream *substream)
> +{
> +	switch (substream->runtime->state) {
> +	case SNDRV_PCM_STATE_XRUN:
> +		return -EPIPE;
> +	case SNDRV_PCM_STATE_PREPARED:
> +	case SNDRV_PCM_STATE_RUNNING:
> +	case SNDRV_PCM_STATE_PAUSED:
> +	case SNDRV_PCM_STATE_DRAINING:
> +		return 0;
> +	default:
> +		return -EBADFD;
> +	}
>  }
>  
>  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);
> +	rc = virtsnd_pcm_ack_state(substream);
> +	if (rc)
> +		return rc;
>  
> -	return snd_pcm_indirect_playback_transfer(substream, &vss->pcm_indirect,
> -						  virtsnd_pcm_trans_copy);
> +	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);

[Severity: Medium]
As reported by Sashiko on v12 [1], can virtsnd_pcm_pb_ack() and
virtsnd_pcm_cp_ack() still race with snd_pcm_sync_stop() ->
virtsnd_pcm_sync_stop() on vss->stopped and substream->runtime->stop_operating
when a stream is in SNDRV_PCM_STATE_PREPARED or SNDRV_PCM_STATE_DRAINING?

Because virtio-snd sets vpcm->pcm->nonatomic = true, snd_pcm_prepare() and
snd_pcm_hw_params() hold runtime->buffer_mutex rather than the PCM stream
lock across snd_pcm_sync_stop() and substream->ops->prepare() /
substream->ops->hw_params(). Meanwhile, a concurrent SNDRV_PCM_IOCTL_SYNC_PTR
in snd_pcm_sync_ptr() only acquires the PCM stream lock and calls
pcm_lib_apply_appl_ptr() -> virtsnd_pcm_pb_ack() / virtsnd_pcm_cp_ack().

In virtsnd_pcm_ack_state(), SNDRV_PCM_STATE_SETUP is rejected, which covers
snd_pcm_drop(), but SNDRV_PCM_STATE_PREPARED and SNDRV_PCM_STATE_DRAINING are
allowed:

1. When a capture stream with available frames is drained via
   snd_pcm_drain(), snd_pcm_do_drain_init() stops the stream and places it in
   SNDRV_PCM_STATE_DRAINING with vss->stopped = true and
   runtime->stop_operating = true. Similarly, if snd_pcm_start() fails on a
   later linked stream, snd_pcm_undo_start() stops the earlier stream while
   leaving it in SNDRV_PCM_STATE_PREPARED with vss->stopped = true and
   runtime->stop_operating = true. On a draining capture stream, any read or
   SNDRV_PCM_IOCTL_SYNC_PTR will also see vss->stopped == true in
   virtsnd_pcm_cp_ack() and fail with -EPIPE.

2. When snd_pcm_prepare() or snd_pcm_hw_params() is called from
   SNDRV_PCM_STATE_PREPARED (or snd_pcm_prepare() from
   SNDRV_PCM_STATE_DRAINING on capture), runtime->state is unchanged while
   snd_pcm_sync_stop() clears runtime->stop_operating = false and calls
   virtsnd_pcm_sync_stop():

   sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_sync_stop() {
       if (!vss->stopped)
           return 0;
       ...
       vss->stopped = false;
   }

   While virtsnd_pcm_sync_stop() is sleeping in cancel_work_sync(),
   VIRTIO_SND_R_PCM_RELEASE, or wait_event_interruptible_timeout(), a
   concurrent SNDRV_PCM_IOCTL_SYNC_PTR passes virtsnd_pcm_ack_state(), sees
   vss->stopped == true, calls virtsnd_pcm_transfer_failed(vss) to re-arm
   vss->stopped = true and runtime->stop_operating = true, and returns -EPIPE
   so pcm_lib_apply_appl_ptr() triggers __snd_pcm_xrun(). Conversely, if
   SNDRV_PCM_IOCTL_SYNC_PTR runs right after virtsnd_pcm_sync_stop() clears
   vss->stopped = false and before prepare finishes, ACK sees
   vss->stopped == false and enqueues I/O messages on the released stream.

3. 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") add vss->lock around vss->stopped and gate ACK on
   vss->prepared and vss->preparing, but virtsnd_pcm_prepare_locked() and
   virtsnd_pcm_hw_params() still call virtsnd_pcm_sync_stop_locked() before
   setting vss->preparing = true or clearing vss->prepared = false. While
   virtsnd_pcm_sync_stop_locked() or virtsnd_pcm_pause_restart_sync_stop()
   sleeps in disable_work_sync(), cancel_work_sync(),
   virtsnd_pcm_stop_device(), or virtsnd_pcm_send_sync() for
   VIRTIO_SND_R_PCM_RELEASE, runtime->state remains SNDRV_PCM_STATE_PREPARED,
   vss->preparing is false, and vss->prepared remains true. A concurrent
   SNDRV_PCM_IOCTL_SYNC_PTR can still enqueue messages during
   disable_work_sync() or observe vss->stopped == true during STOP/RELEASE,
   re-arm runtime->stop_operating = true via virtsnd_pcm_transfer_failed(),
   and return -EPIPE to trigger __snd_pcm_xrun().

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

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