Re: [PATCH v5 06/11] ALSA: virtio: wait through signals for PCM teardown

[sashiko-bot@kernel.org]

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 <mynameisjune111@gmail.com>

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/20261027234334.32551-1-mynameisjune111@gmail.com?part=6