Re: [PATCH v8 13/14] ALSA: virtio: synchronize staged capture reset with PCM positions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: virtio: unupdated capture_notified_periods in virtsnd_pcm_reset()
- [High] ALSA: virtio: false XRUN and skipped rebase in virtsnd_pcm_reset() race
--

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

ALSA: virtio: synchronize staged capture reset with PCM positions

This commit introduces a custom PCM reset operation for staged capture
streams to synchronize private copied and acknowledged byte counters with
ALSA core pointer resets. When resetting a running stream, it calls
snd_pcm_lib_reset(), advances capture_acked_total to
capture_pointer_seen_total, and attempts to refill the RX queue.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 66560d82b7f8e..ba14f395eef8a 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -1591,6 +1598,58 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
>  	return virtsnd_pcm_sync_stop_locked(substream);
>  }
>  
> +/* The core holds the stream lock through the matching appl_ptr reset. */
> +static int virtsnd_pcm_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);
> +	size_t position;
> +	size_t period;
> +	int rc;
> +
> +	/* Keep ALSA's pointer validation, time accounting and XRUN processing. */
> +	rc = snd_pcm_lib_reset(substream);
> +	if (rc || !vss->capture_staging)
> +		return rc;
> +
> +	guard(spinlock_irqsave)(&queue->lock);
> +	guard(spinlock)(&vss->lock);
> +	if (vss->resetting)
> +		return -ECANCELED;
> +	/* The common reset has already handled a pointer-reported XRUN. */
> +	if (vss->xfer_xrun)
> +		return 0;

[Severity: High]
Can an RX completion arriving between snd_pcm_lib_reset() and acquiring
vss->lock trigger a false XRUN and skip the private counter rebase?

During snd_pcm_do_reset() -> virtsnd_pcm_reset(), snd_pcm_lib_reset() calls
virtsnd_pcm_cp_pointer(), which records vss->capture_pointer_seen_total and
releases vss->lock while vss->capture_ready still holds the pre-reset
unacknowledged byte count:

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_cp_pointer() {
    ...
				} else {
					vss->capture_pointer_seen_total = total;
					hw_ptr = virtsnd_pcm_ring_offset(total, vss->buffer_bytes);
				}
    ...
}

In managed mode (!vss->capture_free_running), if an in-flight RX completion
arrives before virtsnd_pcm_reset() acquires queue->lock and vss->lock,
virtsnd_pcm_capture_complete() checks the incoming bytes against the stale
pre-reset vss->capture_ready:

sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_capture_complete() {
    ...
		else if (!free_running &&
			 audio_bytes >= vss->buffer_bytes - vss->capture_ready)
			invalid_reason = "ring_capacity";
		else if (!free_running &&
			 check_add_overflow(vss->capture_ready, audio_bytes,
					    &next_ready))
			invalid_reason = "ready_overflow";
		else if (!free_running && next_ready >= vss->buffer_bytes)
			invalid_reason = "ring_full";
    ...
}

When the pre-reset unacknowledged bytes plus audio_bytes reach
vss->buffer_bytes, virtsnd_pcm_capture_complete() rejects the completion,
drops the captured audio, and sets vss->xfer_xrun = true.

When virtsnd_pcm_reset() then acquires the locks, it sees vss->xfer_xrun and
returns 0 without updating vss->capture_acked_total, vss->capture_ready, or
vss->pcm_indirect.appl_ptr, even though snd_pcm_lib_reset() did not observe
the XRUN and snd_pcm_do_reset() proceeds to rebase runtime->status->hw_ptr
and runtime->control->appl_ptr.

> +	if (snd_pcm_running(substream)) {
> +		/* Keep completions arriving after the core's pointer observation. */
> +		vss->capture_acked_total = vss->capture_pointer_seen_total;
> +		vss->capture_ready = vss->capture_free_running ? 0 :
> +			vss->capture_copied_total - vss->capture_acked_total;
> +	} else {
> +		position = frames_to_bytes(runtime, runtime->status->hw_ptr);
> +		vss->hw_ptr = position;
> +		period = snd_pcm_lib_period_bytes(substream);
> +		vss->capture_copied_total = position;
> +		vss->capture_acked_total = position;
> +		vss->capture_pointer_seen_total = position;
> +		vss->capture_ready = 0;
> +		vss->capture_notified_periods = period ? position / period : 0;
> +	}

[Severity: High]
Should the running-stream branch in virtsnd_pcm_reset() also advance
vss->capture_notified_periods to match vss->capture_pointer_seen_total?

If a completion in virtsnd_pcm_capture_complete() crosses a period boundary
and schedules vss->elapsed_period, and userspace invokes
SNDRV_PCM_IOCTL_RESET before virtsnd_pcm_period_elapsed() acquires the PCM
stream lock, snd_pcm_lib_reset() -> snd_pcm_update_hw_ptr() ->
virtsnd_pcm_cp_pointer() observes the new vss->capture_copied_total and
updates vss->capture_pointer_seen_total.

In snd_pcm_do_reset(), the core then folds that position into
runtime->hw_ptr_interrupt:

sound/core/pcm_native.c:snd_pcm_do_reset() {
    ...
		runtime->hw_ptr_base = 0;
		runtime->hw_ptr_interrupt = runtime->status->hw_ptr -
			runtime->status->hw_ptr % runtime->period_size;
		runtime->silence_start = runtime->status->hw_ptr;
		runtime->silence_filled = 0;
		runtime->control->appl_ptr = runtime->status->hw_ptr;
    ...
}

Because the running branch in virtsnd_pcm_reset() leaves
vss->capture_notified_periods at its pre-reset value, the pending
virtsnd_pcm_period_elapsed() worker later sees period_floor >
vss->capture_notified_periods for a period boundary already accounted for in
runtime->hw_ptr_interrupt:

sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed() {
    ...
			period_floor = div64_ul(vss->capture_copied_total, period_bytes);
			if (period_floor > vss->capture_notified_periods) {
				/* Reserve crossed boundaries before the callback. */
				vss->capture_notified_periods = period_floor;
				notify = true;
			}
    ...
}

This triggers a duplicate snd_pcm_period_elapsed_under_stream_lock() ->
snd_pcm_update_hw_ptr0(substream, 1) call where delta > new_hw_ptr:

sound/core/pcm_lib.c:snd_pcm_update_hw_ptr0() {
    ...
	if (in_interrupt) {
		/* we know that one period was processed */
		/* delta = "expected next hw_ptr" for in_interrupt != 0 */
		delta = runtime->hw_ptr_interrupt + runtime->period_size;
		if (delta > new_hw_ptr) {
			/* check for double acknowledged interrupts */
			hdelta = curr_jiffies - runtime->hw_ptr_jiffies;
			if (hdelta > runtime->hw_ptr_buffer_jiffies/2 + 1) {
				hw_base += runtime->buffer_size;
    ...
}

If the jiffies delta exceeds half the buffer duration, snd_pcm_update_hw_ptr0()
treats the duplicate notification as a missed full-buffer wrap-around and
advances runtime->status->hw_ptr by a full buffer_size of uncaptured frames.

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