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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] ALSA: virtio: false XRUN on capture drain in virtsnd_pcm_msg_complete()
- [High] ALSA: virtio: buffer_mutex trylock in ack corrupts indirect PCM state
- [High] ALSA: virtio: buffer_mutex trylock in virtsnd_pcm_pb_ack() fails during RESET
- [Medium] ALSA: virtio: staged capture drops RX completions during pause in virtsnd_pcm_capture_complete()
- [Medium] ALSA: virtio: missed XRUN wakeup when RX ring fills during pause in virtsnd_pcm_msg_complete()
- [Medium] ALSA: virtio: virtsnd_pcm_event() skips period work for ordinary capture XRUN events
- [Medium] ALSA: virtio: virtsnd_pcm_start_queue() drops period notifications
- [Medium] ALSA: virtio: failed STOP in virtsnd_pcm_trigger() misses xfer_xrun latch
- [Medium] ALSA: virtio: premature state reset in virtsnd_pcm_prepare_locked()
- [Medium] ALSA: virtio: missing stopped check in virtsnd_pcm_capture_staged_ack()
--

commit 89969d05596ec30bdd3e3e6560275f8867f0911d
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
monotonic position counters, while separating device running state, RELEASE
obligations, and reset epochs from ALSA runtime state. It also serializes PCM
lifecycle callbacks with lifecycle_lock and updates pause, XRUN, and period
wakeup handling across ordinary and staged capture paths.

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

> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> index be3893de40a55..aed94a892aaa4 100644
> --- a/sound/virtio/virtio_pcm.c
> +++ b/sound/virtio/virtio_pcm.c

[ ... ]

> @@ -517,8 +630,11 @@ void virtsnd_pcm_event(struct virtio_snd *snd, struct virtio_snd_event *event)
>  		break;
>  	case VIRTIO_SND_EVT_PCM_XRUN:
>  		scoped_guard(spinlock, &vss->lock) {
> -			if (vss->xfer_enabled)
> +			if (vss->xfer_enabled) {
>  				vss->xfer_xrun = true;
> +				if (vss->capture_staging)
> +					virtsnd_pcm_schedule_period_elapsed(vss);
> +			}
>  		}
>  		break;
>  	}

[Severity: Medium]
When VIRTIO_SND_EVT_PCM_XRUN arrives in virtsnd_pcm_event() while
vss->xfer_enabled is true, why is virtsnd_pcm_schedule_period_elapsed(vss)
only called when vss->capture_staging is set?

In virtsnd_pcm_period_elapsed(), ordinary capture now checks vss->xfer_xrun
and calls snd_pcm_progress_under_stream_lock(substream), which observes
SNDRV_PCM_POS_XRUN from virtsnd_pcm_cp_pointer() and triggers
__snd_pcm_xrun().

However, if the device sends VIRTIO_SND_EVT_PCM_XRUN on an ordinary capture
substream (!vss->capture_staging) and stops completing RX buffers, skipping
virtsnd_pcm_schedule_period_elapsed(vss) here leaves ALSA unaware of the XRUN
until a read timeout expires. Should this check vss->direction ==
SNDRV_PCM_STREAM_CAPTURE instead of vss->capture_staging?

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index b94ab24f5ffef..cdd26729568a1 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c

[ ... ]

> @@ -478,6 +530,290 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,

[ ... ]

