Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] ALSA: virtio: race clearing vqueue in virtsnd_reset_vqs() before del_vqs() - [High] ALSA: virtio: uncanceled elapsed_period work on sync_stop and close paths - [Medium] ALSA: virtio: incomplete pause failure recovery in virtsnd_pcm_trigger() - [Medium] ALSA: virtio: race in virtsnd_pcm_sync_stop() across prepare generations - [Medium] ALSA: virtio: race between virtsnd_pcm_prepare_locked() and staged ack() --
commit a385aa4275f0e96bd41c0f455c953ca4c07a25b9 Author: Joonho Lee <[email protected]> ALSA: virtio: add staged capture accounting and lifecycle recovery This commit introduces staged capture with request-owned DMA buffers and separate produced, observed, and consumed accounting positions for virtio-snd. It also adds lifecycle serialization and state tracking for control requests, pause-restart recovery, and virtqueue resets. > diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c > index df7091b74a521..07d66225ce04a 100644 > --- a/sound/virtio/virtio_card.c > +++ b/sound/virtio/virtio_card.c > @@ -348,6 +348,22 @@ 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->reset_epoch++; > + vss->resetting = true; > + vss->prepared = false; > + vss->device_running = false; > + vss->release_required = false; > + vss->control_uncertain = false; > + vss->capture_reset_observing = false; > + vss->xfer_enabled = false; > + vss->suspended = true; > + } > virtio_reset_device(vdev); > > /* Deleting a virtqueue does not unmap its outstanding descriptors. */ > @@ -364,9 +380,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); [Severity: High] Could clearing snd->queues[i].vqueue before calling vdev->config->del_vqs(vdev) cause a NULL pointer dereference in virtsnd_pcm_notify_cb()? On transports whose reset callback does not call synchronize_irq() (such as virtio_mmio's vm_reset() or virtio_vdpa's virtio_vdpa_reset()), virtqueue IRQ handlers are not synchronized or freed until vdev->config->del_vqs(vdev) runs. Unlike virtsnd_ctl_notify_cb() and virtsnd_event_notify_cb(), which use their vqueue parameter directly, virtsnd_pcm_notify_cb() in sound/virtio/virtio_pcm_msg.c dereferences queue->vqueue after acquiring queue->lock: guard(spinlock_irqsave)(&queue->lock); do { virtqueue_disable_cb(queue->vqueue); while ((msg = virtqueue_get_buf(queue->vqueue, &written_bytes))) virtsnd_pcm_msg_complete(msg, written_bytes); } while (!virtqueue_enable_cb(queue->vqueue)); If an in-flight TX or RX interrupt on another CPU has entered virtsnd_pcm_tx_notify_cb() or virtsnd_pcm_rx_notify_cb() and is spinning on queue->lock while virtsnd_reset_vqs() sets snd->queues[i].vqueue = NULL, won't virtsnd_pcm_notify_cb() dereference a NULL queue->vqueue as soon as queue->lock is released? > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index de11bf162006e..ee07e5bb3c62f 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -320,48 +706,372 @@ static int virtsnd_pcm_prepare(struct > snd_pcm_substream *substream) [ ... ] > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (epoch != vss->reset_epoch || vss->resetting) > + return -ECANCELED; > + 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); > + 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); > + 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, epoch); > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (epoch != vss->reset_epoch || vss->resetting) > + return -ECANCELED; > + vss->prepared = !rc; > + if (!rc) > + vss->release_required = true; > + } [Severity: Medium] Can virtsnd_pcm_prepare_locked() race with a concurrent virtsnd_pcm_capture_staged_ack() call and latch a false xrun on the newly prepared substream? In the ALSA PCM core, snd_pcm_do_prepare() calls substream->ops->prepare() under runtime->buffer_mutex without holding pcm_stream_lock, and only resets runtime->control->appl_ptr to 0 afterward in snd_pcm_do_reset(). Here in virtsnd_pcm_prepare_locked(), virtsnd_pcm_msg_reset(vss) zeroes capture_copied_total, capture_acked_total, capture_pointer_seen_total, and capture_ready, and memset() zeroes vss->pcm_indirect.appl_ptr while vss->runtime_active remains true. It then sleeps in virtsnd_pcm_dev_prepare() and sets vss->prepared = true before returning to snd_pcm_do_prepare(). Meanwhile, a concurrent snd_pcm_sync_ptr() ioctl (without SNDRV_PCM_SYNC_PTR_HWSYNC or SNDRV_PCM_SYNC_PTR_APPL) only acquires pcm_stream_lock and invokes: pcm_lib_apply_appl_ptr() -> virtsnd_pcm_cp_ack() -> virtsnd_pcm_capture_staged_ack() In virtsnd_pcm_capture_staged_ack(): if (!vss->prepared && runtime->state != SNDRV_PCM_STATE_DRAINING && !(runtime->state == SNDRV_PCM_STATE_PAUSED && virtsnd_pcm_pause_restart_enabled(vss) && vss->pause_restart_state == VIRTSND_PCM_PAUSE_RESTART_RELEASED)) return -EBADFD; Once vss->prepared is set to true (or while virtsnd_pcm_dev_prepare() is sleeping if runtime->state is SNDRV_PCM_STATE_DRAINING or PAUSED), virtsnd_pcm_capture_staged_ack() can compare the unreset pre-prepare runtime->control->appl_ptr against the zeroed vss->pcm_indirect.appl_ptr and vss->capture_copied_total, setting vss->xfer_xrun = true. Even with the follow-up commit "ALSA: virtio: synchronize staged capture accounting during RESET", virtsnd_pcm_reset() returns early when vss->xfer_xrun is true, so wouldn't the next start fail with -EPIPE? [ ... ] > @@ -377,68 +1087,218 @@ static int virtsnd_pcm_prepare(struct > snd_pcm_substream *substream) [ ... ] > switch (command) { > case SNDRV_PCM_TRIGGER_START: > case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > - queue = virtsnd_pcm_queue(vss); > - > - scoped_guard(spinlock_irqsave, &queue->lock) { > - guard(spinlock)(&vss->lock); > - if (vss->stopped) { > - substream->runtime->stop_operating = true; > - return -EBADFD; > - } > - if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) > - rc = virtsnd_pcm_msg_send(vss, 0, > vss->buffer_bytes); > - if (rc) { > - vss->stopped = true; > - substream->runtime->stop_operating = true; > - return rc; > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (vss->device_running && !vss->control_uncertain && > + !vss->resetting && !vss->stopped) { > + vss->xfer_enabled = true; > + return 0; > } > - vss->xfer_enabled = true; > + } > + rc = virtsnd_pcm_start_queue(vss, epoch); > + if (rc) { > + virtsnd_pcm_start_failed(vss, epoch); > + return rc; > } > > msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_START, > - GFP_KERNEL); > + GFP_KERNEL, epoch); > if (!msg) { > - virtsnd_pcm_start_failed(vss); > + virtsnd_pcm_start_failed(vss, epoch); > return -ENOMEM; > } > > - rc = virtsnd_ctl_msg_send_sync(snd, msg); > - if (rc) > - virtsnd_pcm_start_failed(vss); > + rc = virtsnd_pcm_send_sync(vss, msg); > + if (rc && rc != -ECANCELED) > + virtsnd_pcm_start_failed(vss, epoch); > > + if (!rc) { > + guard(spinlock_irqsave)(&vss->lock); > + if (vss->resetting || vss->reset_epoch != epoch) > + return -ECANCELED; > + vss->device_running = true; > + } > + if (!rc && vss->capture_staging) > + virtsnd_pcm_schedule_period_elapsed(vss); > return rc; > case SNDRV_PCM_TRIGGER_SUSPEND: > - vss->suspended = true; > + scoped_guard(spinlock_irqsave, &vss->lock) > + vss->suspended = true; > fallthrough; > case SNDRV_PCM_TRIGGER_STOP: > case SNDRV_PCM_TRIGGER_PAUSE_PUSH: > scoped_guard(spinlock_irqsave, &vss->lock) { > + if (!vss->prepared) > + return 0; > if (command != SNDRV_PCM_TRIGGER_PAUSE_PUSH) > vss->stopped = true; > vss->xfer_enabled = false; > + if (!vss->device_running) > + return 0; > } > > + rc = virtsnd_pcm_stop_device(vss, epoch); > + if (rc && command == SNDRV_PCM_TRIGGER_PAUSE_PUSH) { > + guard(spinlock_irqsave)(&vss->lock); > + if (vss->reset_epoch == epoch && !vss->resetting && > + !vss->control_uncertain && vss->device_running && > + !vss->xfer_xrun) > + vss->xfer_enabled = true; > + } > + return rc; [Severity: Medium] What happens to staged capture RX descriptors if virtsnd_pcm_stop_device() fails during SNDRV_PCM_TRIGGER_PAUSE_PUSH when pause-restart mode is disabled? Before calling virtsnd_pcm_stop_device(), SNDRV_PCM_TRIGGER_PAUSE_PUSH clears vss->xfer_enabled = false and drops vss->lock while the device is still running. Any staged capture RX buffers that complete during this window see vss->xfer_enabled == false in virtsnd_pcm_capture_complete(), so their audio is discarded, msg->length is cleared to 0, vss->msg_count is decremented, and virtsnd_pcm_capture_enqueue() is skipped. If virtsnd_pcm_stop_device() fails with a definite error (so vss->control_uncertain is false and vss->device_running remains true), both the recovery path above and the SNDRV_PCM_TRIGGER_PAUSE_RELEASE fast path invoked by snd_pcm_undo_pause() set vss->xfer_enabled = true without calling virtsnd_pcm_capture_send() to refill the drained RX buffers. If all queued RX descriptors completed while vss->xfer_enabled was false, won't vss->msg_count remain 0 and stall capture indefinitely? Also, the SNDRV_PCM_TRIGGER_PAUSE_RELEASE fast path when vss->device_running is true does not check !vss->xfer_xrun before setting vss->xfer_enabled = true, and if virtsnd_pcm_stop_device() fails with vss->control_uncertain == true on a linked substream where snd_pcm_action_group() does not call snd_pcm_undo_pause() on the failing stream, the substream is left in SNDRV_PCM_STATE_RUNNING with vss->xfer_enabled == false, vss->stopped == false, and runtime->stop_operating == false. [ ... ] > @@ -455,31 +1315,49 @@ static int virtsnd_pcm_sync_stop(struct > snd_pcm_substream *substream) > unsigned int js = msecs_to_jiffies(virtsnd_msg_timeout_ms); > bool need_release; > int rc; > + u64 epoch; > + > + if (virtsnd_pcm_pause_restart_enabled(vss)) > + return virtsnd_pcm_pause_restart_sync_stop(substream); > > scoped_guard(spinlock_irqsave, &vss->lock) { > + if (vss->resetting) > + return -ECANCELED; > + epoch = vss->reset_epoch; > vss->xfer_enabled = false; > need_release = vss->release_required; > if (need_release) > vss->stopped = true; > } > cancel_work_sync(&vss->elapsed_period); [Severity: High] Can vss->elapsed_period remain pending across sync_stop, hw_free, and close, racing with snd_pcm_detach_substream() freeing substream->runtime? There appear to be two ways vss->elapsed_period is left uncanceled when a substream is closed: 1. In virtsnd_reset_vqs(), vss->resetting = true is set in the first loop before virtio_reset_device() and vdev->config->del_vqs() run, while cancel_work_sync(&vss->elapsed_period) only runs in the final loop (and if virtsnd_restore() fails in virtsnd_find_vqs(), vss->resetting stays true). When vss->resetting is true, virtsnd_pcm_sync_stop_locked() and virtsnd_pcm_pause_restart_sync_stop() return -ECANCELED before reaching cancel_work_sync(&vss->elapsed_period) (and virtsnd_pcm_sync_stop() also returns 0 early when !vss->stopped without canceling work). For non-staged substreams (!vss->capture_staging), neither virtsnd_pcm_hw_free() nor virtsnd_pcm_close() calls cancel_work_sync(&vss->elapsed_period): sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_close() { if (vss->capture_staging) { scoped_guard(spinlock_irqsave, &vss->lock) { vss->runtime_active = false; vss->xfer_enabled = false; } cancel_work_sync(&vss->elapsed_period); } } 2. In virtsnd_pcm_pause_restart_sync_stop(), cancel_work_sync() runs near the beginning of the function, but the subsequent error paths call virtsnd_pcm_pause_restart_fail(), which re-schedules vss->elapsed_period when vss->runtime_active is true. When snd_pcm_release_substream() then calls snd_pcm_detach_substream(), which clears substream->runtime = NULL and calls kfree(runtime) without holding pcm_stream_lock, can virtsnd_pcm_period_elapsed() concurrently access the freed runtime? [ ... ] > @@ -637,6 +1675,19 @@ 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); > + scoped_guard(spinlock_irqsave, &vss->lock) { > + /* A newer prepare may have consumed this stop generation. */ > + if (!vss->stopped) > + return 0; > + } > + return virtsnd_pcm_sync_stop_locked(substream); > +} [Severity: Medium] Is checking !vss->stopped under vss->lifecycle_lock sufficient to detect that a newer prepare already consumed an earlier stop generation? In sound/core/pcm_native.c, snd_pcm_drop() drops pcm_stream_lock before calling snd_pcm_sync_stop() without holding runtime->buffer_mutex. If thread A in snd_pcm_sync_stop() clears runtime->stop_operating = false and is preempted before acquiring vss->lifecycle_lock in virtsnd_pcm_sync_stop(), thread B can: 1. Call snd_pcm_prepare(), which acquires vss->lifecycle_lock, consumes thread A's stop via virtsnd_pcm_sync_stop_locked(), and prepares a new stream generation. 2. Start the stream via virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_START). 3. Issue SNDRV_PCM_TRIGGER_PAUSE_PUSH or SNDRV_PCM_TRIGGER_STOP, which run under pcm_stream_lock without vss->lifecycle_lock and set vss->stopped = true. When thread A then acquires vss->lifecycle_lock, vss->stopped is true again, so thread A proceeds into virtsnd_pcm_sync_stop_locked() against the new generation. In pause-restart mode, if thread B is mid-transition inside virtsnd_pcm_pause_restart_pause() or virtsnd_pcm_pause_restart_stop(), thread A will see virtsnd_pcm_pause_restart_transitioning(state) and force VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED; or if thread B has completed PAUSE_PUSH (reaching VIRTSND_PCM_PAUSE_RESTART_RELEASED with vss->stopped == true), thread A will clear vss->stopped = false in virtsnd_pcm_pause_restart_sync_stop(), causing a subsequent PAUSE_RELEASE in virtsnd_pcm_pause_restart_start() to fail with -EBADFD. In normal mode, if thread B is inside virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_STOP), thread A can issue overlapping STOP and RELEASE control requests concurrently. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
