Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ALSA: virtio: race with sync_stop in virtsnd_pcm_pb_ack() --
commit 75f714a08c45af9f82b5baffff5589166c1182bf Author: Joonho Lee <[email protected]> ALSA: virtio: Propagate PCM send failures through ACK This commit restores message length and notifies the virtqueue on partial send failures in virtsnd_pcm_msg_send(), and propagates those errors through the PCM ACK callbacks as XRUNs while rejecting ACK calls from invalid PCM states. > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index 1091100a4c982..67460bd2a61ca 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c > @@ -466,36 +466,124 @@ virtsnd_pcm_cp_pointer(struct snd_pcm_substream > *substream) [ ... ] > +/* Caller holds vss->lock. */ > +static void virtsnd_pcm_transfer_failed(struct virtio_pcm_substream *vss) > +{ > + vss->xfer_enabled = false; > + vss->stopped = true; > + vss->substream->runtime->stop_operating = true; > +} > > - virtsnd_pcm_msg_send(vss, rec->sw_data, bytes); > +/* The caller holds the PCM lock, also used by DROP and state transitions. */ > +static int virtsnd_pcm_ack_state(struct snd_pcm_substream *substream) > +{ > + switch (substream->runtime->state) { > + case SNDRV_PCM_STATE_XRUN: > + return -EPIPE; > + case SNDRV_PCM_STATE_PREPARED: > + case SNDRV_PCM_STATE_RUNNING: > + case SNDRV_PCM_STATE_PAUSED: > + case SNDRV_PCM_STATE_DRAINING: > + return 0; > + default: > + return -EBADFD; > + } > } > > static int virtsnd_pcm_pb_ack(struct snd_pcm_substream *substream) > { > struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss); > + struct virtsnd_pcm_transfer transfer = {}; > + int rc; > > - guard(spinlock_irqsave)(&queue->lock); > - guard(spinlock)(&vss->lock); > + rc = virtsnd_pcm_ack_state(substream); > + if (rc) > + return rc; > > - return snd_pcm_indirect_playback_transfer(substream, &vss->pcm_indirect, > - virtsnd_pcm_trans_copy); > + scoped_guard(spinlock_irqsave, &queue->lock) { > + guard(spinlock)(&vss->lock); > + transfer.indirect = vss->pcm_indirect; > + if (vss->stopped) { > + rc = -EPIPE; > + } else { > + rc = snd_pcm_indirect_playback_transfer(substream, > + > &transfer.indirect, > + > virtsnd_pcm_trans_copy); > + } > + if (!rc) > + rc = transfer.error; > + if (rc) > + virtsnd_pcm_transfer_failed(vss); [Severity: Medium] As reported by Sashiko on v12 [1], can virtsnd_pcm_pb_ack() and virtsnd_pcm_cp_ack() still race with snd_pcm_sync_stop() -> virtsnd_pcm_sync_stop() on vss->stopped and substream->runtime->stop_operating when a stream is in SNDRV_PCM_STATE_PREPARED or SNDRV_PCM_STATE_DRAINING? Because virtio-snd sets vpcm->pcm->nonatomic = true, snd_pcm_prepare() and snd_pcm_hw_params() hold runtime->buffer_mutex rather than the PCM stream lock across snd_pcm_sync_stop() and substream->ops->prepare() / substream->ops->hw_params(). Meanwhile, a concurrent SNDRV_PCM_IOCTL_SYNC_PTR in snd_pcm_sync_ptr() only acquires the PCM stream lock and calls pcm_lib_apply_appl_ptr() -> virtsnd_pcm_pb_ack() / virtsnd_pcm_cp_ack(). In virtsnd_pcm_ack_state(), SNDRV_PCM_STATE_SETUP is rejected, which covers snd_pcm_drop(), but SNDRV_PCM_STATE_PREPARED and SNDRV_PCM_STATE_DRAINING are allowed: 1. When a capture stream with available frames is drained via snd_pcm_drain(), snd_pcm_do_drain_init() stops the stream and places it in SNDRV_PCM_STATE_DRAINING with vss->stopped = true and runtime->stop_operating = true. Similarly, if snd_pcm_start() fails on a later linked stream, snd_pcm_undo_start() stops the earlier stream while leaving it in SNDRV_PCM_STATE_PREPARED with vss->stopped = true and runtime->stop_operating = true. On a draining capture stream, any read or SNDRV_PCM_IOCTL_SYNC_PTR will also see vss->stopped == true in virtsnd_pcm_cp_ack() and fail with -EPIPE. 2. When snd_pcm_prepare() or snd_pcm_hw_params() is called from SNDRV_PCM_STATE_PREPARED (or snd_pcm_prepare() from SNDRV_PCM_STATE_DRAINING on capture), runtime->state is unchanged while snd_pcm_sync_stop() clears runtime->stop_operating = false and calls virtsnd_pcm_sync_stop(): sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_sync_stop() { if (!vss->stopped) return 0; ... vss->stopped = false; } While virtsnd_pcm_sync_stop() is sleeping in cancel_work_sync(), VIRTIO_SND_R_PCM_RELEASE, or wait_event_interruptible_timeout(), a concurrent SNDRV_PCM_IOCTL_SYNC_PTR passes virtsnd_pcm_ack_state(), sees vss->stopped == true, calls virtsnd_pcm_transfer_failed(vss) to re-arm vss->stopped = true and runtime->stop_operating = true, and returns -EPIPE so pcm_lib_apply_appl_ptr() triggers __snd_pcm_xrun(). Conversely, if SNDRV_PCM_IOCTL_SYNC_PTR runs right after virtsnd_pcm_sync_stop() clears vss->stopped = false and before prepare finishes, ACK sees vss->stopped == false and enqueues I/O messages on the released stream. 3. Later commits in this series ("ALSA: virtio: Recover PCM state after a failed start" and "ALSA: virtio: add staged capture and serialize PCM lifecycle accounting") add vss->lock around vss->stopped and gate ACK on vss->prepared and vss->preparing, but virtsnd_pcm_prepare_locked() and virtsnd_pcm_hw_params() still call virtsnd_pcm_sync_stop_locked() before setting vss->preparing = true or clearing vss->prepared = false. While virtsnd_pcm_sync_stop_locked() or virtsnd_pcm_pause_restart_sync_stop() sleeps in disable_work_sync(), cancel_work_sync(), virtsnd_pcm_stop_device(), or virtsnd_pcm_send_sync() for VIRTIO_SND_R_PCM_RELEASE, runtime->state remains SNDRV_PCM_STATE_PREPARED, vss->preparing is false, and vss->prepared remains true. A concurrent SNDRV_PCM_IOCTL_SYNC_PTR can still enqueue messages during disable_work_sync() or observe vss->stopped == true during STOP/RELEASE, re-arm runtime->stop_operating = true via virtsnd_pcm_transfer_failed(), and return -EPIPE to trigger __snd_pcm_xrun(). [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
