Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] ALSA: virtio: deadlock and broken retry in virtsnd_pcm_pause_restart_pause() - [High] ALSA: virtio: XRUN wakeup suppressed in virtsnd_pcm_pause_restart_fail() - [High] ALSA: virtio: premature paused stop arming in virtsnd_pcm_cp_ack() - [High] ALSA: virtio: post-reset substream state clobbered after virtsnd_reset_vqs() - [High] ALSA: virtio: desynchronized prepared and stopped state on prepare and stop - [High] ALSA: virtio: missing paused stop arming in non-staged virtsnd_pcm_cp_ack() --
commit ff9dabc7b348389a68870e8f3710dbf75bdbd0a8 Author: Joonho Lee <[email protected]> ALSA: virtio: fence staged capture state and recovery This commit tracks device preparation under the substream lock, fences virtqueue submissions and state transitions across pause, reset, and open failure paths, and updates staged capture ACK and XRUN recovery handling. > diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c > index df7091b74a521..852cc27ac4a45 100644 > --- a/sound/virtio/virtio_card.c > +++ b/sound/virtio/virtio_card.c > @@ -348,6 +348,16 @@ static void virtsnd_reset_vqs(struct virtio_snd *snd) > struct virtio_device *vdev = snd->vdev; > unsigned int i; > > + for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) { > + struct virtio_pcm_substream *vss = &snd->substreams[i]; > + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss); > + > + guard(spinlock_irqsave)(&queue->lock); > + guard(spinlock)(&vss->lock); > + vss->prepared = false; > + vss->xfer_enabled = false; > + vss->suspended = true; > + } > virtio_reset_device(vdev); > > /* Deleting a virtqueue does not unmap its outstanding descriptors. */ > @@ -364,9 +374,20 @@ static void virtsnd_reset_vqs(struct virtio_snd *snd) > spin_unlock_irqrestore(&queue->lock, flags); > } > > - vdev->config->del_vqs(vdev); > - for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i) > + for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i) { > + guard(spinlock_irqsave)(&snd->queues[i].lock); > snd->queues[i].vqueue = NULL; > + } > + vdev->config->del_vqs(vdev); > + for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) { > + struct virtio_pcm_substream *vss = &snd->substreams[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; > + } > } [Severity: High] Can resetting vss->stopped, vss->xfer_xrun, and vss->pause_restart_state inside virtsnd_reset_vqs() get clobbered by in-flight control message senders that are only woken up afterwards? In virtsnd_freeze() (and virtsnd_remove()), the teardown sequence runs: virtsnd_ctl_msg_cancel_all(snd); virtsnd_reset_vqs(snd); virtsnd_ctl_msg_cleanup(snd); virtsnd_ctl_msg_cancel_all() marks pending control messages with msg->cancelled = true, but it does not complete msg->notify. The threads waiting in virtsnd_ctl_msg_send_sync() are only woken later when virtsnd_ctl_msg_cleanup() calls virtsnd_ctl_msg_complete(). When a thread blocked in virtsnd_pcm_pause_restart_send() wakes up after virtsnd_reset_vqs() has already reset the substream state, virtsnd_ctl_msg_uncertain(msg, rc) returns true and vss->pause_restart_state (now VIRTSND_PCM_PAUSE_RESTART_NONE) no longer matches submitted_state: scoped_guard(spinlock_irqsave, &vss->lock) { if ((rc && uncertain) || vss->pause_restart_state != submitted_state) { vss->pause_restart_state = VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED; if (!rc) rc = -EIO; } ... } The woken caller then runs virtsnd_pcm_pause_restart_fail() (or virtsnd_pcm_start_failed() in non-pause-restart mode), overwriting vss->pause_restart_state with VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED and setting vss->stopped = true and vss->xfer_xrun = true after the reset has already completed, leaving the substream wedged after virtsnd_restore(). [ ... ] > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index c7e85b151c5c4..0685a001cf95d 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -223,7 +227,8 @@ virtsnd_pcm_pause_restart_fail(struct > virtio_pcm_substream *vss, > bool notify_xrun; > > scoped_guard(spinlock_irqsave, &vss->lock) { > - notify_xrun = !vss->xfer_xrun; > + notify_xrun = vss->runtime_active && vss->xfer_enabled && > + !vss->xfer_xrun; > vss->xfer_enabled = false; > vss->stopped = true; > vss->xfer_xrun = true; [Severity: High] Does requiring vss->xfer_enabled in virtsnd_pcm_pause_restart_fail() suppress XRUN notifications when SNDRV_PCM_TRIGGER_PAUSE_RELEASE or SNDRV_PCM_TRIGGER_PAUSE_PUSH fails? When a stream is paused, vss->xfer_enabled is false. During SNDRV_PCM_TRIGGER_PAUSE_RELEASE in virtsnd_pcm_pause_restart_start(), vss->xfer_enabled remains false until virtsnd_pcm_pause_prepare() and virtsnd_pcm_capture_send() both succeed: if (resume) { rc = virtsnd_pcm_pause_prepare(vss); if (rc) { virtsnd_pcm_pause_restart_failed(vss); return rc; } ... } scoped_guard(spinlock_irqsave, &queue->lock) { guard(spinlock)(&vss->lock); ... rc = virtsnd_pcm_capture_send(vss); if (!rc) vss->xfer_enabled = true; } if (rc) { virtsnd_pcm_pause_restart_failed(vss); return rc; } If either step fails, virtsnd_pcm_pause_restart_fail() sees vss->xfer_enabled == false and skips virtsnd_pcm_schedule_period_elapsed(). Because snd_pcm_action_single() runs snd_pcm_undo_pause() (where vss->xfer_xrun is already true) and skips snd_pcm_post_pause(), the stream stays in SNDRV_PCM_STATE_PAUSED with vss->xfer_xrun latched to true and virtsnd_pcm_period_elapsed() is never scheduled to transition it to SNDRV_PCM_STATE_XRUN and wake blocked readers. Similarly, in virtsnd_pcm_pause_restart_pause(), vss->xfer_enabled is cleared to false at function entry before VIRTIO_SND_R_PCM_STOP or virtsnd_pcm_pause_release_flush() runs, so any failure in virtsnd_pcm_pause_restart_pause() also sees vss->xfer_enabled == false and never schedules virtsnd_pcm_period_elapsed(). [ ... ] > @@ -599,8 +614,6 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream > *substream) > return -EBADFD; > } > > - vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream); > - vss->hw_ptr = 0; > } else { > struct snd_pcm_runtime *runtime = substream->runtime; > unsigned int buffer_bytes = snd_pcm_lib_buffer_bytes(substream); > @@ -616,31 +629,30 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream > *substream) > } > } > > - /* ALSA prepare resets its pointers even after a suspended stream. */ > - if (vss->capture_staging) > + scoped_guard(spinlock_irqsave, &vss->lock) { > + vss->prepared = false; > + vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream); > vss->hw_ptr = 0; > - > - if (!pause_restart) > - vss->xfer_xrun = false; > - vss->suspended = false; > - vss->msg_count = 0; > - virtsnd_pcm_msg_reset(vss); > - if (vss->capture_staging) { > - scoped_guard(spinlock_irqsave, &vss->lock) { > + if (!pause_restart) > + vss->xfer_xrun = false; > + vss->suspended = false; > + vss->msg_count = 0; > + virtsnd_pcm_msg_reset(vss); > + memset(&vss->pcm_indirect, 0, sizeof(vss->pcm_indirect)); > + vss->pcm_indirect.sw_buffer_size = vss->buffer_bytes; > + vss->pcm_indirect.hw_buffer_size = vss->buffer_bytes; > + if (vss->capture_staging) { > vss->capture_boundary = substream->runtime->boundary; > rc = virtsnd_pcm_capture_refresh_mode_locked(vss, > - > &free_running); > + &free_running); > + if (rc) > + return rc; > } > - if (rc) > - return rc; > } > > - memset(&vss->pcm_indirect, 0, sizeof(vss->pcm_indirect)); > - vss->pcm_indirect.sw_buffer_size = > - vss->pcm_indirect.hw_buffer_size = > - snd_pcm_lib_buffer_bytes(substream); > - > rc = virtsnd_pcm_dev_prepare(vss); > + scoped_guard(spinlock_irqsave, &vss->lock) > + vss->prepared = !rc; > if (pause_restart) { > if (rc) { > virtsnd_pcm_pause_restart_fail(vss, false); > @@ -759,13 +771,23 @@ 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_RELEASE_SENT || > - state == VIRTSND_PCM_PAUSE_RESTART_RELEASED) > - return 0; > + if (state == VIRTSND_PCM_PAUSE_RESTART_RELEASED) > + 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; > + } > + return rc; > + } [Severity: High] Won't calling virtsnd_pcm_pause_restart_sync_stop() from inside virtsnd_pcm_pause_restart_pause() deadlock on the PCM stream lock when vss->elapsed_period work is pending or running? Because vpcm->pcm->nonatomic is true, the ALSA PCM stream lock is a mutex (substream->self_group.mutex), and ALSA holds it across substream->ops->trigger(): CPU 0 CPU 1 (workqueue) snd_pcm_pause_lock_irq() pcm_stream_lock_irq() snd_pcm_action_single() snd_pcm_do_pause() virtsnd_pcm_trigger(PAUSE_PUSH) virtsnd_pcm_pause_restart_pause() virtsnd_pcm_pause_restart_sync_stop() cancel_work_sync(&vss->elapsed_period) virtsnd_pcm_period_elapsed() guard(pcm_stream_lock_irqsave)(substream) virtsnd_pcm_period_elapsed() blocks waiting for pcm_stream_lock while virtsnd_pcm_pause_restart_sync_stop() waits for virtsnd_pcm_period_elapsed() to finish. In addition, VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT is reached when an earlier virtsnd_pcm_pause_release_flush() timed out, which called virtsnd_pcm_pause_restart_failed() and set vss->xfer_xrun = true while ALSA left runtime->state in SNDRV_PCM_STATE_RUNNING. When a subsequent SNDRV_PCM_TRIGGER_PAUSE_PUSH retry drains the queue and returns -EPIPE because vss->xfer_xrun is true, snd_pcm_action_single() runs snd_pcm_undo_pause() and skips snd_pcm_post_pause(), keeping runtime->state in SNDRV_PCM_STATE_RUNNING where snd_pcm_prepare() is rejected with -EBUSY. > if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED || > virtsnd_pcm_pause_restart_transitioning(state)) { > virtsnd_pcm_pause_restart_fail(vss, true); > @@ -813,10 +835,9 @@ static int virtsnd_pcm_pause_restart_stop(struct > virtio_pcm_substream *vss, > enum virtio_pcm_pause_restart_state state; > int rc; > > - if (suspend) > - vss->suspended = true; > - > scoped_guard(spinlock_irqsave, &vss->lock) { > + if (suspend) > + vss->suspended = true; > vss->xfer_enabled = false; > vss->stopped = true; > state = vss->pause_restart_state; > @@ -887,7 +908,7 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream > *substream, int command) > > scoped_guard(spinlock_irqsave, &queue->lock) { > guard(spinlock)(&vss->lock); > - if (vss->stopped) { > + if (vss->stopped || !vss->prepared) { > substream->runtime->stop_operating = true; > return -EBADFD; > } > @@ -920,17 +941,18 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream > *substream, int command) > msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_START, > GFP_KERNEL); > if (!msg) { > - virtsnd_pcm_start_failed(vss, false); > + virtsnd_pcm_start_failed(vss); > return -ENOMEM; > } > > rc = virtsnd_ctl_msg_send_sync(snd, msg); > if (rc) > - virtsnd_pcm_start_failed(vss, false); > + virtsnd_pcm_start_failed(vss); > > return rc; > case SNDRV_PCM_TRIGGER_SUSPEND: > - vss->suspended = true; > + scoped_guard(spinlock_irqsave, &vss->lock) > + vss->suspended = true; > fallthrough; [Severity: High] Are vss->prepared, vss->stopped, and vss->suspended kept consistent across early prepare failures, sync_stop releases, and unprepared stop triggers? 1. In virtsnd_pcm_prepare(), vss->prepared = false is only set after the early return checks for virtsnd_pcm_msg_pending_num(vss) and virtsnd_pcm_dev_set_params(). If a stream is already in SNDRV_PCM_STATE_PREPARED (with vss->prepared == true) and a re-prepare fails in virtsnd_pcm_dev_set_params() or virtsnd_pcm_msg_pending_num(), snd_pcm_do_prepare() leaves runtime->state as SNDRV_PCM_STATE_PREPARED and virtsnd_pcm_prepare() returns with vss->prepared still true, allowing subsequent START or ACK calls to queue transfers after a failed PREPARE. 2. In virtsnd_pcm_sync_stop() and virtsnd_pcm_pause_restart_sync_stop(), sending VIRTIO_SND_R_PCM_RELEASE clears vss->stopped = false without setting vss->prepared = false. After SNDRV_PCM_IOCTL_DROP releases the stream on the device and moves runtime->state to SNDRV_PCM_STATE_SETUP, vss->prepared remains true and vss->stopped is false, so a subsequent SNDRV_PCM_IOCTL_SYNC_PTR (snd_pcm_sync_ptr() -> pcm_lib_apply_appl_ptr() -> substream->ops->ack()) passes the !vss->prepared and vss->stopped checks in virtsnd_pcm_pb_ack() and virtsnd_pcm_cp_ack() and queues descriptors on a released stream. 3. Conversely, when !vss->prepared (such as after virtsnd_reset_vqs() sets vss->prepared = false and vss->stopped = false), if SNDRV_PCM_TRIGGER_START returns -EBADFD, snd_pcm_action_single() calls snd_pcm_undo_start() -> virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_STOP). Because the SNDRV_PCM_TRIGGER_STOP case does not check vss->prepared, it sets vss->stopped = true and sends VIRTIO_SND_R_PCM_STOP to an unprepared stream, and the next virtsnd_pcm_sync_stop() sends VIRTIO_SND_R_PCM_RELEASE to the unconfigured stream, which fails and leaves vss->stopped latched to true. 4. Also, vss->suspended = false in virtsnd_pcm_open() and the if (!vss->suspended) check in virtsnd_pcm_prepare() still access vss->suspended without holding vss->lock. [ ... ] > @@ -1352,7 +1376,9 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream > *substream) > > if (vss->capture_staging) { > rc = virtsnd_pcm_capture_staged_ack(vss, runtime); > - if (rc == -EPIPE) { > + if (rc == -EPIPE || > + (!rc && runtime->state == SNDRV_PCM_STATE_PAUSED && > + snd_pcm_capture_avail(runtime) >= > runtime->stop_threshold)) { > guard(spinlock_irqsave)(&vss->lock); > virtsnd_pcm_capture_arm_paused_stop(vss); > } [Severity: High] Can arming virtsnd_pcm_capture_arm_paused_stop() inside virtsnd_pcm_cp_ack() when rc == 0 leave vss->stopped = true while the ALSA substream remains in SNDRV_PCM_STATE_PAUSED? In __snd_pcm_lib_xfer(), a single read that wraps around the ring buffer boundary is split across two loop iterations. pcm_lib_apply_appl_ptr() -> substream->ops->ack() is called after each contiguous chunk, whereas snd_pcm_update_state() is only called once after the entire transfer loop finishes. If snd_pcm_capture_avail(runtime) >= runtime->stop_threshold after the first chunk at the wrap boundary, but avail drops below runtime->stop_threshold after the second chunk completes the read, virtsnd_pcm_cp_ack() calls virtsnd_pcm_capture_arm_paused_stop(vss) on the first chunk (setting vss->stopped = true and vss->xfer_enabled = false), while snd_pcm_update_state() sees avail < runtime->stop_threshold and keeps runtime->state in SNDRV_PCM_STATE_PAUSED. Calling snd_pcm_forward(), snd_pcm_rewind(), or snd_pcm_sync_ptr() while paused also invokes pcm_lib_apply_appl_ptr() -> substream->ops->ack() without calling snd_pcm_update_state(). When userspace subsequently unpauses the stream with SNDRV_PCM_TRIGGER_PAUSE_RELEASE in native pause mode, virtsnd_pcm_trigger() sees vss->stopped == true and fails with -EBADFD. > return rc; > } > > scoped_guard(spinlock_irqsave, &queue->lock) { > guard(spinlock)(&vss->lock); > + if (!vss->prepared) > + return -EBADFD; > transfer.indirect = vss->pcm_indirect; > if (vss->stopped && runtime->state != SNDRV_PCM_STATE_DRAINING) > { > rc = -EPIPE; [Severity: High] What happens in the non-staged capture path (!vss->capture_staging) when a paused read crosses runtime->stop_threshold and snd_pcm_update_state() moves the substream to SNDRV_PCM_STATE_XRUN? Because virtsnd_pcm_capture_arm_paused_stop() is only called in the vss->capture_staging branch of virtsnd_pcm_cp_ack(), a paused non-staged capture stream that enters SNDRV_PCM_STATE_XRUN from snd_pcm_update_state() after a partial read (or when virtsnd_pcm_cp_ack() returns -EPIPE) never sets vss->stopped = true or runtime->stop_operating = true, since snd_pcm_do_stop() is a no-op when !snd_pcm_running(substream). As a result, the queued RX descriptors remain pending in the virtqueue and subsequent snd_pcm_prepare() calls skip virtsnd_pcm_sync_stop() and fail with -EBADFD at virtsnd_pcm_msg_pending_num(vss). Furthermore, on !CONFIG_MMU, because virtsnd_pcm_msg_complete() now checks vss->prepared && !vss->stopped instead of vss->xfer_enabled: #ifndef CONFIG_MMU if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && vss->prepared && !vss->stopped && written_bytes > sizeof(msg->status)) { ... memcpy(runtime->dma_area + msg->data_offset, vss->owned_dma_area + msg->data_offset, data_bytes); } #endif if userspace calls snd_pcm_drop() followed by snd_pcm_hw_params(), snd_pcm_lib_malloc_pages() can free and reallocate runtime->dma_area before virtsnd_pcm_hw_params() clears vss->prepared, allowing a concurrent RX completion in virtsnd_pcm_msg_complete() to copy into freed runtime->dma_area. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
