From: Takashi Iwai <tiwai@suse.de>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: alsa-devel@alsa-project.org, Mark Brown <broonie@kernel.org>,
Bard liao <yung-chuan.liao@linux.intel.com>,
Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
Kai Vehmanen <kai.vehmanen@linux.intel.com>
Subject: Re: [alsa-devel] [PATCH] ALSA: pcm: Fix memory leak at closing a stream without hw_free
Date: Thu, 13 Feb 2020 07:17:55 +0100 [thread overview]
Message-ID: <s5hd0ajyprg.wl-tiwai@suse.de> (raw)
In-Reply-To: <1a8475a4-d7d6-7a09-0540-4aa1ceedbe2f@linux.intel.com>
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;
}
--
2.16.4
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
next prev parent reply other threads:[~2020-02-13 6:19 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-29 19:59 [alsa-devel] [PATCH] ALSA: pcm: Fix memory leak at closing a stream without hw_free Takashi Iwai
2020-02-12 23:26 ` Pierre-Louis Bossart
2020-02-13 6:17 ` Takashi Iwai [this message]
2020-02-13 15:24 ` Pierre-Louis Bossart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=s5hd0ajyprg.wl-tiwai@suse.de \
--to=tiwai@suse.de \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=kai.vehmanen@linux.intel.com \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=ranjani.sridharan@linux.intel.com \
--cc=yung-chuan.liao@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).