Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] ALSA: virtio: checking vss->suspended in sync_stop wedges suspended streams - [High] ALSA: virtio: split reset_epoch update and missing checks corrupt pause state - [High] ALSA: virtio: race and missing RX refill in virtsnd_pcm_ioctl() on RESET - [Medium] ALSA: virtio: missing failure handling and state check in pause retry path - [Medium] ALSA: virtio: missing XRUN wakeup on staged native-pause release failure - [Medium] ALSA: virtio: race between snd_pcm_drop() and virtsnd_pcm_prepare() --
commit 24d284e1ac3e3d50dab01d4de1927240e8a33acb Author: Joonho Lee <[email protected]> ALSA: virtio: serialize capture lifecycle and reset state This patch serializes PCM lifecycle callbacks with a per-substream mutex, rebases staged capture counters on RESET, and adds reset epoch and suspend checks to control and sync-stop paths. > diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c > index 852cc27ac4a45..2fd8595046563 100644 > --- a/sound/virtio/virtio_card.c > +++ b/sound/virtio/virtio_card.c > @@ -354,6 +354,7 @@ static void virtsnd_reset_vqs(struct virtio_snd *snd) > > guard(spinlock_irqsave)(&queue->lock); > guard(spinlock)(&vss->lock); > + vss->reset_epoch++; > vss->prepared = false; > vss->xfer_enabled = false; > vss->suspended = true; [Severity: High] Can incrementing vss->reset_epoch in the first loop of virtsnd_reset_vqs() while resetting vss->pause_restart_state in the fourth loop corrupt the pause-restart state machine if a control operation runs concurrently? In virtsnd_reset_vqs(), the first loop increments vss->reset_epoch and clears vss->prepared, while the fourth loop later resets vss->pause_restart_state: sound/virtio/virtio_card.c:virtsnd_reset_vqs() { ... for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) { cancel_work_sync(&vss->elapsed_period); guard(spinlock_irqsave)(&vss->lock); vss->stopped = false; vss->xfer_xrun = false; vss->pause_restart_state = VIRTSND_PCM_PAUSE_RESTART_NONE; } } If virtsnd_pcm_pause_restart_send() enters between the first and fourth loops, it samples the already-incremented vss->reset_epoch and transitions vss->pause_restart_state to submitted_state. After the fourth loop resets vss->pause_restart_state to VIRTSND_PCM_PAUSE_RESTART_NONE, the post-send check in virtsnd_pcm_pause_restart_send() sees vss->reset_epoch == epoch and vss->pause_restart_state != submitted_state, poisoning vss->pause_restart_state to VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED across virtsnd_restore(). Similarly, in virtsnd_pcm_pause_release_flush(), waking from wait_event_timeout(vss->msg_empty, ...) when virtsnd_reset_vqs() cancels pending RX buffers does not check vss->reset_epoch: if it runs before the fourth loop it transitions to VIRTSND_PCM_PAUSE_RESTART_RELEASED and returns 0 across a reset, and if it runs after the fourth loop it sees vss->pause_restart_state == VIRTSND_PCM_PAUSE_RESTART_NONE and returns -ETIMEDOUT instead of -ECANCELED. In virtsnd_pcm_pause_restart_start(), the two locked blocks after virtsnd_pcm_pause_prepare() also omit vss->reset_epoch checks, allowing vss->prepared = true to be set after the first loop of virtsnd_reset_vqs() or returning -EBADFD instead of -ECANCELED after the fourth loop. [ ... ] > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index 0685a001cf95d..573fab6525a61 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -667,6 +724,14 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream > *substream) > return rc; > } > > +static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream) > +{ > + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > + > + guard(mutex)(&vss->lifecycle_lock); > + return virtsnd_pcm_prepare_locked(substream); > +} [Severity: Medium] Can a concurrent snd_pcm_drop() and snd_pcm_prepare() still race despite vss->lifecycle_lock? In sound/core/pcm_native.c, snd_pcm_drop() stops the stream (setting vss->stopped = true and runtime->stop_operating = true), drops the stream lock, and calls snd_pcm_sync_stop() without holding runtime->buffer_mutex. snd_pcm_sync_stop() clears runtime->stop_operating = false before calling substream->ops->sync_stop(substream) (virtsnd_pcm_sync_stop()), which only then acquires vss->lifecycle_lock. If a concurrent snd_pcm_prepare() runs after snd_pcm_drop() clears runtime->stop_operating = false in snd_pcm_sync_stop() but before virtsnd_pcm_sync_stop() acquires vss->lifecycle_lock, the prepare path's own snd_pcm_sync_stop() sees runtime->stop_operating == false and skips calling virtsnd_pcm_sync_stop(). If virtsnd_pcm_prepare() then acquires vss->lifecycle_lock first, virtsnd_pcm_prepare_locked() does not call virtsnd_pcm_sync_stop_locked() (unlike virtsnd_pcm_hw_params()), so it observes vss->stopped == true and fails with -EBADFD. Conversely, if the delayed virtsnd_pcm_sync_stop() from snd_pcm_drop() acquires vss->lifecycle_lock after a concurrent prepare completes, virtsnd_pcm_sync_stop_locked() now includes vss->prepared in need_release (and virtsnd_pcm_pause_restart_sync_stop() releases VIRTSND_PCM_PAUSE_RESTART_PREPARED), so it sends VIRTIO_SND_R_PCM_RELEASE and clears vss->prepared = false on the newly prepared stream. [ ... ] > @@ -771,20 +849,31 @@ static int virtsnd_pcm_pause_restart_pause(struct > virtio_pcm_substream *vss) > > scoped_guard(spinlock_irqsave, &vss->lock) { > state = vss->pause_restart_state; > - if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASED) > + if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASED) { > + if (vss->xfer_xrun) > + virtsnd_pcm_schedule_period_elapsed(vss); > return vss->xfer_xrun ? -EPIPE : 0; > + } > vss->xfer_enabled = false; > vss->stopped = true; > } > > if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT) { > - rc = virtsnd_pcm_pause_restart_sync_stop(vss->substream); > - if (!rc) { > - guard(spinlock_irqsave)(&vss->lock); > - vss->stopped = true; > - if (vss->xfer_xrun) > - rc = -EPIPE; > + unsigned int js = msecs_to_jiffies(virtsnd_msg_timeout_ms); > + > + /* Trigger holds the PCM lock: never cancel the period worker > here. */ > + if (!wait_event_timeout(vss->msg_empty, > + !virtsnd_pcm_msg_pending_num(vss), js)) > + return -ETIMEDOUT; > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (vss->suspended) > + return -ECANCELED; > + vss->prepared = false; > + vss->pause_restart_state = > VIRTSND_PCM_PAUSE_RESTART_RELEASED; > + rc = vss->xfer_xrun ? -EPIPE : 0; > } > + if (rc) > + virtsnd_pcm_pause_restart_failed(vss); > return rc; > } [Severity: Medium] In the VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT branch of virtsnd_pcm_pause_restart_pause(), does returning -ETIMEDOUT directly on wait_event_timeout() failure skip calling virtsnd_pcm_pause_restart_failed()? Previously this path called virtsnd_pcm_pause_restart_sync_stop(), which invoked virtsnd_pcm_pause_restart_fail(vss, false) on timeout to set vss->xfer_xrun = true, schedule vss->elapsed_period, and set substream->runtime->stop_operating = true. In the new open-coded path, timing out leaves vss->stopped = true while runtime->stop_operating and vss->xfer_xrun remain false. Also, when wait_event_timeout() returns non-zero, should the locked block re-verify that vss->msg_count == 0 and vss->pause_restart_state == VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT before setting VIRTSND_PCM_PAUSE_RESTART_RELEASED, matching virtsnd_pcm_pause_release_flush() and virtsnd_pcm_pause_restart_sync_stop()? [ ... ] > @@ -945,10 +1039,12 @@ static int virtsnd_pcm_trigger(struct > snd_pcm_substream *substream, int command) > return -ENOMEM; > } > > - rc = virtsnd_ctl_msg_send_sync(snd, msg); > - if (rc) > + rc = virtsnd_pcm_send_sync(vss, msg); > + if (rc && rc != -ECANCELED) > virtsnd_pcm_start_failed(vss); > > + if (!rc && vss->capture_staging) > + virtsnd_pcm_schedule_period_elapsed(vss); > return rc; [Severity: Medium] When SNDRV_PCM_TRIGGER_PAUSE_RELEASE fails for a staged capture stream in native-pause mode (vss->capture_staging && !virtsnd_pcm_pause_restart_enabled(vss)), does calling virtsnd_pcm_start_failed() leave paused capture readers sleeping without an XRUN notification? When virtsnd_pcm_capture_send() fails earlier in virtsnd_pcm_trigger() on SNDRV_PCM_TRIGGER_PAUSE_RELEASE, the error path sets vss->xfer_xrun = true and calls virtsnd_pcm_schedule_period_elapsed(vss): sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trigger() { ... if (rc) { vss->stopped = true; substream->runtime->stop_operating = true; if (vss->capture_staging && vss->direction == SNDRV_PCM_STREAM_CAPTURE && command == SNDRV_PCM_TRIGGER_PAUSE_RELEASE) { /* Pause undo leaves ALSA paused; wake its readers. */ vss->xfer_xrun = true; virtsnd_pcm_schedule_period_elapsed(vss); } return rc; } } However, if SNDRV_PCM_TRIGGER_PAUSE_RELEASE fails either at the initial (vss->stopped || !vss->prepared) check or when allocating/sending VIRTIO_SND_R_PCM_START via virtsnd_pcm_start_failed(), vss->xfer_xrun is never set and vss->elapsed_period is not scheduled, so ALSA's pause undo leaves runtime->state in SNDRV_PCM_STATE_PAUSED without transitioning the stream to XRUN. [ ... ] > @@ -1135,8 +1244,12 @@ static int virtsnd_pcm_sync_stop(struct > snd_pcm_substream *substream) > return !rc ? -ETIMEDOUT : rc; > } > > - scoped_guard(spinlock_irqsave, &vss->lock) > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (vss->suspended) > + return -ECANCELED; > + vss->prepared = false; > vss->stopped = false; > + } > > return 0; > } [Severity: High] Won't returning -ECANCELED when vss->suspended is true in virtsnd_pcm_sync_stop_locked() and virtsnd_pcm_pause_restart_sync_stop() permanently wedge the substream after SNDRV_PCM_TRIGGER_SUSPEND? During system suspend, snd_pcm_suspend_all() triggers SNDRV_PCM_TRIGGER_SUSPEND, which in virtsnd_pcm_trigger() and virtsnd_pcm_pause_restart_stop(vss, true) sets both vss->suspended = true and vss->stopped = true. When virtsnd_pcm_sync_stop_locked() (or virtsnd_pcm_pause_restart_sync_stop()) then runs, it sends VIRTIO_SND_R_PCM_RELEASE and flushes the queue, but returns -ECANCELED because vss->suspended is true, leaving vss->stopped = true and vss->prepared = true. Because this patch also removes vss->suspended = false from virtsnd_pcm_open(), vss->suspended is now only cleared in virtsnd_pcm_prepare_locked() after the vss->stopped check: sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_prepare_locked() { ... scoped_guard(spinlock_irqsave, &vss->lock) stopped = vss->stopped; if (stopped) { dev_err(&vdev->dev, "SID %u: stream release failed\n", vss->sid); substream->runtime->stop_operating = true; return -EBADFD; } ... scoped_guard(spinlock_irqsave, &vss->lock) { ... vss->suspended = false; } Once vss->stopped and vss->suspended are both true: - virtsnd_pcm_prepare_locked() sees vss->stopped == true, sets runtime->stop_operating = true, and returns -EBADFD before reaching vss->suspended = false. - virtsnd_pcm_hw_params() calls virtsnd_pcm_sync_stop_locked(), which sees need_release (vss->stopped is true) and vss->suspended == true and fails with -ECANCELED. - Closing and reopening the PCM device calls virtsnd_pcm_open(), which no longer clears vss->suspended and calls virtsnd_pcm_sync_stop(), failing open with -ECANCELED. In addition, virtsnd_pcm_pause_restart_fail() and virtsnd_pcm_start_failed() now return early when vss->suspended is true. In virtsnd_pcm_prepare_locked(), the else branch of if (!suspended) calls virtsnd_pcm_dev_set_params() while vss->suspended is still true; if virtsnd_pcm_dev_set_params() fails, calling virtsnd_pcm_pause_restart_fail(vss, false) is a no-op that skips setting vss->stopped, vss->xfer_xrun, and runtime->stop_operating. Likewise, after virtsnd_freeze() and virtsnd_restore(), vss->suspended remains true until prepare succeeds, so a failed resume via SNDRV_PCM_TRIGGER_PAUSE_RELEASE causes virtsnd_pcm_pause_restart_fail() to return early without setting vss->xfer_xrun, scheduling vss->elapsed_period, or setting runtime->stop_operating. [ ... ] > @@ -1415,12 +1531,52 @@ static int virtsnd_pcm_cp_ack(struct > snd_pcm_substream *substream) > return 0; > } > > +static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream) > +{ > + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > + > + guard(mutex)(&vss->lifecycle_lock); > + return virtsnd_pcm_sync_stop_locked(substream); > +} > + > +static int virtsnd_pcm_ioctl(struct snd_pcm_substream *substream, > + unsigned int cmd, void *arg) > +{ > + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > + struct snd_pcm_runtime *runtime = substream->runtime; > + size_t position; > + size_t period; > + int rc; > + > + if (cmd != SNDRV_PCM_IOCTL1_RESET || !vss->capture_staging) > + return snd_pcm_lib_ioctl(substream, cmd, arg); > + > + rc = snd_pcm_lib_ioctl(substream, cmd, arg); > + if (rc) > + return rc; > + guard(pcm_stream_lock_irqsave)(substream); > + runtime->status->hw_ptr %= runtime->buffer_size; > + position = frames_to_bytes(runtime, runtime->status->hw_ptr); > + period = snd_pcm_lib_period_bytes(substream); > + guard(spinlock_irqsave)(&vss->lock); > + vss->hw_ptr = position; > + vss->capture_copied_total = position; > + vss->capture_acked_total = position; > + vss->capture_pointer_seen_total = position; > + vss->capture_ready = 0; > + vss->capture_notified_periods = period ? position / period : 0; > + vss->capture_callback_snapshot = 0; > + vss->capture_callback_snapshot_active = false; > + vss->pcm_indirect.appl_ptr = runtime->status->hw_ptr; > + return 0; > +} [Severity: High] What prevents virtsnd_pcm_ioctl() on SNDRV_PCM_IOCTL1_RESET from racing with RX completions and virtsnd_pcm_period_elapsed() on a running staged capture stream, or leaving RX buffers unqueued after clearing vss->capture_ready? When SNDRV_PCM_IOCTL_RESET is called while the stream is in SNDRV_PCM_STATE_RUNNING: 1. snd_pcm_lib_ioctl() -> snd_pcm_lib_ioctl_reset() acquires the PCM stream lock, updates runtime->status->hw_ptr via virtsnd_pcm_cp_pointer(), reduces runtime->status->hw_ptr modulo runtime->buffer_size, and drops the PCM stream lock. Before virtsnd_pcm_ioctl() acquires vss->lock, virtsnd_pcm_capture_complete() can run in RX interrupt context: it checks ring capacity against the pre-reset vss->capture_ready (which can trigger a false ring_full XRUN) and advances vss->hw_ptr and vss->capture_copied_total as new audio is copied. virtsnd_pcm_ioctl() then overwrites vss->hw_ptr and vss->capture_copied_total with the older position sampled before vss->lock was taken, rewinding vss->hw_ptr over newly captured audio. 2. Between snd_pcm_lib_ioctl_reset() dropping the stream lock, virtsnd_pcm_ioctl() returning, and snd_pcm_do_reset() / snd_pcm_post_reset() re-acquiring the stream lock to zero runtime->hw_ptr_base and set runtime->control->appl_ptr = runtime->status->hw_ptr, virtsnd_pcm_period_elapsed() can run on system_highpri_wq. If virtsnd_pcm_period_elapsed() advances runtime->status->hw_ptr before snd_pcm_post_reset() sets runtime->control->appl_ptr = runtime->status->hw_ptr, runtime->control->appl_ptr is moved ahead of vss->pcm_indirect.appl_ptr and vss->capture_acked_total without an ACK callback, leaving vss->capture_ready inflated and causing subsequent ACKs or completions to fail with -EPIPE. 3. In bounded capture mode (!vss->capture_free_running), virtsnd_pcm_capture_enqueue() throttles queued RX descriptors based on vss->buffer_bytes - vss->capture_ready. After virtsnd_pcm_ioctl() resets vss->capture_ready = 0 on a running stream, nothing calls virtsnd_pcm_capture_send(vss) under queue->lock and vss->lock to refill held-back RX requests. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