> +static void virtsnd_pcm_capture_complete(struct virtio_pcm_msg *msg,
> +					 size_t written_bytes)
> +{
> +	struct virtio_pcm_substream *vss = msg->substream;
> +	size_t audio_bytes = 0;
> +	size_t next_ready = 0;
> +	u64 next_copied_total = 0;
> +	bool free_running = false;
> +	bool got_audio = false;
> +
> +	guard(spinlock)(&vss->lock);
> +	vss->msg_count--;
> +	if (vss->runtime_active && vss->xfer_enabled && !vss->xfer_xrun) {
> +		struct snd_pcm_runtime *runtime = vss->substream->runtime;

[Severity: Medium]
In virtsnd_pcm_capture_complete(), does gating completion processing on
vss->xfer_enabled drop in-flight RX completions during a native pause
transition?

When userspace triggers SNDRV_PCM_TRIGGER_PAUSE_PUSH on an active staged
capture stream (with virtsnd_capture_pause_restart disabled),
virtsnd_pcm_trigger() clears vss->xfer_enabled = false under vss->lock while
keeping vss->prepared = true and vss->stopped = false, and then calls
virtsnd_pcm_stop_device() to send VIRTIO_SND_R_PCM_STOP synchronously:

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trigger() {
	...
	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);
}

While VIRTIO_SND_R_PCM_STOP is in flight (both when STOP succeeds and when
the device rejects STOP), the device can complete already-queued RX requests.
Unlike ordinary capture in virtsnd_pcm_msg_complete(), which checks
vss->prepared && !vss->stopped for NOMMU copyout and still updates
vss->hw_ptr, vss->capture_copied_total, and vss->xfer_xrun when
vss->xfer_enabled is false, virtsnd_pcm_capture_complete() skips status
validation, overrun checks, memcpy() into runtime->dma_area, and counter
updates, and then resets msg->length = 0.

When pause is rejected or later resumed via SNDRV_PCM_TRIGGER_PAUSE_RELEASE,
virtsnd_pcm_capture_enqueue() zeroes and re-queues those descriptors,
permanently losing the captured audio frames and any device status or ring
overrun error.

[ ... ]

> @@ -580,14 +924,22 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
>  	 * the actual SG payload capacity before accounting them.
>  	 */
>  	if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK ||
> -	    written_bytes <= sizeof(msg->status)) {
> -		vss->hw_ptr += msg->length;
> -	} else {
> -		size_t data_bytes = min_t(size_t,
> -					  written_bytes - sizeof(msg->status),
> -					  msg->length);
> -
> -		vss->hw_ptr += data_bytes;
> +	    written_bytes <= sizeof(*msg->status))
> +		completed_bytes = msg->length;
> +	else
> +		completed_bytes = min_t(size_t,
> +					written_bytes - sizeof(*msg->status),
> +					msg->length);
> +	vss->hw_ptr += completed_bytes;
> +	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) {
> +		/* A full unseen ring cannot be represented by the modulo pointer. */
> +		if (check_add_overflow(vss->capture_copied_total,
> +				       (u64)completed_bytes,
> +				       &vss->capture_copied_total) ||
> +		    vss->capture_copied_total < vss->capture_pointer_seen_total ||
> +		    vss->capture_copied_total - vss->capture_pointer_seen_total >=
> +			vss->buffer_bytes)
> +			vss->xfer_xrun = true;
>  	}

[Severity: High]
Can this unconditional update of vss->capture_copied_total in
virtsnd_pcm_msg_complete() trigger a false XRUN during capture drain?

When an application calls snd_pcm_drain() (SNDRV_PCM_IOCTL_DRAIN) on a
running ordinary capture stream with unread frames in the buffer,
snd_pcm_do_drain_init() transitions the stream to SNDRV_PCM_STATE_DRAINING
and invokes virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_STOP), which sets
vss->stopped = true and vss->xfer_enabled = false and sends
VIRTIO_SND_R_PCM_STOP.

In SNDRV_PCM_STATE_DRAINING, snd_pcm_running(substream) is false and
virtsnd_pcm_cp_pointer() is no longer called, so
vss->capture_pointer_seen_total stops advancing. However,
virtsnd_pcm_msg_complete() continues to add completed_bytes (including a full
period msg->length when written_bytes <= sizeof(*msg->status) for empty or
status-only completions) to vss->hw_ptr and vss->capture_copied_total even
when vss->stopped is true.

When the seeded RX ring descriptors complete after STOP or RELEASE,
vss->capture_copied_total - vss->capture_pointer_seen_total reaches
vss->buffer_bytes and latches vss->xfer_xrun = true. Subsequent draining
reads via snd_pcm_readi() or SNDRV_PCM_IOCTL_SYNC_PTR then call
virtsnd_pcm_cp_ack_locked(), which checks:

	if (vss->xfer_xrun ||
	    (vss->stopped && runtime->state != SNDRV_PCM_STATE_DRAINING)) {
		rc = -EPIPE;
	}

and aborts the capture drain with -EPIPE, discarding the already-captured
audio frames.

