Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] ALSA: virtio: virtsnd_pcm_indirect_reset() schedules work during device reset - [High] ALSA: virtio: paused virtsnd_pcm_indirect_reset() ignores unconsumed hw_ptr - [High] ALSA: virtio: virtsnd_pcm_msg_complete() clears XRUN latched by indirect reset - [Medium] ALSA: virtio: snd_pcm_lib_reset() ACK refill failure bypasses indirect reset --
commit 8b5c75eb2645291e7d9290840b04f0f9a62b651f Author: Joonho Lee <[email protected]> ALSA: virtio: rebase ordinary capture accounting on RESET This patch adds virtsnd_pcm_indirect_reset() and selects virtsnd_pcm_capture_indirect_ops for non-staged capture streams to keep indirect capture accounting synchronized with the ALSA core during SNDRV_PCM_IOCTL_RESET. When refilling RX buffers fails after snd_pcm_lib_reset() mutates hw_ptr, it latches XRUN and schedules elapsed_period so the core can finish rebasing appl_ptr before reporting the error. > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index c3d272163907d..ba255803c6577 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -1693,6 +1697,62 @@ static int virtsnd_pcm_sync_stop(struct > snd_pcm_substream *substream) > return virtsnd_pcm_sync_stop_locked(substream); > } > > +/* > + * The PCM lock covers indirect accounting and core completion. Once > lib_reset > + * changes hw_ptr, complete the core rebase even if refilling fails. The > latched > + * XRUN rejects subsequent pointer/ACK operations; the worker publishes XRUN > to > + * paused streams too. RESET success therefore does not imply stream > recovery. > + */ > +static int virtsnd_pcm_indirect_reset(struct snd_pcm_substream *substream) > +{ > + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss); > + struct virtsnd_pcm_transfer transfer = {}; > + int rc; > + > + rc = snd_pcm_lib_reset(substream); > + if (rc) > + return rc; > + if (!snd_pcm_running(substream) && > + runtime->state != SNDRV_PCM_STATE_PAUSED) > + return 0; [Severity: Medium] What happens if RX virtqueue refilling fails inside virtsnd_pcm_cp_ack() while snd_pcm_lib_reset() is updating the pointer on a running stream? When snd_pcm_running(substream) is true, the call chain is: virtsnd_pcm_indirect_reset() -> snd_pcm_lib_reset() -> snd_pcm_update_hw_ptr() -> virtsnd_pcm_cp_pointer() -> snd_pcm_indirect_capture_pointer() -> virtsnd_pcm_cp_ack() In include/sound/pcm-indirect.h:snd_pcm_indirect_capture_pointer(), rec fields are mutated before invoking substream->ops->ack(): rec->hw_io = ptr; rec->hw_ready += bytes; qsize = rec->hw_queue_size ? rec->hw_queue_size : rec->hw_buffer_size; if (rec->hw_ready > qsize) return SNDRV_PCM_POS_XRUN; rec->sw_io += bytes; if (rec->sw_io >= rec->sw_buffer_size) rec->sw_io -= rec->sw_buffer_size; if (substream->ops->ack) { err = substream->ops->ack(substream); if (err == -EPIPE) return SNDRV_PCM_POS_XRUN; } Whenever rec->hw_ready > 0 and rec->sw_ready < rec->sw_buffer_size, virtsnd_pcm_cp_ack() runs snd_pcm_indirect_capture_transfer() and attempts to refill the RX virtqueue before snd_pcm_lib_reset() returns. If virtsnd_pcm_msg_send() fails during that ack callback, snd_pcm_indirect_capture_pointer() returns SNDRV_PCM_POS_XRUN, snd_pcm_update_hw_ptr0() transitions runtime->state to SNDRV_PCM_STATE_XRUN and returns -EPIPE, and snd_pcm_lib_reset() executes: if (snd_pcm_running(substream) && snd_pcm_update_hw_ptr(substream) >= 0) runtime->status->hw_ptr %= runtime->buffer_size; else { runtime->status->hw_ptr = 0; runtime->hw_ptr_wrap = 0; } virtsnd_pcm_indirect_reset() then sees !snd_pcm_running(substream) and runtime->state != SNDRV_PCM_STATE_PAUSED and returns 0 early, so snd_pcm_do_reset() sets runtime->control->appl_ptr = 0 while vss->pcm_indirect retains its partially updated hw_io, hw_ready, and sw_io values. Should virtsnd_pcm_cp_ack() be suppressed during snd_pcm_lib_reset() (as staged capture does with vss->capture_reset_observing) so that refilling only happens once in virtsnd_pcm_indirect_reset() after hw_ptr is rebased? > + > + guard(spinlock_irqsave)(&queue->lock); > + guard(spinlock)(&vss->lock); > + if (vss->resetting || !vss->prepared || vss->stopped) { > + vss->xfer_xrun = true; > + virtsnd_pcm_schedule_period_elapsed(vss); > + return 0; > + } [Severity: Critical] Can scheduling vss->elapsed_period here when vss->resetting is true cause a use-after-free on substream->runtime during substream close? Unlike virtsnd_pcm_reset(), which returns 0 immediately when vss->resetting is true: if (vss->resetting) return 0; virtsnd_pcm_indirect_reset() calls virtsnd_pcm_schedule_period_elapsed(vss) when vss->resetting is true (which can happen after virtsnd_freeze() / virtsnd_reset_vqs() has already set vss->resetting = true and finished its own cancel_work_sync(&vss->elapsed_period)). If userspace then closes the substream while vss->resetting is still true, virtsnd_pcm_sync_stop_locked() returns -ECANCELED before reaching cancel_work_sync(&vss->elapsed_period): scoped_guard(spinlock_irqsave, &vss->lock) { if (vss->resetting) return -ECANCELED; ... } cancel_work_sync(&vss->elapsed_period); In addition, both virtsnd_pcm_hw_free() and virtsnd_pcm_close() only call cancel_work_sync(&vss->elapsed_period) when vss->capture_staging is true: if (vss->capture_staging) { scoped_guard(spinlock_irqsave, &vss->lock) { vss->runtime_active = false; vss->xfer_enabled = false; } cancel_work_sync(&vss->elapsed_period); } When snd_pcm_release_substream() subsequently calls snd_pcm_detach_substream(), runtime is freed without holding pcm_stream_lock, while virtsnd_pcm_period_elapsed() may still be queued or running and dereferencing substream->runtime in sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed(). Also, if this branch is taken when vss->resetting is false and !vss->prepared && !vss->stopped, virtsnd_pcm_transfer_failed(vss) is not called, leaving vss->stopped false so a subsequent virtsnd_pcm_prepare() skips virtsnd_pcm_sync_stop_locked() without canceling the scheduled work. > + > + /* Pausing does not move the device's fixed DMA buffer positions. */ > + if (runtime->state == SNDRV_PCM_STATE_PAUSED) > + runtime->status->hw_ptr = > + bytes_to_frames(runtime, vss->pcm_indirect.sw_io); > + > + transfer.indirect = vss->pcm_indirect; > + transfer.indirect.sw_ready = 0; [Severity: High] When runtime->state == SNDRV_PCM_STATE_PAUSED, does using vss->pcm_indirect.sw_io and vss->pcm_indirect.hw_ready directly miss RX completions that already advanced vss->hw_ptr? For ordinary capture (!vss->capture_staging), RX completions in sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_msg_complete() advance vss->hw_ptr and clear msg->length = 0 under vss->lock without updating vss->pcm_indirect: vss->hw_ptr += data_bytes; } if (vss->hw_ptr >= vss->buffer_bytes) vss->hw_ptr -= vss->buffer_bytes; msg->length = 0; vss->pcm_indirect (hw_io, sw_io, and hw_ready) is only updated from vss->hw_ptr when virtsnd_pcm_cp_pointer() calls snd_pcm_indirect_capture_pointer(). When a stream is paused via snd_pcm_pause(), the core does not call snd_pcm_update_hw_ptr(). Any RX completion whose elapsed_period work runs after runtime->state becomes SNDRV_PCM_STATE_PAUSED exits early in sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed(): if (!snd_pcm_running(substream)) { ... return; } And any RX completion arriving during SNDRV_PCM_TRIGGER_PAUSE_PUSH while virtsnd_pcm_stop_device() waits for VIRTIO_SND_R_PCM_STOP also advances vss->hw_ptr without updating vss->pcm_indirect. When virtsnd_pcm_indirect_reset() then runs in SNDRV_PCM_STATE_PAUSED: 1. snd_pcm_lib_reset() skips snd_pcm_update_hw_ptr() because snd_pcm_running(substream) is false, and sets runtime->status->hw_ptr = 0. 2. virtsnd_pcm_indirect_reset() sets runtime->status->hw_ptr from the stale vss->pcm_indirect.sw_io and runs snd_pcm_indirect_capture_transfer() without folding the unconsumed delta (vss->hw_ptr - vss->pcm_indirect.hw_io) into hw_io, sw_io, and hw_ready. As a result, completed RX messages between hw_io and vss->hw_ptr are not counted in hw_ready and are not re-submitted to the RX virtqueue, hw_ptr/appl_ptr are rebased to the stale sw_io instead of vss->hw_ptr, and after resume the next virtsnd_pcm_cp_pointer() call sees those pre-RESET bytes as newly captured data. (Note also that if the earlier vss->resetting || !vss->prepared || vss->stopped check returns 0 while paused, runtime->status->hw_ptr is left at 0 from snd_pcm_lib_reset() rather than restored from sw_io.) > + /* Consume no application delta while returning discarded RX space. */ > + transfer.indirect.appl_ptr = runtime->control->appl_ptr; > + rc = snd_pcm_indirect_capture_transfer(substream, &transfer.indirect, > + virtsnd_pcm_trans_copy); > + if (!rc) > + rc = transfer.error; > + if (rc) { > + virtsnd_pcm_transfer_failed(vss); > + vss->xfer_xrun = true; > + virtsnd_pcm_schedule_period_elapsed(vss); > + return 0; > + } [Severity: High] Can an in-flight RX completion in virtsnd_pcm_msg_complete() clear vss->xfer_xrun before virtsnd_pcm_period_elapsed() or subsequent pointer checks observe the latched XRUN? When snd_pcm_indirect_capture_transfer() fails here (or on the earlier error path), virtsnd_pcm_transfer_failed(vss) sets vss->xfer_enabled = false and vss->stopped = true, but it does not stop the VirtIO device or flush RX buffers already queued in the virtqueue (including any messages queued and kicked by earlier iterations of virtsnd_pcm_trans_copy() before a later period failed). In sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_msg_complete(), any completing non-staged message unconditionally clears vss->xfer_xrun under vss->lock: msg->length = 0; vss->xfer_xrun = false; vss->msg_count--; If an in-flight RX message completes before the scheduled virtsnd_pcm_period_elapsed() worker runs, vss->xfer_xrun is reset to false. In sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed(): 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); } paused_xrun then evaluates to false, so a paused stream never transitions to SNDRV_PCM_STATE_XRUN. Because RESET rebased appl_ptr = hw_ptr (avail == 0), subsequent reads on the paused stream never invoke pointer() or ack(), leaving nonblocking reads returning -EAGAIN indefinitely and blocking reads hanging instead of returning -EPIPE. Furthermore, because this error return skips updating vss->pcm_indirect, vss->pcm_indirect is left out of sync with the rebased runtime->control->appl_ptr. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
