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 <[email protected]> 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/[email protected]?part=2