>  	if (vss->hw_ptr >= vss->buffer_bytes)
> @@ -595,7 +947,9 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
>  
>  	msg->length = 0;
>  
> -	vss->xfer_xrun = false;
> +	/* Preserve a latched capture error until prepare recovers it. */
> +	if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK)
> +		vss->xfer_xrun = false;
>  	vss->msg_count--;
>  
>  	if (vss->xfer_enabled) {
> @@ -603,8 +957,8 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
>  
>  		runtime->delay =
>  			bytes_to_frames(runtime,
> -					le32_to_cpu(msg->status.latency_bytes));
> -		schedule_work(&vss->elapsed_period);
> +					le32_to_cpu(msg->status->latency_bytes));
> +		virtsnd_pcm_schedule_period_elapsed(vss);
>  	} else if (!vss->msg_count) {
>  		wake_up_all(&vss->msg_empty);
>  	}

[Severity: Medium]
What happens if in-flight ordinary capture RX completions fill the unread
ring while SNDRV_PCM_TRIGGER_PAUSE_PUSH is waiting for VIRTIO_SND_R_PCM_STOP
and the STOP request succeeds?

During SNDRV_PCM_TRIGGER_PAUSE_PUSH, virtsnd_pcm_trigger() clears
vss->xfer_enabled = false before calling virtsnd_pcm_stop_device(vss, epoch).
If the device completes queued RX requests before processing
VIRTIO_SND_R_PCM_STOP and vss->capture_copied_total -
vss->capture_pointer_seen_total reaches vss->buffer_bytes,
virtsnd_pcm_msg_complete() latches vss->xfer_xrun = true, but skips
virtsnd_pcm_schedule_period_elapsed(vss) because vss->xfer_enabled is false.

When virtsnd_pcm_stop_device() returns 0, virtsnd_pcm_trigger() only checks
if (rc && command == SNDRV_PCM_TRIGGER_PAUSE_PUSH) and returns 0 without
scheduling elapsed_period. ALSA transitions runtime->state to
SNDRV_PCM_STATE_PAUSED, and the paused_xrun handler in
virtsnd_pcm_period_elapsed() never runs to move the stream to
SNDRV_PCM_STATE_XRUN, leaving readers in wait_for_avail() sleeping
indefinitely.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index ad602b6b6530b..d587c8f699553 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -320,47 +706,383 @@ 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;
> +		}
> +	}
> +
> +	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;
> +	}
> +	if (pause_restart) {
> +		if (rc) {
> +			if (rc == -ECANCELED)
> +				return rc;
> +			virtsnd_pcm_pause_restart_fail(vss, false, epoch);
> +		} else {
> +			scoped_guard(spinlock_irqsave, &vss->lock) {
> +				if (vss->resetting || vss->reset_epoch != epoch)
> +					return -ECANCELED;
> +				vss->stopped = false;
> +				vss->xfer_xrun = false;
> +			}
> +		}
> +	}
> +
> +	return rc;
> +}

[Severity: Medium]
In virtsnd_pcm_prepare_locked(), if virtsnd_pcm_dev_prepare() fails when
pause_restart is false, doesn't clearing vss->xfer_xrun and resetting
vss->pcm_indirect and the capture counters before sending
VIRTIO_SND_R_PCM_PREPARE leave the driver out of sync with ALSA's runtime
state?

When userspace invokes SNDRV_PCM_IOCTL_PREPARE on a capture substream in
SNDRV_PCM_STATE_XRUN or SNDRV_PCM_STATE_DRAINING, snd_pcm_do_prepare() calls
virtsnd_pcm_prepare_locked(). Before virtsnd_pcm_dev_prepare() is called,
this block clears vss->xfer_xrun = false (when !pause_restart), calls
virtsnd_pcm_msg_reset(vss), and zeroes vss->pcm_indirect.

If virtsnd_pcm_dev_prepare() then returns an error, snd_pcm_do_prepare()
aborts before snd_pcm_do_reset() and snd_pcm_post_prepare(), leaving
runtime->state and runtime->control->appl_ptr unchanged. Because only the
pause_restart branch restores vss->xfer_xrun = true and vss->stopped = true
on failure:

- If the stream was in SNDRV_PCM_STATE_XRUN, the latched vss->xfer_xrun state
  is lost even though prepare failed.
