Message ID | 20200129195907.12197-1-tiwai@suse.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ALSA: pcm: Fix memory leak at closing a stream without hw_free | expand |
On 1/29/20 1:59 PM, Takashi Iwai wrote: > ALSA PCM core recently introduced a new managed PCM buffer allocation > mode that does allocate / free automatically at hw_params and > hw_free. However, it overlooked the code path directly calling > hw_free PCM ops at releasing the PCM substream, and it may result in a > memory leak as spotted by syzkaller when no buffer preallocation is > used (e.g. vmalloc buffer). > > This patch papers over it with a slight refactoring. The hw_free ops > call and relevant tasks are unified in a new helper function, and call > it from both places. > > Fixes: 0dba808eae26 ("ALSA: pcm: Introduce managed buffer allocation mode") > Reported-by: syzbot+30edd0f34bfcdc548ac4@syzkaller.appspotmail.com > Cc: <stable@vger.kernel.org> > Signed-off-by: Takashi Iwai <tiwai@suse.de> Takashi, this patch introduces a regression for our SoundWire work - credits to Bard Liao for reporting this the first. We see the hw_free() being called twice and as a result the SoundWire stream state becomes inconsistent, with some memory becoming corrupted: [ 107.864109] sof-audio-pci 0000:00:1f.3: pcm: free stream 0 dir 0 [ 107.864324] sof-audio-pci 0000:00:1f.3: ipc tx: 0x80010000: GLB_DAI_MSG: CONFIG [ 107.864507] sof-audio-pci 0000:00:1f.3: ipc tx succeeded: 0x80010000: GLB_DAI_MSG: CONFIG [ 107.864615] sof-audio-pci 0000:00:1f.3: pcm: free stream 0 dir 0 [ 107.864627] sdw_deprepare_stream: \xc0Pjf\xe0\xa3\xff\xff: inconsistent state state 6 [ 107.864640] int-sdw int-sdw.0: sdw_deprepare_stream: failed -22 we detected this while merging your latest code as part of our weekly rebase, then realized the error was already present in v5.6-rc1 and continued to narrow the scope to sound-fix-5.6-rc1 and this specific patch. I can't claim to fully understand the code in this patch, but I am not sure why hw_free() ends up being unconditionally called at [1] below > --- > sound/core/pcm_native.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index bb23f5066654..4ac42ee1238c 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -786,10 +786,22 @@ static int snd_pcm_hw_params_user(struct snd_pcm_substream *substream, > return err; > } > > +static int do_hw_free(struct snd_pcm_substream *substream) > +{ > + int result = 0; > + > + snd_pcm_sync_stop(substream); > + if (substream->ops->hw_free) > + result = substream->ops->hw_free(substream); > + if (substream->managed_buffer_alloc) > + snd_pcm_lib_free_pages(substream); > + return result; > +} > + > static int snd_pcm_hw_free(struct snd_pcm_substream *substream) > { > struct snd_pcm_runtime *runtime; > - int result = 0; > + int result; > > if (PCM_RUNTIME_CHECK(substream)) > return -ENXIO; > @@ -806,11 +818,7 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream) > snd_pcm_stream_unlock_irq(substream); > if (atomic_read(&substream->mmap_count)) > return -EBADFD; > - snd_pcm_sync_stop(substream); > - if (substream->ops->hw_free) > - result = substream->ops->hw_free(substream); > - if (substream->managed_buffer_alloc) > - snd_pcm_lib_free_pages(substream); > + result = do_hw_free(substream); > snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN); > pm_qos_remove_request(&substream->latency_pm_qos_req); > return result; > @@ -2529,9 +2537,7 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream) > > snd_pcm_drop(substream); > if (substream->hw_opened) { > - if (substream->ops->hw_free && > - substream->runtime->status->state != SNDRV_PCM_STATE_OPEN) > - substream->ops->hw_free(substream); > + do_hw_free(substream); [1] don't we need to only do the hw_free() when substream->runtime->status->state != SNDRV_PCM_STATE_OPEN e.g. with the following patch? Or is the expectation that the hw_free() callback be implemented so that only the first call has an effect? Thanks -Pierre diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 336406bcb59e..051a1f234975 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -787,12 +787,12 @@ static int snd_pcm_hw_params_user(struct snd_pcm_substream *substream, return err; } -static int do_hw_free(struct snd_pcm_substream *substream) +static int do_hw_free(struct snd_pcm_substream *substream, bool cond_free) { int result = 0; snd_pcm_sync_stop(substream); - if (substream->ops->hw_free) + if (cond_free && substream->ops->hw_free) result = substream->ops->hw_free(substream); if (substream->managed_buffer_alloc) snd_pcm_lib_free_pages(substream); @@ -819,7 +819,7 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream) snd_pcm_stream_unlock_irq(substream); if (atomic_read(&substream->mmap_count)) return -EBADFD; - result = do_hw_free(substream); + result = do_hw_free(substream, true); snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN); pm_qos_remove_request(&substream->latency_pm_qos_req); return result; @@ -2594,7 +2594,9 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream) snd_pcm_drop(substream); if (substream->hw_opened) { - do_hw_free(substream); + do_hw_free(substream, + substream->runtime->status->state != + SNDRV_PCM_STATE_OPEN); substream->ops->close(substream); substream->hw_opened = 0; }
On Thu, 13 Feb 2020 00:26:44 +0100, Pierre-Louis Bossart wrote: > > > > On 1/29/20 1:59 PM, Takashi Iwai wrote: > > ALSA PCM core recently introduced a new managed PCM buffer allocation > > mode that does allocate / free automatically at hw_params and > > hw_free. However, it overlooked the code path directly calling > > hw_free PCM ops at releasing the PCM substream, and it may result in a > > memory leak as spotted by syzkaller when no buffer preallocation is > > used (e.g. vmalloc buffer). > > > > This patch papers over it with a slight refactoring. The hw_free ops > > call and relevant tasks are unified in a new helper function, and call > > it from both places. > > > > Fixes: 0dba808eae26 ("ALSA: pcm: Introduce managed buffer allocation mode") > > Reported-by: syzbot+30edd0f34bfcdc548ac4@syzkaller.appspotmail.com > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > Takashi, this patch introduces a regression for our SoundWire work - > credits to Bard Liao for reporting this the first. > > We see the hw_free() being called twice and as a result the SoundWire > stream state becomes inconsistent, with some memory becoming > corrupted: > > [ 107.864109] sof-audio-pci 0000:00:1f.3: pcm: free stream 0 dir 0 > [ 107.864324] sof-audio-pci 0000:00:1f.3: ipc tx: 0x80010000: > GLB_DAI_MSG: CONFIG > [ 107.864507] sof-audio-pci 0000:00:1f.3: ipc tx succeeded: > 0x80010000: GLB_DAI_MSG: CONFIG > [ 107.864615] sof-audio-pci 0000:00:1f.3: pcm: free stream 0 dir 0 > [ 107.864627] sdw_deprepare_stream: \xc0Pjf\xe0\xa3\xff\xff: > inconsistent state state 6 > [ 107.864640] int-sdw int-sdw.0: sdw_deprepare_stream: failed -22 > > we detected this while merging your latest code as part of our weekly > rebase, then realized the error was already present in v5.6-rc1 and > continued to narrow the scope to sound-fix-5.6-rc1 and this specific > patch. > > I can't claim to fully understand the code in this patch, but I am not > sure why hw_free() ends up being unconditionally called at [1] below > > > --- > > sound/core/pcm_native.c | 24 +++++++++++++++--------- > > 1 file changed, 15 insertions(+), 9 deletions(-) > > > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > > index bb23f5066654..4ac42ee1238c 100644 > > --- a/sound/core/pcm_native.c > > +++ b/sound/core/pcm_native.c > > @@ -786,10 +786,22 @@ static int snd_pcm_hw_params_user(struct snd_pcm_substream *substream, > > return err; > > } > > +static int do_hw_free(struct snd_pcm_substream *substream) > > +{ > > + int result = 0; > > + > > + snd_pcm_sync_stop(substream); > > + if (substream->ops->hw_free) > > + result = substream->ops->hw_free(substream); > > + if (substream->managed_buffer_alloc) > > + snd_pcm_lib_free_pages(substream); > > + return result; > > +} > > + > > static int snd_pcm_hw_free(struct snd_pcm_substream *substream) > > { > > struct snd_pcm_runtime *runtime; > > - int result = 0; > > + int result; > > if (PCM_RUNTIME_CHECK(substream)) > > return -ENXIO; > > @@ -806,11 +818,7 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream) > > snd_pcm_stream_unlock_irq(substream); > > if (atomic_read(&substream->mmap_count)) > > return -EBADFD; > > - snd_pcm_sync_stop(substream); > > - if (substream->ops->hw_free) > > - result = substream->ops->hw_free(substream); > > - if (substream->managed_buffer_alloc) > > - snd_pcm_lib_free_pages(substream); > > + result = do_hw_free(substream); > > snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN); > > pm_qos_remove_request(&substream->latency_pm_qos_req); > > return result; > > @@ -2529,9 +2537,7 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream) > > snd_pcm_drop(substream); > > if (substream->hw_opened) { > > - if (substream->ops->hw_free && > > - substream->runtime->status->state != SNDRV_PCM_STATE_OPEN) > > - substream->ops->hw_free(substream); > > + do_hw_free(substream); > > [1] don't we need to only do the hw_free() when > > substream->runtime->status->state != SNDRV_PCM_STATE_OPEN > > e.g. with the following patch? Yes, my bad, it's just an oversight. But the condition check should be applied to the whole do_hw_free() call, so it can be simpler like the patch below. > Or is the expectation that the hw_free() callback be implemented so > that only the first call has an effect? This is another solution, but let's follow to the original code that had the condition check at the caller side. thanks, Takashi -- 8< -- From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH] ALSA: pcm: Fix double hw_free calls The commit 66f2d19f8116 ("ALSA: pcm: Fix memory leak at closing a stream without hw_free") tried to fix the regression wrt the missing hw_free call at closing without SNDRV_PCM_IOCTL_HW_FREE ioctl. However, the code change dropped mistakenly the state check, resulting in calling hw_free twice when SNDRV_PCM_IOCTL_HW_FRE got called beforehand. For most drivers, this is almost harmless, but the drivers like SOF show another regression now. This patch adds the state condition check before calling do_hw_free() at releasing the stream for avoiding the double hw_free calls. Fixes: 66f2d19f8116 ("ALSA: pcm: Fix memory leak at closing a stream without hw_free") Reported-by: Bard Liao <yung-chuan.liao@linux.intel.com> Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/core/pcm_native.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 336406bcb59e..d5443eeb8b63 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2594,7 +2594,8 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream) snd_pcm_drop(substream); if (substream->hw_opened) { - do_hw_free(substream); + if (substream->runtime->status->state != SNDRV_PCM_STATE_OPEN) + do_hw_free(substream); substream->ops->close(substream); substream->hw_opened = 0; }
> From: Takashi Iwai <tiwai@suse.de> > Subject: [PATCH] ALSA: pcm: Fix double hw_free calls > > The commit 66f2d19f8116 ("ALSA: pcm: Fix memory leak at closing a > stream without hw_free") tried to fix the regression wrt the missing > hw_free call at closing without SNDRV_PCM_IOCTL_HW_FREE ioctl. > However, the code change dropped mistakenly the state check, resulting > in calling hw_free twice when SNDRV_PCM_IOCTL_HW_FRE got called > beforehand. For most drivers, this is almost harmless, but the > drivers like SOF show another regression now. > > This patch adds the state condition check before calling do_hw_free() > at releasing the stream for avoiding the double hw_free calls. > > Fixes: 66f2d19f8116 ("ALSA: pcm: Fix memory leak at closing a stream without hw_free") > Reported-by: Bard Liao <yung-chuan.liao@linux.intel.com> > Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Cc: <stable@vger.kernel.org> > Signed-off-by: Takashi Iwai <tiwai@suse.de> Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Thanks Takashi! > --- > sound/core/pcm_native.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index 336406bcb59e..d5443eeb8b63 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -2594,7 +2594,8 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream) > > snd_pcm_drop(substream); > if (substream->hw_opened) { > - do_hw_free(substream); > + if (substream->runtime->status->state != SNDRV_PCM_STATE_OPEN) > + do_hw_free(substream); > substream->ops->close(substream); > substream->hw_opened = 0; > } >
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index bb23f5066654..4ac42ee1238c 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -786,10 +786,22 @@ static int snd_pcm_hw_params_user(struct snd_pcm_substream *substream, return err; } +static int do_hw_free(struct snd_pcm_substream *substream) +{ + int result = 0; + + snd_pcm_sync_stop(substream); + if (substream->ops->hw_free) + result = substream->ops->hw_free(substream); + if (substream->managed_buffer_alloc) + snd_pcm_lib_free_pages(substream); + return result; +} + static int snd_pcm_hw_free(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime; - int result = 0; + int result; if (PCM_RUNTIME_CHECK(substream)) return -ENXIO; @@ -806,11 +818,7 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream) snd_pcm_stream_unlock_irq(substream); if (atomic_read(&substream->mmap_count)) return -EBADFD; - snd_pcm_sync_stop(substream); - if (substream->ops->hw_free) - result = substream->ops->hw_free(substream); - if (substream->managed_buffer_alloc) - snd_pcm_lib_free_pages(substream); + result = do_hw_free(substream); snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN); pm_qos_remove_request(&substream->latency_pm_qos_req); return result; @@ -2529,9 +2537,7 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream) snd_pcm_drop(substream); if (substream->hw_opened) { - if (substream->ops->hw_free && - substream->runtime->status->state != SNDRV_PCM_STATE_OPEN) - substream->ops->hw_free(substream); + do_hw_free(substream); substream->ops->close(substream); substream->hw_opened = 0; }
ALSA PCM core recently introduced a new managed PCM buffer allocation mode that does allocate / free automatically at hw_params and hw_free. However, it overlooked the code path directly calling hw_free PCM ops at releasing the PCM substream, and it may result in a memory leak as spotted by syzkaller when no buffer preallocation is used (e.g. vmalloc buffer). This patch papers over it with a slight refactoring. The hw_free ops call and relevant tasks are unified in a new helper function, and call it from both places. Fixes: 0dba808eae26 ("ALSA: pcm: Introduce managed buffer allocation mode") Reported-by: syzbot+30edd0f34bfcdc548ac4@syzkaller.appspotmail.com Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/core/pcm_native.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)