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 <[email protected]> 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/[email protected]?part=10