- If the stream was in SNDRV_PCM_STATE_DRAINING, runtime->state remains
  SNDRV_PCM_STATE_DRAINING with runtime->control->appl_ptr at its pre-prepare
  value while vss->pcm_indirect is zeroed and vss->prepared, vss->stopped,
  and vss->xfer_xrun are all false; a subsequent read or
  SNDRV_PCM_IOCTL_SYNC_PTR in SNDRV_PCM_STATE_DRAINING bypasses the
  !vss->prepared and vss->stopped checks in virtsnd_pcm_cp_ack_locked() and
  subtracts appl_ptr from a zeroed vss->pcm_indirect.sw_ready.

[ ... ]

> +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->capture_rx_seeded) {
> +			rc = virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes);
> +			if (!rc)
> +				vss->capture_rx_seeded = true;
> +		}
> +	}
>  	if (!rc) {
> -		guard(spinlock_irqsave)(&vss->lock);
> -		vss->release_required = true;
> +		if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> +		    !vss->capture_staging) {
> +			size_t period = snd_pcm_lib_period_bytes(vss->substream);
> +
> +			/* A synthetic resume notification is not a new period IRQ. */
> +			vss->capture_notified_periods = period ?
> +				div64_ul(vss->capture_copied_total, period) : 0;
> +		}
> +		vss->xfer_enabled = true;
>  	}
>  	return rc;
>  }

[Severity: Medium]
Does overwriting vss->capture_notified_periods in virtsnd_pcm_start_queue()
drop period notifications for RX periods that completed while
vss->xfer_enabled was false?

During SNDRV_PCM_TRIGGER_PAUSE_PUSH, virtsnd_pcm_trigger() clears
vss->xfer_enabled = false before calling virtsnd_pcm_stop_device(). If one or
more RX periods complete in virtsnd_pcm_msg_complete() while
vss->xfer_enabled is false (either before STOP takes effect or when STOP is
rejected by the device), vss->capture_copied_total advances across a period
boundary without scheduling vss->elapsed_period.

When virtsnd_pcm_start_queue() subsequently runs on rejected PAUSE_PUSH
recovery or on SNDRV_PCM_TRIGGER_PAUSE_RELEASE, it overwrites
vss->capture_notified_periods with div64_ul(vss->capture_copied_total,
period) before virtsnd_pcm_period_elapsed() executes. When
virtsnd_pcm_period_elapsed() then runs:

sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed() {
	...
	partial = vss->xfer_xrun ||
		period_floor <= vss->capture_notified_periods;
	if (!partial)
		vss->capture_notified_periods = period_floor;
}

period_floor <= vss->capture_notified_periods is forced to true (partial =
true), so it calls snd_pcm_progress_under_stream_lock() instead of
snd_pcm_period_elapsed_under_stream_lock(), losing the period notification,
runtime->hw_ptr_interrupt update, and PCM timer tick for the completed
periods.

> @@ -377,72 +1099,226 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)

[ ... ]

>  	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) {
> +			if (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);
> +				}
> +			} else if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) {
> +				virtsnd_pcm_schedule_period_elapsed(vss);
> +			}
>  		}
> +		return rc;

[Severity: Medium]
If virtsnd_pcm_stop_device() fails during SNDRV_PCM_TRIGGER_STOP on a capture
stream, should vss->xfer_xrun also be latched here?

When userspace invokes SNDRV_PCM_IOCTL_DRAIN on a running capture stream with
unread frames, snd_pcm_do_drain_init() calls snd_pcm_do_stop(substream,
SNDRV_PCM_STATE_DRAINING), which ignores the return value of
substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP) and sets
runtime->state to SNDRV_PCM_STATE_DRAINING.

When virtsnd_pcm_stop_device() fails, vss->device_running remains true.
Unlike virtsnd_pcm_pause_restart_stop(), which calls
virtsnd_pcm_pause_restart_failed() to set vss->xfer_xrun = true, normal-mode
virtsnd_pcm_trigger() only handles rc != 0 for SNDRV_PCM_TRIGGER_PAUSE_PUSH
and leaves vss->xfer_xrun false. Because both virtsnd_pcm_capture_staged_ack()
and virtsnd_pcm_cp_ack_locked() allow ACK in SNDRV_PCM_STATE_DRAINING when
vss->stopped is true unless vss->xfer_xrun is set, draining capture reads and
ACKs can proceed while the device is still running and DMA-writing into the
capture buffer.

[ ... ]

