[alsa-devel] [PATCH] ALSA: pcm: Fix possible inconsistent appl_ptr update via mmap

[Takashi Iwai]

On Mon, 19 Jun 2017 14:10:00 +0200,
Takashi Sakamoto wrote:
> 
> On Jun 19 2017 02:23, Takashi Iwai wrote:
> >> I'll post a patch to take 'snd_pcm_reset()' to use
> >> 'snd_pcm_action_lock_irq()' instead of
> >> 'snd_pcm_action_nonatomic()'. How do you think about it?
> >
> > Conceptually seen, that's not right.  snd_pcm_reset() calls PCM ioctl
> > ops, and this is supposed to be always non-atomic (it's a kind of
> > ioctl wrapper, after all).  So the correct fix would be simply to wrap
> > only the snd_pcm_playback_silence() call with the stream lock.
> 
> I once investigated for this idea. But I was discouraged because of
> several resons:
> 
> 1. There's a semantic contradiction. 'snd_pcm_action_nonatomic()'
> includes atomic operation. The idea perhaps puzzles developers.

Oh no, that's a big misunderstanding.  snd_pcm_action_nonatomic()
doesn't mean to prohibit the critical section protected via a spinlock
inside its execution code.  Instead, it's the code you can't call
*from* spinlocked context.

> 2. In 'snd_pcm_pre_reset()', 'snd_pcm_do_reset()' and
> 'snd_pcm_post_reset()', parameters of runtime for PCM substream are
> also changed or referred. They can be changed in other functions
> concurrently, thus should be locked.

They are protected in terms of the mutex.

> 3. Currently there're a few drivers which implements the local
> ioctl. All of them handle the callback without calling task
> scheduler. It's reasonable against the cost to have an additional
> restraint just for
> context with the reset command.

But the local ioctl *is* always non-atomic, that's the reason I wrote
"conceptually seen".  If you don't want it to be non-atomic, you'd
need to introduce yet another ops for doing that.

> 
> Well, this is a revised version.
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 7e8f3656b695..c74bee099155 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1593,7 +1593,11 @@ static int snd_pcm_pre_reset(struct
> snd_pcm_substream *substream, int state)
>  static int snd_pcm_do_reset(struct snd_pcm_substream *substream, int
> state)
>  {
>         struct snd_pcm_runtime *runtime = substream->runtime;
> -       int err = substream->ops->ioctl(substream,
> SNDRV_PCM_IOCTL1_RESET, NULL);
> +       int err;
> +
> +       snd_pcm_stream_unlock_irq(substream);
> +       err = substream->ops->ioctl(substream, SNDRV_PCM_IOCTL1_RESET,
> NULL);
> +       snd_pcm_stream_lock_irq(substream);
>         if (err < 0)
>                 return err;
>         runtime->hw_ptr_base = 0;
> @@ -1621,7 +1625,7 @@ static const struct action_ops
> snd_pcm_action_reset = {
> 
>  static int snd_pcm_reset(struct snd_pcm_substream *substream)
>  {
> -       return snd_pcm_action_nonatomic(&snd_pcm_action_reset,
> substream, 0);
> +       return snd_pcm_action_lock_irq(&snd_pcm_action_reset, substream, 0);
>  }
> 
>  /*
> 
> There's still a contradiction about atomicity but as a whole better
> than before.

No, this code is just wrong...  The PCM locking doesn't work like
that.

Imagine multiple PCM streams are linked.  Then you'll be screwed up.


thanks,

Takashi