Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ALSA: pcm: unlocked appl_ptr overwrite in snd_pcm_post_prepare() with reset op --
commit 963c7726f3bbfa33b12ff37026f791cf2e45509e Author: Joonho Lee <[email protected]> ALSA: pcm: add an optional reset operation under the stream lock This commit adds an optional reset callback to struct snd_pcm_ops that runs under the substream's PCM stream lock, and exposes snd_pcm_lib_reset() for drivers to retain the common pointer update logic. > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index a2ea44c3c1d29..41c691512f54c 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -1988,7 +1988,31 @@ static int snd_pcm_do_reset(struct snd_pcm_substream > *substream, > snd_pcm_state_t state) > { > struct snd_pcm_runtime *runtime = substream->runtime; > - int err = snd_pcm_ops_ioctl(substream, SNDRV_PCM_IOCTL1_RESET, NULL); > + int err; > + > + if (substream->ops->reset) { > + /* > + * Complete this member before releasing its PCM lock. A later > + * linked member may fail; RESET has no group rollback > operation. > + */ > + guard(pcm_stream_lock_irq)(substream); > + > + err = substream->ops->reset(substream); > + if (err < 0) > + return err; > + runtime->hw_ptr_base = 0; > + runtime->hw_ptr_interrupt = runtime->status->hw_ptr - > + runtime->status->hw_ptr % runtime->period_size; > + runtime->silence_start = runtime->status->hw_ptr; > + runtime->silence_filled = 0; > + runtime->control->appl_ptr = runtime->status->hw_ptr; > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && > + runtime->silence_size > 0) > + snd_pcm_playback_silence(substream, ULONG_MAX); > + return 0; > + } > + > + err = snd_pcm_ops_ioctl(substream, SNDRV_PCM_IOCTL1_RESET, NULL); > if (err < 0) > return err; > guard(pcm_stream_lock_irq)(substream); > @@ -2004,6 +2028,9 @@ static void snd_pcm_post_reset(struct snd_pcm_substream > *substream, > snd_pcm_state_t state) > { > struct snd_pcm_runtime *runtime = substream->runtime; > + > + if (substream->ops->reset) > + return; [Severity: Medium] Does snd_pcm_post_prepare() also need to account for substream->ops->reset? In sound/core/pcm_native.c, the prepare path runs: snd_pcm_prepare() -> snd_pcm_action_nonatomic(&snd_pcm_action_prepare, ...) -> snd_pcm_do_prepare() -> snd_pcm_do_reset() -> snd_pcm_post_prepare() When substream->ops->reset is implemented, snd_pcm_do_reset() rebases runtime->control->appl_ptr = runtime->status->hw_ptr under guard(pcm_stream_lock_irq)(substream) and then releases the lock. While snd_pcm_post_reset() skips the redundant assignment when substream->ops->reset is set, snd_pcm_post_prepare() was not updated: static void snd_pcm_post_prepare(struct snd_pcm_substream *substream, snd_pcm_state_t state) { struct snd_pcm_runtime *runtime = substream->runtime; runtime->control->appl_ptr = runtime->status->hw_ptr; snd_pcm_set_state(substream, SNDRV_PCM_STATE_PREPARED); } Between snd_pcm_do_reset() releasing pcm_stream_lock_irq and snd_pcm_post_prepare() running (for example, while subsequent linked group members execute snd_pcm_do_prepare()), a concurrent SNDRV_PCM_IOCTL_SYNC_PTR in snd_pcm_sync_ptr() or a transfer/rewind/forward on another linked member that was already in SNDRV_PCM_STATE_PREPARED can take pcm_stream_lock_irq and update runtime->control->appl_ptr via pcm_lib_apply_appl_ptr(). Afterward, snd_pcm_post_prepare() overwrites runtime->control->appl_ptr without holding pcm_stream_lock_irq, which can desynchronize runtime->control->appl_ptr from driver state. In addition, because snd_pcm_do_prepare() calls snd_pcm_do_reset(), calling snd_pcm_playback_silence(substream, ULONG_MAX) inside snd_pcm_do_reset() when substream->ops->reset is set causes prepare on a playback stream with runtime->silence_size > 0 to fill silence during do_prepare before the stream transitions to SNDRV_PCM_STATE_PREPARED, whereas the legacy path only runs snd_pcm_playback_silence() in snd_pcm_post_reset() and snd_pcm_post_start(). Should snd_pcm_post_prepare() skip the appl_ptr assignment when substream->ops->reset is set, and should snd_pcm_do_reset() avoid calling snd_pcm_playback_silence() when invoked from snd_pcm_do_prepare()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
