Re: [PATCH v2 2/5] ALSA: virtio: Recover PCM state after a failed start

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] sound: virtio: Data race on vss->stopped due to missing ALSA stream lock in prepare/hw_params
--

commit cf7b0007082a8c0e3a16320e4229781856250174
Author: Joonho Lee <mynameisjune111@gmail.com>
ALSA: virtio: Recover PCM state after a failed start

This commit modifies the capture trigger to disable transfers, mark the stream
as stopped, and arm the stop operating flag if a PCM START fails. It also adds
checks in the hw_params and prepare callbacks to reject retries if the stream
is stopped, until a subsequent sync_stop resolves the state.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index a40ea394bf174..fe8e0dc3eafca 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
[ ... ]
> @@ -224,6 +233,12 @@ static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,
>  			vss->sid);
>  		return -EBADFD;
>  	}
> +	if (vss->stopped) {

[Severity: Medium]
Is it safe to read vss->stopped locklessly here?

ALSA explicitly drops the stream lock (group->mutex) before invoking
virtsnd_pcm_hw_params(), virtsnd_pcm_prepare(), and hw_free() (which calls
virtsnd_pcm_sync_stop()).

Concurrently, ioctls like SNDRV_PCM_IOCTL_FORWARD (which invoke the ack
callback) acquire group->mutex but not buffer_mutex.

This means virtsnd_pcm_pb_ack() can execute concurrently with
virtsnd_pcm_hw_params() or virtsnd_pcm_prepare() on the same substream.

Since the ack path writes to vss->stopped under vss->lock, reading it here
without vss->lock could result in a data race. A torn read or compiler
reordering could result in an incorrect stream state being observed.

> +		dev_err(&vdev->dev, "SID %u: stream release failed\n",
> +			vss->sid);
> +		substream->runtime->stop_operating = true;
> +		return -EBADFD;
> +	}
>  
>  	rc = virtsnd_pcm_dev_set_params(vss, params_buffer_bytes(hw_params),
[ ... ]
> @@ -274,6 +289,13 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
>  	struct virtio_device *vdev = vss->snd->vdev;
>  	struct virtio_snd_msg *msg;
>  
> +	if (vss->stopped) {

[Severity: Medium]
Similarly, vss->stopped is read locklessly here in virtsnd_pcm_prepare().
Could this also race with concurrent ack callbacks for the same reason?

This problem wasn't introduced by this patch, but vss->stopped is also
written locklessly at the end of virtsnd_pcm_sync_stop().

Since virtsnd_pcm_sync_stop() is called from hw_free where ALSA also drops
group->mutex, could that lockless write also race with concurrent ack
callbacks?

> +		dev_err(&vdev->dev, "SID %u: stream release failed\n",
> +			vss->sid);
> +		substream->runtime->stop_operating = true;
> +		return -EBADFD;
> +	}
> +
>  	if (!vss->suspended) {
>  		if (virtsnd_pcm_msg_pending_num(vss)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791877396.git.mynameisjune111@gmail.com?part=2