Re: [PATCH v5 04/11] ALSA: virtio: Keep PCM DMA buffers alive through device ownership

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: virtio: virtsnd_pcm_msg_complete() drops NOMMU capture data on pause
--

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

ALSA: virtio: Keep PCM DMA buffers alive through device ownership

This commit retains references to the pages backing ALSA's vmalloc buffer
until its queued I/O messages are freed, uses a driver-owned bounce buffer on
NOMMU systems, and clamps device-reported capture lengths to the message
capacity.

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 762480363b861..b7f2334bb221c 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
[ ... ]
> @@ -363,15 +539,48 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
>  	 * the hw_ptr value is unconditionally advanced.
>  	 */
>  	guard(spinlock)(&vss->lock);
> +#ifndef CONFIG_MMU
> +	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> +	    vss->xfer_enabled && written_bytes > sizeof(msg->status)) {
> +		struct snd_pcm_runtime *runtime =
> +			vss->substream ? vss->substream->runtime : NULL;
> +		size_t reported_bytes = written_bytes - sizeof(msg->status);
> +		size_t data_bytes = min_t(size_t, reported_bytes, msg->length);
> +
> +		if (!runtime || !runtime->dma_area || !vss->owned_dma_area ||
> +		    msg->data_offset > runtime->dma_bytes ||
> +		    data_bytes > runtime->dma_bytes - msg->data_offset ||
> +		    msg->data_offset > vss->owned_dma_bytes ||
> +		    data_bytes > vss->owned_dma_bytes - msg->data_offset)
> +			dev_err_ratelimited(&vss->snd->vdev->dev,
> +					    "SID %u: invalid capture buffer for I/O message\n",
> +				vss->sid);
> +		else
> +			memcpy(runtime->dma_area + msg->data_offset,
> +			       vss->owned_dma_area + msg->data_offset, data_bytes);
> +	}
> +#endif
> +	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE &&
> +	    written_bytes > sizeof(msg->status) &&
> +	    written_bytes - sizeof(msg->status) > msg->length)
> +		dev_err_ratelimited(&vss->snd->vdev->dev,
> +				    "SID %u: capture length exceeds I/O message capacity\n",
> +			vss->sid);
>  	/*
>  	 * If the capture substream returned an incorrect status, then just
> -	 * increase the hw_ptr by the message size.
> +	 * increase the hw_ptr by the message size. Clamp valid capture lengths to
> +	 * the actual SG payload capacity before accounting them.
>  	 */
>  	if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK ||
> -	    written_bytes <= sizeof(msg->status))
> +	    written_bytes <= sizeof(msg->status)) {
>  		vss->hw_ptr += msg->length;
> -	else
> -		vss->hw_ptr += written_bytes - sizeof(msg->status);
> +	} else {
> +		size_t data_bytes = min_t(size_t,
> +					  written_bytes - sizeof(msg->status),
> +					  msg->length);
> +
> +		vss->hw_ptr += data_bytes;
> +	}

[Severity: High]
Can gating the NOMMU capture bounce copy on vss->xfer_enabled drop captured
audio data when a stream is paused?

In sound/virtio/virtio_pcm_ops.c, virtsnd_pcm_trigger() handles
SNDRV_PCM_TRIGGER_PAUSE_PUSH by clearing vss->xfer_enabled while leaving
vss->stopped false before synchronously sending VIRTIO_SND_R_PCM_STOP:

	case SNDRV_PCM_TRIGGER_STOP:
	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
		scoped_guard(spinlock_irqsave, &vss->lock) {
			if (command != SNDRV_PCM_TRIGGER_PAUSE_PUSH)
				vss->stopped = true;
			vss->xfer_enabled = false;
		}

		msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_STOP,
						GFP_KERNEL);
		if (!msg)
			return -ENOMEM;

		return virtsnd_ctl_msg_send_sync(snd, msg);

If an in-flight capture I/O message completes while vss->xfer_enabled is
false during pause, virtsnd_pcm_msg_complete() skips the memcpy() from
vss->owned_dma_area to runtime->dma_area, but still advances vss->hw_ptr by
data_bytes and resets msg->length to 0.

When the stream is later resumed with SNDRV_PCM_TRIGGER_PAUSE_RELEASE, will
userspace read stale or zeroed bytes from runtime->dma_area for that
completed period?

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