> @@ -606,17 +1560,134 @@ static int virtsnd_pcm_pb_ack(struct snd_pcm_substream *substream)
>  	return 0;
>  }
>  
> -static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
> +/* The PCM lock is held; exclude every member's buffer reconfiguration. */
> +static int virtsnd_pcm_pb_ack(struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	int rc;
> +
> +	if (!mutex_trylock(&runtime->buffer_mutex))
> +		return -EBADFD;
> +	rc = virtsnd_pcm_pb_ack_locked(substream);
> +	mutex_unlock(&runtime->buffer_mutex);
> +	return rc;
> +}

[Severity: High]
Can failing mutex_trylock(&runtime->buffer_mutex) in virtsnd_pcm_pb_ack() and
virtsnd_pcm_cp_ack() corrupt vss->pcm_indirect when another thread transiently
holds runtime->buffer_mutex on a running stream?

In sound/core/pcm_native.c, snd_pcm_hw_refine_with_lock(),
snd_pcm_hw_params(), snd_pcm_hw_free(), and snd_pcm_action_nonatomic()
acquire runtime->buffer_mutex via snd_pcm_buffer_access_lock() (or
mutex_lock_nested() on linked group members in snd_pcm_action_group()) before
acquiring the PCM stream lock to check whether runtime->state is
SNDRV_PCM_STATE_RUNNING. Meanwhile, virtsnd_pcm_period_elapsed(),
snd_pcm_status64(), and snd_pcm_sync_ptr() hold the PCM stream lock without
incrementing runtime->buffer_accessing.

If virtsnd_pcm_period_elapsed() or snd_pcm_status64() runs while another
thread holds runtime->buffer_mutex waiting for the PCM stream lock,
virtsnd_pcm_pb_pointer() or virtsnd_pcm_cp_pointer() calls
snd_pcm_indirect_playback_pointer() or snd_pcm_indirect_capture_pointer(),
which first mutates vss->pcm_indirect in place (updating hw_io, hw_ready, and
sw_io) and then invokes substream->ops->ack(substream).

When mutex_trylock(&runtime->buffer_mutex) fails and returns -EBADFD,
snd_pcm_indirect_playback_pointer() and snd_pcm_indirect_capture_pointer()
ignore the error (only checking err == -EPIPE), leaving vss->pcm_indirect
half-updated without running snd_pcm_indirect_playback_transfer() or
snd_pcm_indirect_capture_transfer(). Concurrent SNDRV_PCM_IOCTL_SYNC_PTR
calls without HWSYNC will also fail with -EBADFD whenever another thread is
in the middle of an ioctl that locks buffer_mutex before checking stream
state.

> +
> +static int virtsnd_pcm_capture_staged_ack(struct virtio_pcm_substream *vss,
> +					  struct snd_pcm_runtime *runtime)
> +{
> +	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +	int rc = 0;
> +
> +	/*
> +	 * The RX callback holds the queue lock while draining completions.
> +	 * Account ALSA's read under the substream lock alone so completions later
> +	 * in that drain observe the newly released ring space.
> +	 */
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		snd_pcm_uframes_t appl_ptr;
> +		snd_pcm_sframes_t diff;
> +		size_t bytes;
> +		u64 outstanding;
> +		u64 next_acked_total;
> +		bool free_running;
> +
> +		if (!vss->runtime_active || !runtime || !runtime->control ||
> +		    !runtime->dma_area || vss->suspended)
> +			return -EBADFD;
> +		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;
> +		if (vss->xfer_xrun)
> +			return -EPIPE;

[Severity: Medium]
Should virtsnd_pcm_capture_staged_ack() also check vss->stopped (or verify
that runtime->state permits acknowledgments), similar to
virtsnd_pcm_cp_ack_locked()?

When ALSA transitions a running staged capture substream to
SNDRV_PCM_STATE_XRUN without the virtio driver setting vss->xfer_xrun (for
example, when snd_pcm_update_state() detects avail >= runtime->stop_threshold,
or when userspace issues SNDRV_PCM_IOCTL_XRUN via snd_pcm_xrun()),
__snd_pcm_xrun() calls snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN), which
invokes virtsnd_pcm_trigger(substream, SNDRV_PCM_TRIGGER_STOP) without
calling snd_pcm_sync_stop(). That sets vss->stopped = true and
vss->xfer_enabled = false while leaving vss->prepared = true and
vss->xfer_xrun = false. Similarly, if SNDRV_PCM_IOCTL_DROP is called while in
SNDRV_PCM_STATE_PREPARED, snd_pcm_do_stop() transitions runtime->state to
SNDRV_PCM_STATE_SETUP without setting stop_operating, leaving vss->prepared =
true.

