On Sun, 18 Jun 2017 12:49:24 +0200, Takashi Sakamoto wrote: > > Hi, > > On Jun 17 2017 05:40, Takashi Iwai wrote: > > The ALSA PCM core refers to the appl_ptr value stored on the mmapped > > page that is shared between kernel and user-space. Although the > > reference is performed in the PCM stream lock, it doesn't guarantee > > the atomic access when the value gets updated concurrently from the > > user-space on another CPU. > > > > In most of codes, this is no big problem, but still there are a few > > places that may result in slight inconsistencies because they access > > runtime->control->appl_ptr multiple times; that is, the second read > > might be a different value from the first value. It can be even > > backward or jumping, as we have no control for it. Hence, the > > calculation may give an unexpected value. Luckily, there is no > > security vulnerability by that, as far as I've checked. But still we > > should address it. > > > > This patch tries to reduce such possible cases. The fix is simple -- > > we just read once, store it to a local variable and use it for the > > rest calculations. > > > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > > --- > > sound/core/pcm_lib.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > This looks good to me by itself. > > Reviewed-by: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx> > > This function, 'snd_pcm_playback_silence()' can be called without > acquiring stream lock by 'snd_pcm_reset()' when runtime of PCM > substream is configured to have a larger value than zero to its > 'silence_size' member. When a task or any IRQ handler operates the > runtime and another task operates the reset in the same time, both of > them got expected result. Good catch. The whole snd_pcm_reset() behavior is a bit whacky. > 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. thanks, Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
