Re: [PATCH v10 09/14] ALSA: virtio: add staged capture accounting and lifecycle recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ALSA: virtio: duplicate RX buffer enqueue on non-staged pause release
- [Medium] ALSA: virtio: detached virtqueue race in virtsnd_reset_vqs() and notify_cb
- [Medium] ALSA: virtio: incomplete pause rejection recovery in virtsnd_pcm_trigger()
- [Medium] ALSA: virtio: missing virtsnd_pcm_start_failed() on running trigger error
--

commit 4e7eab1d20a52ea1742792d7fd3cc04245ceba41
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: add staged capture accounting and lifecycle recovery

This commit adds staged capture accounting using request-owned DMA buffers
and introduces lifecycle recovery and epoch tracking across PCM operations
and device resets.

Link: https://sashiko.dev/#/patchset/20261029085032.70689-1-mynameisjune111%40gmail.com?part=9

> 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;
> +	}

[Severity: Medium]
As reported by Sashiko on v9 [1], can an IRQ callback still observe a
non-NULL queue->vqueue after its buffers have been detached in
virtsnd_reset_vqs()?

In virtsnd_reset_vqs(), the first loop over VIRTIO_SND_VQ_MAX detaches
unused buffers under queue->lock and then drops the lock:

	for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i) {
		struct virtio_snd_queue *queue = &snd->queues[i];
		unsigned long flags;
		void *msg;

		spin_lock_irqsave(&queue->lock, flags);
		if (queue->vqueue)
			while ((msg = virtqueue_detach_unused_buf(queue->vqueue)))
				if (i == VIRTIO_SND_VQ_TX || i == VIRTIO_SND_VQ_RX)
					virtsnd_pcm_msg_cancel(msg);
		spin_unlock_irqrestore(&queue->lock, flags);
	}

Only in the separate loop above does virtsnd_reset_vqs() re-acquire
snd->queues[i].lock to set snd->queues[i].vqueue = NULL.

Because virtqueue_detach_unused_buf_split() clears
vq->split.desc_state[i].data = NULL without advancing vq->last_used_idx,
if virtsnd_pcm_notify_cb() acquires queue->lock between those two loops,
it will see queue->vqueue != NULL and call virtqueue_get_buf() on a ring
whose descriptor state was already cleared, triggering BAD_RING() in
virtqueue_get_buf_ctx_split() if the device wrote a used entry just before
virtio_reset_device().

In addition, virtsnd_ctl_notify_cb() in sound/virtio/virtio_ctl_msg.c and
virtsnd_event_notify_cb() in sound/virtio/virtio_card.c do not check
queue->vqueue under queue->lock at all before calling virtqueue_get_buf(),
virtqueue_enable_cb(), or virtsnd_event_send().

Should queue->vqueue = NULL be set inside the same queue->lock critical
section that calls virtqueue_detach_unused_buf(), and should
virtsnd_ctl_notify_cb() and virtsnd_event_notify_cb() also check
if (!queue->vqueue) under queue->lock?

[ ... ]

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index de11bf162006e..8a6499d0cfea7 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> +static int virtsnd_pcm_start_queue(struct virtio_pcm_substream *vss, u64 epoch)
> +{
> +	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +	int rc = 0;
> +
> +	guard(spinlock_irqsave)(&queue->lock);
> +	guard(spinlock)(&vss->lock);
> +	if (vss->resetting || vss->reset_epoch != epoch)
> +		return -ECANCELED;
> +	if (vss->stopped || !vss->prepared || vss->xfer_xrun)
> +		return vss->xfer_xrun ? -EPIPE : -EBADFD;
> +	if (vss->control_uncertain)
> +		return -EIO;
> +	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) {
> +		if (vss->capture_staging)
> +			rc = vss->runtime_active ? virtsnd_pcm_capture_send(vss) : -EBADFD;
> +		else if (!vss->device_running)
> +			rc = virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes);
> +	}

[Severity: High]
On a non-staged capture stream, won't resuming from pause via
SNDRV_PCM_TRIGGER_PAUSE_RELEASE re-submit the entire capture buffer while
descriptors from before the pause are still queued or hold unread data?

When SNDRV_PCM_TRIGGER_START initially starts a non-staged capture stream,
virtsnd_pcm_start_queue() calls virtsnd_pcm_msg_send(vss, 0,
vss->buffer_bytes) to queue all vss->nmsgs descriptors with
msg->length = period_bytes.

When SNDRV_PCM_TRIGGER_PAUSE_PUSH pauses the stream,
virtsnd_pcm_stop_device() sends VIRTIO_SND_R_PCM_STOP and sets
vss->device_running = false without releasing the pending RX descriptors.

