Re: [PATCH v16 10/12] ALSA: virtio: add staged capture and serialize PCM lifecycle accounting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: virtio: virtsnd_pcm_period_elapsed() corrupts linked paused streams
--

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

ALSA: virtio: add staged capture and serialize PCM lifecycle accounting

This commit adds staged capture using request-owned RX storage and
separates produced, observed, and consumed positions for shared capture
clients. It also serializes PCM lifecycle operations with lifecycle_lock
and ACK accounting with the PCM stream lock.

> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> index be3893de40a55..f9b301e4a70ae 100644
> --- a/sound/virtio/virtio_pcm.c
> +++ b/sound/virtio/virtio_pcm.c
> @@ -310,8 +310,119 @@ static void virtsnd_pcm_period_elapsed(struct work_struct *work)
>  {
>  	struct virtio_pcm_substream *vss =
>  		container_of(work, struct virtio_pcm_substream, elapsed_period);
> +	struct snd_pcm_substream *substream = vss->substream;
> +	struct snd_pcm_runtime *runtime;
> +	size_t period_bytes;
> +	u64 period_floor;
> +	u64 snapshot;
> +	bool staged_capture;
> +	bool paused_xrun = false;
> +	bool notify = false;
> +	bool partial = false;
> +	bool requeue = false;
> +
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		staged_capture = vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> +				 vss->capture_staging;
> +	}
> +
> +	/* Serialize the pointer snapshot with ALSA's pointer and ACK callbacks. */
> +	guard(pcm_stream_lock_irqsave)(substream);
> +	if (!substream->runtime)
> +		return;
> +	if (!snd_pcm_running(substream)) {
> +		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);
> +		}
> +		if (paused_xrun) {
> +			/* Stop paused transfers and wake blocked readers. */
> +			snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
> +		}
> +		return;
> +	}

[Severity: High]
How does virtsnd_pcm_period_elapsed() handle paused substreams that are
linked to sibling substreams via SNDRV_PCM_IOCTL_LINK?

When runtime->state is SNDRV_PCM_STATE_PAUSED and vss->xfer_xrun is set,
virtsnd_pcm_period_elapsed() calls virtsnd_pcm_capture_arm_paused_stop(vss)
on only that single substream (setting vss->xfer_enabled = false,
vss->stopped = true, and runtime->stop_operating = true), and then calls
snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN).

Because snd_pcm_stop() runs snd_pcm_action(&snd_pcm_action_stop, ...)
across all substreams in substream->group, and snd_pcm_do_stop() only
invokes ops->trigger(s, SNDRV_PCM_TRIGGER_STOP) and sets
s->runtime->stop_operating = true when snd_pcm_running(s) is true (which is
false for SNDRV_PCM_STATE_PAUSED), snd_pcm_post_stop() transitions any
paused sibling substream to SNDRV_PCM_STATE_XRUN without
virtsnd_pcm_capture_arm_paused_stop() ever running on it.

When userspace subsequently calls SNDRV_PCM_IOCTL_DROP, snd_pcm_drop() sees
the sibling substream already in SNDRV_PCM_STATE_XRUN rather than
SNDRV_PCM_STATE_PAUSED, so snd_pcm_pause(s, false) and snd_pcm_do_stop()
are skipped, and snd_pcm_sync_stop() is also skipped because
s->runtime->stop_operating is false. Does this leave the sibling substream
in SNDRV_PCM_STATE_SETUP while its VirtIO stream remains prepared on the
device with active virtqueue buffers?

In addition, if substream is linked and snd_pcm_stream_group_ref() inside
snd_pcm_stop() contends on group->mutex, it temporarily drops substream's
PCM stream lock to acquire group->mutex in lock order. Can a concurrent
snd_pcm_drop() or snd_pcm_prepare() complete during that window and place
the substreams into SNDRV_PCM_STATE_SETUP or a new SNDRV_PCM_STATE_PREPARED
epoch before snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN) finishes locking
and overwrites runtime->state with SNDRV_PCM_STATE_XRUN?

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