Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] ALSA: virtio: interruptible I/O flush wait in virtsnd_pcm_sync_stop() - [Medium] ALSA: virtio: interrupted PCM_START in virtsnd_ctl_msg_send() breaks teardown --
commit cb2fb94625971d45e77abbb9e6f9bcb8abb321ee Author: Joonho Lee <[email protected]> ALSA: virtio: wait through signals for PCM teardown This commit changes virtsnd_ctl_msg_send() to use an uninterruptible timeout wait for VIRTIO_SND_R_PCM_STOP and VIRTIO_SND_R_PCM_RELEASE control requests so that pending signals do not abort PCM teardown before completion. > diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c > index 2e0ff15e729da..e50e6c7f3b8d1 100644 > --- a/sound/virtio/virtio_ctl_msg.c > +++ b/sound/virtio/virtio_ctl_msg.c > @@ -314,7 +314,20 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct > virtio_snd_msg *msg, > */ > virtsnd_ctl_msg_unref(msg); > } else if (!nowait) { > - rc = wait_for_completion_interruptible_timeout(&msg->notify, > js); > + /* > + * Teardown must observe completion even with a pending signal. > + * Otherwise a subsequent open can retry RELEASE while the > device > + * is still processing the previous teardown request. > + */ > + switch (le32_to_cpu(request->code)) { > + case VIRTIO_SND_R_PCM_STOP: > + case VIRTIO_SND_R_PCM_RELEASE: > + rc = wait_for_completion_timeout(&msg->notify, js); > + break; [Severity: High] When a signal is pending during PCM teardown, virtsnd_ctl_msg_send() now waits uninterruptibly for VIRTIO_SND_R_PCM_RELEASE to complete on the control virtqueue and returns 0 to virtsnd_pcm_sync_stop() with the signal still pending on current. However, right after virtsnd_ctl_msg_send_sync() returns in sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_sync_stop(), the wait for pending TX/RX I/O buffers to drain is still interruptible: rc = virtsnd_ctl_msg_send_sync(snd, msg); if (rc) { substream->runtime->stop_operating = true; return rc; } rc = wait_event_interruptible_timeout(vss->msg_empty, !virtsnd_pcm_msg_pending_num(vss), js); if (rc <= 0) { dev_warn(&snd->vdev->dev, "SID %u: failed to flush I/O queue\n", vss->sid); substream->runtime->stop_operating = true; return !rc ? -ETIMEDOUT : rc; } scoped_guard(spinlock_irqsave, &vss->lock) vss->stopped = false; Because TX/RX buffer completions arrive asynchronously on separate virtqueues (VIRTIO_SND_VQ_TX / VIRTIO_SND_VQ_RX), vss->msg_count can still be non-zero when the control message completes. With the signal still pending on current, wait_event_interruptible_timeout() immediately returns -ERESTARTSYS, leaving vss->stopped = true and setting substream->runtime->stop_operating = true. In snd_pcm_release_substream(), do_hw_free() runs after snd_pcm_drop(), sees runtime->stop_operating == true, and calls virtsnd_pcm_sync_stop() a second time (or a subsequent virtsnd_pcm_open() preserves vss->stopped via vss->stopped |= !!vss->msg_count and calls virtsnd_pcm_sync_stop()). Both paths see need_release = vss->stopped || vss->msg_count as true and send a duplicate VIRTIO_SND_R_PCM_RELEASE while the first RELEASE is still flushing I/O or after the stream is already released. In addition, virtsnd_pcm_hw_free() skips virtsnd_pcm_msg_free(vss) while virtsnd_pcm_msg_pending_num(vss) is non-zero, leaving vss->msgs and retained vss->dma_pages (or vss->owned_dma_area on !CONFIG_MMU) allocated after close. Should the vss->msg_empty wait in virtsnd_pcm_sync_stop() also use wait_event_timeout() so that the entire RELEASE teardown completes through pending signals? > + default: > + rc = > wait_for_completion_interruptible_timeout(&msg->notify, js); > + break; > + } [Severity: Medium] What happens if a signal interrupts VIRTIO_SND_R_PCM_START in this default branch after the request has already been queued to the control virtqueue? In sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trigger(), both SNDRV_PCM_TRIGGER_START and SNDRV_PCM_TRIGGER_PAUSE_RELEASE send VIRTIO_SND_R_PCM_START synchronously: rc = virtsnd_ctl_msg_send_sync(snd, msg); if (rc) virtsnd_pcm_start_failed(vss); return rc; Notice that snd_pcm_drop() also triggers SNDRV_PCM_TRIGGER_PAUSE_RELEASE via snd_pcm_pause(substream, false) during teardown when runtime->state is SNDRV_PCM_STATE_PAUSED. If a signal is pending or arrives while waiting for VIRTIO_SND_R_PCM_START, virtsnd_ctl_msg_send_sync() returns -ERESTARTSYS while the device still processes the queued VIRTIO_SND_R_PCM_START and transitions the stream to STARTED. Meanwhile, virtsnd_pcm_start_failed() sets vss->stopped = true and substream->runtime->stop_operating = true, and ALSA keeps runtime->state in SNDRV_PCM_STATE_PREPARED or SNDRV_PCM_STATE_PAUSED: 1. For linked streams in snd_pcm_action_group(), ops->undo_action is only called on streams iterated before the failing stream, so SNDRV_PCM_TRIGGER_STOP is not sent for the interrupted stream. Subsequent snd_pcm_stop() sees snd_pcm_running(substream) == false and skips SNDRV_PCM_TRIGGER_STOP, after which virtsnd_pcm_sync_stop() sees vss->stopped == true and sends VIRTIO_SND_R_PCM_RELEASE while the device stream is still STARTED. 2. For unlinked streams where snd_pcm_undo_start() or snd_pcm_undo_pause() sends VIRTIO_SND_R_PCM_STOP, vss->stopped remains true while runtime->state stays PREPARED or PAUSED without snd_pcm_sync_stop() running, so a syscall restart or retry of START/PAUSE_RELEASE fails with -EBADFD in virtsnd_pcm_trigger(). In addition, when pause-restart mode is active (from commit "ALSA: virtio: stage VZ capture and restart paused streams"), an interrupted PAUSE_RELEASE during snd_pcm_drop() transitions vss->pause_restart_state to VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED, skipping STOP and RELEASE and wedging the substream. Should VIRTIO_SND_R_PCM_START also wait uninterruptibly once queued so the driver and device stream states stay synchronized? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
