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