Re: [PATCH v23 09/14] ALSA: virtio: bound PCM transfers and retain DMA buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ALSA: virtio: partial RX on stop latches XRUN in
  virtsnd_pcm_msg_complete()
--

commit 8701e733a8297edcfeb307fc6817693d437f58dc
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: bound PCM transfers and retain DMA buffers

This commit validates PCM buffer geometry, ACK transfer bounds, and RX
completions while retaining backing DMA pages until virtqueue messages are
freed.

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 59ad99e21e899..c728e8fafb37f 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c

[ ... ]

> @@ -356,42 +536,81 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
>  				     size_t written_bytes)
>  {
>  	struct virtio_pcm_substream *vss = msg->substream;
> +	bool capture_invalid = false;
>  
>  	/*
> -	 * hw_ptr always indicates the buffer position of the first I/O message
> -	 * in the virtqueue. Therefore, on each completion of an I/O message,
> -	 * the hw_ptr value is unconditionally advanced.
> +	 * Completed requests can return captured data after STOP while the PCM
> +	 * buffer remains prepared. Once preparation ends, completions only retire
> +	 * queue ownership.
>  	 */
>  	guard(spinlock)(&vss->lock);
> -	/*
> -	 * If the capture substream returned an incorrect status, then just
> -	 * increase the hw_ptr by the message size.
> -	 */
> -	if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK ||
> -	    written_bytes <= sizeof(msg->status))
> -		vss->hw_ptr += msg->length;
> -	else
> -		vss->hw_ptr += written_bytes - sizeof(msg->status);
> +	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> +	    (!vss->prepared || vss->xfer_xrun ||
> +	     (vss->stopped && written_bytes <= sizeof(msg->status)))) {
> +		msg->length = 0;
> +		vss->msg_count--;
> +		if (!vss->msg_count)
> +			wake_up_all(&vss->msg_empty);
> +		return;
> +	}
>  
> -	if (vss->hw_ptr >= vss->buffer_bytes)
> -		vss->hw_ptr -= vss->buffer_bytes;
> +	/* Only the next complete ring slice can extend captured data. */
> +	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> +	    (msg->data_offset != vss->hw_ptr ||
> +	     written_bytes < sizeof(msg->status) ||
> +	     le32_to_cpu(msg->status.status) != VIRTIO_SND_S_OK ||
> +	     written_bytes - sizeof(msg->status) != msg->length)) {
> +		dev_err_ratelimited(&vss->snd->vdev->dev,
> +				    "SID %u: invalid capture position, status or length\n",
> +			vss->sid);
> +		vss->xfer_xrun = true;
> +		capture_invalid = true;
> +	}

[Severity: Medium]
Does this latch vss->xfer_xrun when the device completes a partially filled RX
buffer upon VIRTIO_SND_R_PCM_STOP?

Under the VirtIO Sound specification (section 5.14.6.8.2.1), the last RX
buffer returned when a capture stream stops may contain fewer than
period_bytes of PCM frames.  During SNDRV_PCM_IOCTL_DRAIN,
virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_STOP) sets vss->stopped = true and sends
VIRTIO_SND_R_PCM_STOP while leaving the substream in SNDRV_PCM_STATE_DRAINING.
A partial RX completion has written_bytes > sizeof(msg->status) and
written_bytes - sizeof(msg->status) < msg->length, so it bypasses the early
return and sets vss->xfer_xrun = true.  That causes virtsnd_pcm_cp_ack() to
fail with -EBADFD and virtsnd_pcm_cp_pointer() to report SNDRV_PCM_POS_XRUN,
preventing userspace from draining full periods that had already completed
before STOP.

Similarly, SNDRV_PCM_TRIGGER_PAUSE_PUSH sends VIRTIO_SND_R_PCM_STOP without
setting vss->stopped, so any empty or partial in-flight RX buffer returned on
pause also latches vss->xfer_xrun.

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