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