ALSA: pcm: Fix memory leak at closing a stream without hw_free
diff mbox series

Message ID 20200129195907.12197-1-tiwai@suse.de
State New
Headers show
Series
  • ALSA: pcm: Fix memory leak at closing a stream without hw_free
Related show

Commit Message

Takashi Iwai Jan. 29, 2020, 7:59 p.m. UTC
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(-)

Comments

Pierre-Louis Bossart Feb. 12, 2020, 11:26 p.m. UTC | #1
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;
         }
Takashi Iwai Feb. 13, 2020, 6:17 a.m. UTC | #2
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;
 	}
Pierre-Louis Bossart Feb. 13, 2020, 3:24 p.m. UTC | #3
> 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;
>   	}
>

Patch
diff mbox series

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;
 	}