If userspace then invokes SNDRV_PCM_IOCTL_SYNC_PTR without
SNDRV_PCM_SYNC_PTR_HWSYNC or SNDRV_PCM_SYNC_PTR_APPL, snd_pcm_sync_ptr()
calls pcm_lib_apply_appl_ptr() -> virtsnd_pcm_cp_ack() ->
virtsnd_pcm_capture_staged_ack(). Because virtsnd_pcm_capture_staged_ack()
does not check (vss->stopped && runtime->state != SNDRV_PCM_STATE_DRAINING),
it accepts the ACK and advances vss->capture_acked_total, vss->capture_ready,
and vss->pcm_indirect.appl_ptr while the stream is stopped in
SNDRV_PCM_STATE_XRUN or SNDRV_PCM_STATE_SETUP.

[ ... ]

> @@ -641,6 +1712,50 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
>  	return 0;
>  }
>  
> +/*
> + * Linked prepare locks each member's buffer_mutex, while buffer_accessing is
> + * negative only on its initiating member. ACK must exclude every prepare.
> + * The PCM lock is already held, so a failed trylock must return immediately.
> + */
> +static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	int rc;
> +
> +	if (!mutex_trylock(&runtime->buffer_mutex))
> +		return -EBADFD;
> +	rc = virtsnd_pcm_cp_ack_locked(substream);
> +	mutex_unlock(&runtime->buffer_mutex);
> +	return rc;
> +}

[Severity: High]
Won't mutex_trylock(&runtime->buffer_mutex) in virtsnd_pcm_pb_ack() and
virtsnd_pcm_cp_ack() always fail during SNDRV_PCM_IOCTL_RESET on a running
stream?

When userspace invokes SNDRV_PCM_IOCTL_RESET, snd_pcm_reset() calls
snd_pcm_action_nonatomic(&snd_pcm_action_reset, substream,
ACTION_ARG_IGNORE), which acquires substream->runtime->buffer_mutex via
snd_pcm_buffer_access_lock() (and locks runtime->buffer_mutex on all linked
group members in snd_pcm_action_group()).

While holding runtime->buffer_mutex on the same thread, snd_pcm_do_reset()
calls snd_pcm_lib_ioctl_reset() (or snd_pcm_lib_reset()). When the stream is
running, snd_pcm_lib_ioctl_reset() acquires the PCM stream lock and calls
snd_pcm_update_hw_ptr(substream) -> substream->ops->pointer(substream).

For playback (virtsnd_pcm_pb_pointer()) and ordinary capture
(virtsnd_pcm_cp_pointer()), snd_pcm_indirect_playback_pointer() and
snd_pcm_indirect_capture_pointer() first mutate vss->pcm_indirect in place
(updating hw_io, hw_ready, and sw_io) and then invoke
substream->ops->ack(substream). Because the calling thread already holds
runtime->buffer_mutex, mutex_trylock(&runtime->buffer_mutex) in
virtsnd_pcm_pb_ack() and virtsnd_pcm_cp_ack() is guaranteed to fail and
return -EBADFD.

snd_pcm_indirect_playback_pointer() and snd_pcm_indirect_capture_pointer()
ignore -EBADFD (only checking err == -EPIPE) and skip
snd_pcm_indirect_playback_transfer() / snd_pcm_indirect_capture_transfer()
after vss->pcm_indirect has already been modified. Immediately afterwards,
snd_pcm_post_reset() sets runtime->control->appl_ptr =
runtime->status->hw_ptr without updating vss->pcm_indirect.appl_ptr, causing
the next playback ACK or period completion to compute a negative diff =
runtime->control->appl_ptr - rec->appl_ptr and abort the stream with -EPIPE.

Later commits in this series ("ALSA: pcm: add an optional reset operation
under the stream lock", "ALSA: virtio: synchronize staged capture accounting
during RESET", and "ALSA: virtio: rebase ordinary capture accounting on
RESET") add .reset callbacks for capture streams, but playback streams in
virtsnd_pcm_ops[0] still have no .reset callback or bypass in
virtsnd_pcm_pb_ack(), leaving SNDRV_PCM_IOCTL_RESET broken on running
playback streams.

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