When SNDRV_PCM_TRIGGER_PAUSE_RELEASE later resumes the stream,
virtsnd_pcm_start_queue() sees !vss->device_running == true and calls
virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes) again across the whole
buffer:

	msg->length += n;
	if (msg->length == period_bytes) {

For descriptors still pending in the virtqueue, msg->length is incremented
to 2 * period_bytes, corrupting vss->hw_ptr accounting on error status and
preventing subsequent ACK re-submissions. For descriptors that already
completed before or during pause and are waiting for userspace to read and
ACK them, virtsnd_pcm_msg_send() re-queues them into the RX virtqueue,
allowing the device to overwrite unread audio samples via DMA.

> +	if (!rc)
> +		vss->xfer_enabled = true;
> +	return rc;
> +}
> +
>  /**
>   * virtsnd_pcm_trigger() -Start/stop/pause/resume the PCM substream.
>   * @substream: Kernel ALSA substream.
> @@ -377,68 +1087,219 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
>  static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
>  {
>  	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> -	struct virtio_snd *snd = vss->snd;
> -	struct virtio_snd_queue *queue;
>  	struct virtio_snd_msg *msg;
>  	int rc = 0;
> +	bool running;
> +	u64 epoch;
> +
> +	scoped_guard(spinlock_irqsave, &vss->lock)
> +		epoch = vss->reset_epoch;
> +
> +	if (virtsnd_pcm_pause_restart_enabled(vss)) {
> +		switch (command) {
> +		case SNDRV_PCM_TRIGGER_START:
> +			return virtsnd_pcm_pause_restart_start(vss, false);
> +		case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +			return virtsnd_pcm_pause_restart_start(vss, true);
> +		case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +			return virtsnd_pcm_pause_restart_pause(vss);
> +		case SNDRV_PCM_TRIGGER_STOP:
> +			return virtsnd_pcm_pause_restart_stop(vss, false);
> +		case SNDRV_PCM_TRIGGER_SUSPEND:
> +			return virtsnd_pcm_pause_restart_stop(vss, true);
> +		default:
> +			return -EINVAL;
> +		}
> +	}
>  
>  	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;
> -			}
> -			vss->xfer_enabled = true;
> +		scoped_guard(spinlock_irqsave, &vss->lock)
> +			running = vss->device_running;
> +		/* Refill RX descriptors returned during a rejected native pause. */
> +		if (running)
> +			return virtsnd_pcm_start_queue(vss, epoch);

[Severity: Medium]
As reported by Sashiko on v9 [1], what happens when running is true here
and virtsnd_pcm_start_queue(vss, epoch) fails, or when it succeeds on a
staged capture stream?

When SNDRV_PCM_TRIGGER_PAUSE_PUSH is rejected by the device in
virtsnd_pcm_stop_device(), vss->device_running remains true. When
SNDRV_PCM_TRIGGER_PAUSE_RELEASE subsequently runs (either from
snd_pcm_undo_pause(), which ignores the trigger return value, or a later
resume on a linked substream), virtsnd_pcm_trigger() sees running == true
and returns virtsnd_pcm_start_queue(vss, epoch) directly.

If virtsnd_pcm_start_queue() fails (for example, if
virtsnd_pcm_capture_refresh_mode_locked() returns -EPIPE after a
stop_threshold update, or virtqueue_add_sgs() fails while refilling
descriptors completed after PAUSE_PUSH), this early return bypasses
virtsnd_pcm_start_failed(vss, epoch), leaving vss->xfer_enabled,
vss->stopped, substream->runtime->stop_operating, and vss->xfer_xrun
unlatched and failing to schedule elapsed_period work to wake blocked
staged-capture readers.

Conversely, if virtsnd_pcm_start_queue() succeeds for staged capture on
this running == true path, returning early skips the
virtsnd_pcm_schedule_period_elapsed(vss) call at the end of the
SNDRV_PCM_TRIGGER_START / SNDRV_PCM_TRIGGER_PAUSE_RELEASE case.

> +		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 &&
> +		    virtsnd_pcm_start_queue(vss, epoch)) {
> +			guard(spinlock_irqsave)(&vss->lock);
> +			if (vss->reset_epoch == epoch && !vss->resetting) {
> +				vss->xfer_xrun = true;
> +				vss->stopped = true;
> +				substream->runtime->stop_operating = true;
> +				virtsnd_pcm_schedule_period_elapsed(vss);
> +			}
> +		}
> +		return rc;

[Severity: Medium]
As reported by Sashiko on v9 [1], can a non-staged capture stream stall if
VIRTIO_SND_R_PCM_STOP is rejected during SNDRV_PCM_TRIGGER_PAUSE_PUSH?

Before calling virtsnd_pcm_stop_device(vss, epoch),
SNDRV_PCM_TRIGGER_PAUSE_PUSH sets vss->xfer_enabled = false. While
VIRTIO_SND_R_PCM_STOP is in flight, the device is still running and can
complete queued RX buffers. In virtsnd_pcm_msg_complete(), completions for
non-staged capture advance vss->hw_ptr, clear msg->length = 0, and
decrement vss->msg_count, but skip virtsnd_pcm_schedule_period_elapsed(vss)
because vss->xfer_enabled is false.

When virtsnd_pcm_stop_device() fails and leaves vss->device_running = true,
both virtsnd_pcm_start_queue(vss, epoch) here and the subsequent
SNDRV_PCM_TRIGGER_PAUSE_RELEASE recovery path check:

	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) {
		if (vss->capture_staging)
			rc = vss->runtime_active ? virtsnd_pcm_capture_send(vss) : -EBADFD;
		else if (!vss->device_running)
			rc = virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes);
	}
	if (!rc)
		vss->xfer_enabled = true;

Because vss->device_running is still true, virtsnd_pcm_start_queue() only
sets vss->xfer_enabled = true without scheduling elapsed_period work or
refilling descriptors. If all queued RX descriptors completed while
vss->xfer_enabled was false, vss->msg_count is 0 and no period work is
scheduled, so ALSA never runs pointer/ack to re-enqueue RX buffers and the
capture stream stalls indefinitely.

[1] https://lore.kernel.org/r/20261029085032.70689-10-mynameisjune111@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030052629.27742-1-mynameisjune111@gmail.com?part=9