Re: [PATCH v10 12/14] ALSA: pcm: add an optional reset operation under the stream lock

[sashiko-bot@kernel.org]

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 <mynameisjune111@gmail.com>

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/20261030052629.27742-1-mynameisjune111@gmail.com?part=12