From: Takashi Iwai <tiwai@suse.de>
To: Prateek Sood <prsood@codeaurora.org>
Cc: mcgrof@kernel.org, gregkh@linuxfoundation.org, rafael@kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] firmware_loader: fix memory leak for paged buffer
Date: Thu, 13 Aug 2020 14:58:36 +0200 [thread overview]
Message-ID: <s5ha6yyvh1f.wl-tiwai@suse.de> (raw)
In-Reply-To: <1597258819-14080-1-git-send-email-prsood@codeaurora.org>
On Wed, 12 Aug 2020 21:00:19 +0200,
Prateek Sood wrote:
>
> vfree() is being called on paged buffer allocated
> using alloc_page() and mapped using vmap().
>
> Freeing of pages in vfree() relies on nr_pages of
> struct vm_struct. vmap() does not update nr_pages.
> It can lead to memory leaks.
>
> Signed-off-by: Prateek Sood <prsood@codeaurora.org>
Thanks for spotting this out! This is essentially a revert of the
commit ddaf29fd9bb6 ("firmware: Free temporary page table after
vmapping"), so better to mention it via Fixes tag as well as Cc to
stable.
About the changes:
> --- a/drivers/base/firmware_loader/firmware.h
> +++ b/drivers/base/firmware_loader/firmware.h
> @@ -142,10 +142,12 @@ static inline void fw_state_done(struct fw_priv *fw_priv)
> void fw_free_paged_buf(struct fw_priv *fw_priv);
> int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed);
> int fw_map_paged_buf(struct fw_priv *fw_priv);
> +bool fw_is_paged_buf(struct fw_priv *fw_priv);
I guess this isn't necessary if we just swap the call order of
fw_free_paged_buf() and vfree(); then fw_priv->is_paged_buf is
referred only in fw_free_paged_buf().
That is, something like below.
In anyway, take my review tag:
Reviewed-by: Takashi Iwai <tiwai@suse.de>
thanks,
Takashi
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -252,9 +252,9 @@ static void __free_fw_priv(struct kref *ref)
list_del(&fw_priv->list);
spin_unlock(&fwc->lock);
- fw_free_paged_buf(fw_priv); /* free leftover pages */
if (!fw_priv->allocated_size)
vfree(fw_priv->data);
+ fw_free_paged_buf(fw_priv); /* free leftover pages */
kfree_const(fw_priv->fw_name);
kfree(fw_priv);
}
@@ -272,7 +272,7 @@ void fw_free_paged_buf(struct fw_priv *fw_priv)
{
int i;
- if (!fw_priv->pages)
+ if (!fw_priv->is_paged_buf)
return;
for (i = 0; i < fw_priv->nr_pages; i++)
@@ -328,10 +328,6 @@ int fw_map_paged_buf(struct fw_priv *fw_priv)
if (!fw_priv->data)
return -ENOMEM;
- /* page table is no longer needed after mapping, let's free */
- kvfree(fw_priv->pages);
- fw_priv->pages = NULL;
-
return 0;
}
#endif
next prev parent reply other threads:[~2020-08-13 12:58 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-12 19:00 [PATCH] firmware_loader: fix memory leak for paged buffer Prateek Sood
2020-08-13 12:58 ` Takashi Iwai [this message]
2020-08-13 19:51 ` Prateek Sood
2020-08-13 20:00 ` [PATCH v2] " Prateek Sood
2020-08-17 12:12 ` Takashi Iwai
2020-08-20 20:57 ` [PATCH v3] " Prateek Sood
2020-08-24 8:19 ` Takashi Iwai
2020-08-24 8:21 ` Greg KH
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=s5ha6yyvh1f.wl-tiwai@suse.de \
--to=tiwai@suse.de \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=prsood@codeaurora.org \
--cc=rafael@kernel.org \
/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).