[PATCH] firmware_loader: fix memory leak for paged buffer

[PATCH] firmware_loader: fix memory leak for paged buffer

[Prateek Sood]

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>
---
 drivers/base/firmware_loader/firmware.h |  2 ++
 drivers/base/firmware_loader/main.c     | 17 +++++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 933e2192..d08efc7 100644
--- 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);
 #else
 static inline void fw_free_paged_buf(struct fw_priv *fw_priv) {}
 static inline int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed) { return -ENXIO; }
 static inline int fw_map_paged_buf(struct fw_priv *fw_priv) { return -ENXIO; }
+static inline bool fw_is_paged_buf(struct fw_priv *fw_priv) { return false; }
 #endif
 
 #endif /* __FIRMWARE_LOADER_H */
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index ca871b1..36bf455 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -252,9 +252,11 @@ 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)
+	if (fw_is_paged_buf(fw_priv))
+		fw_free_paged_buf(fw_priv);
+	else if (!fw_priv->allocated_size)
 		vfree(fw_priv->data);
+
 	kfree_const(fw_priv->fw_name);
 	kfree(fw_priv);
 }
@@ -268,6 +270,11 @@ static void free_fw_priv(struct fw_priv *fw_priv)
 }
 
 #ifdef CONFIG_FW_LOADER_PAGED_BUF
+bool fw_is_paged_buf(struct fw_priv *fw_priv)
+{
+	return fw_priv->is_paged_buf;
+}
+
 void fw_free_paged_buf(struct fw_priv *fw_priv)
 {
 	int i;
@@ -275,6 +282,8 @@ void fw_free_paged_buf(struct fw_priv *fw_priv)
 	if (!fw_priv->pages)
 		return;
 
+	vunmap(fw_priv->data);
+
 	for (i = 0; i < fw_priv->nr_pages; i++)
 		__free_page(fw_priv->pages[i]);
 	kvfree(fw_priv->pages);
@@ -328,10 +337,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
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., 
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] firmware_loader: fix memory leak for paged buffer

[Takashi Iwai]

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

Re: [PATCH] firmware_loader: fix memory leak for paged buffer

[Prateek Sood]

On 8/13/2020 6:28 PM, Takashi Iwai wrote:
> 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 for reviewing.

I would prefer to keep the patch as is to have vmap() and vunmap() pair used

for readability. Will upload a new version with Fixes tag and your 
Reviewed-by.

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

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project

[PATCH v2] firmware_loader: fix memory leak for paged buffer

[Prateek Sood]

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.

Fixes: ddaf29fd9bb6 ("firmware: Free temporary page table after vmapping")
Signed-off-by: Prateek Sood <prsood@codeaurora.org>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_loader/firmware.h |  2 ++
 drivers/base/firmware_loader/main.c     | 17 +++++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 933e2192..d08efc7 100644
--- 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);
 #else
 static inline void fw_free_paged_buf(struct fw_priv *fw_priv) {}
 static inline int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed) { return -ENXIO; }
 static inline int fw_map_paged_buf(struct fw_priv *fw_priv) { return -ENXIO; }
+static inline bool fw_is_paged_buf(struct fw_priv *fw_priv) { return false; }
 #endif
 
 #endif /* __FIRMWARE_LOADER_H */
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index ca871b1..36bf455 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -252,9 +252,11 @@ 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)
+	if (fw_is_paged_buf(fw_priv))
+		fw_free_paged_buf(fw_priv);
+	else if (!fw_priv->allocated_size)
 		vfree(fw_priv->data);
+
 	kfree_const(fw_priv->fw_name);
 	kfree(fw_priv);
 }
@@ -268,6 +270,11 @@ static void free_fw_priv(struct fw_priv *fw_priv)
 }
 
 #ifdef CONFIG_FW_LOADER_PAGED_BUF
+bool fw_is_paged_buf(struct fw_priv *fw_priv)
+{
+	return fw_priv->is_paged_buf;
+}
+
 void fw_free_paged_buf(struct fw_priv *fw_priv)
 {
 	int i;
@@ -275,6 +282,8 @@ void fw_free_paged_buf(struct fw_priv *fw_priv)
 	if (!fw_priv->pages)
 		return;
 
+	vunmap(fw_priv->data);
+
 	for (i = 0; i < fw_priv->nr_pages; i++)
 		__free_page(fw_priv->pages[i]);
 	kvfree(fw_priv->pages);
@@ -328,10 +337,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
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., 
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2] firmware_loader: fix memory leak for paged buffer

[Takashi Iwai]

On Thu, 13 Aug 2020 22:00:22 +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.
> 
> Fixes: ddaf29fd9bb6 ("firmware: Free temporary page table after vmapping")
> Signed-off-by: Prateek Sood <prsood@codeaurora.org>
> Reviewed-by: Takashi Iwai <tiwai@suse.de>

Note that, for including to stable kernels properly, you'd need to put
a Cc-to-stable line around the signed-off-by section in the patch
itself.  OTOH, you don't have to send the patch itself to stable when
you submit it.


thanks,

Takashi

> ---
>  drivers/base/firmware_loader/firmware.h |  2 ++
>  drivers/base/firmware_loader/main.c     | 17 +++++++++++------
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
> index 933e2192..d08efc7 100644
> --- 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);
>  #else
>  static inline void fw_free_paged_buf(struct fw_priv *fw_priv) {}
>  static inline int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed) { return -ENXIO; }
>  static inline int fw_map_paged_buf(struct fw_priv *fw_priv) { return -ENXIO; }
> +static inline bool fw_is_paged_buf(struct fw_priv *fw_priv) { return false; }
>  #endif
>  
>  #endif /* __FIRMWARE_LOADER_H */
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index ca871b1..36bf455 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -252,9 +252,11 @@ 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)
> +	if (fw_is_paged_buf(fw_priv))
> +		fw_free_paged_buf(fw_priv);
> +	else if (!fw_priv->allocated_size)
>  		vfree(fw_priv->data);
> +
>  	kfree_const(fw_priv->fw_name);
>  	kfree(fw_priv);
>  }
> @@ -268,6 +270,11 @@ static void free_fw_priv(struct fw_priv *fw_priv)
>  }
>  
>  #ifdef CONFIG_FW_LOADER_PAGED_BUF
> +bool fw_is_paged_buf(struct fw_priv *fw_priv)
> +{
> +	return fw_priv->is_paged_buf;
> +}
> +
>  void fw_free_paged_buf(struct fw_priv *fw_priv)
>  {
>  	int i;
> @@ -275,6 +282,8 @@ void fw_free_paged_buf(struct fw_priv *fw_priv)
>  	if (!fw_priv->pages)
>  		return;
>  
> +	vunmap(fw_priv->data);
> +
>  	for (i = 0; i < fw_priv->nr_pages; i++)
>  		__free_page(fw_priv->pages[i]);
>  	kvfree(fw_priv->pages);
> @@ -328,10 +337,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
> -- 
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., 
> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

[PATCH v3] firmware_loader: fix memory leak for paged buffer

[Prateek Sood]

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.

Fixes: ddaf29fd9bb6 ("firmware: Free temporary page table after vmapping")
Signed-off-by: Prateek Sood <prsood@codeaurora.org>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Cc: stable@vger.kernel.org
---
 drivers/base/firmware_loader/firmware.h |  2 ++
 drivers/base/firmware_loader/main.c     | 17 +++++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 933e2192..d08efc7 100644
--- 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);
 #else
 static inline void fw_free_paged_buf(struct fw_priv *fw_priv) {}
 static inline int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed) { return -ENXIO; }
 static inline int fw_map_paged_buf(struct fw_priv *fw_priv) { return -ENXIO; }
+static inline bool fw_is_paged_buf(struct fw_priv *fw_priv) { return false; }
 #endif
 
 #endif /* __FIRMWARE_LOADER_H */
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index ca871b1..36bf455 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -252,9 +252,11 @@ 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)
+	if (fw_is_paged_buf(fw_priv))
+		fw_free_paged_buf(fw_priv);
+	else if (!fw_priv->allocated_size)
 		vfree(fw_priv->data);
+
 	kfree_const(fw_priv->fw_name);
 	kfree(fw_priv);
 }
@@ -268,6 +270,11 @@ static void free_fw_priv(struct fw_priv *fw_priv)
 }
 
 #ifdef CONFIG_FW_LOADER_PAGED_BUF
+bool fw_is_paged_buf(struct fw_priv *fw_priv)
+{
+	return fw_priv->is_paged_buf;
+}
+
 void fw_free_paged_buf(struct fw_priv *fw_priv)
 {
 	int i;
@@ -275,6 +282,8 @@ void fw_free_paged_buf(struct fw_priv *fw_priv)
 	if (!fw_priv->pages)
 		return;
 
+	vunmap(fw_priv->data);
+
 	for (i = 0; i < fw_priv->nr_pages; i++)
 		__free_page(fw_priv->pages[i]);
 	kvfree(fw_priv->pages);
@@ -328,10 +337,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
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., 
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3] firmware_loader: fix memory leak for paged buffer

[Takashi Iwai]

On Thu, 20 Aug 2020 22:57:50 +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.
> 
> Fixes: ddaf29fd9bb6 ("firmware: Free temporary page table after vmapping")
> Signed-off-by: Prateek Sood <prsood@codeaurora.org>
> Reviewed-by: Takashi Iwai <tiwai@suse.de>
> Cc: stable@vger.kernel.org

Greg, could you review and merge this one please?


thanks,

Takashi


> ---
>  drivers/base/firmware_loader/firmware.h |  2 ++
>  drivers/base/firmware_loader/main.c     | 17 +++++++++++------
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
> index 933e2192..d08efc7 100644
> --- 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);
>  #else
>  static inline void fw_free_paged_buf(struct fw_priv *fw_priv) {}
>  static inline int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed) { return -ENXIO; }
>  static inline int fw_map_paged_buf(struct fw_priv *fw_priv) { return -ENXIO; }
> +static inline bool fw_is_paged_buf(struct fw_priv *fw_priv) { return false; }
>  #endif
>  
>  #endif /* __FIRMWARE_LOADER_H */
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index ca871b1..36bf455 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -252,9 +252,11 @@ 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)
> +	if (fw_is_paged_buf(fw_priv))
> +		fw_free_paged_buf(fw_priv);
> +	else if (!fw_priv->allocated_size)
>  		vfree(fw_priv->data);
> +
>  	kfree_const(fw_priv->fw_name);
>  	kfree(fw_priv);
>  }
> @@ -268,6 +270,11 @@ static void free_fw_priv(struct fw_priv *fw_priv)
>  }
>  
>  #ifdef CONFIG_FW_LOADER_PAGED_BUF
> +bool fw_is_paged_buf(struct fw_priv *fw_priv)
> +{
> +	return fw_priv->is_paged_buf;
> +}
> +
>  void fw_free_paged_buf(struct fw_priv *fw_priv)
>  {
>  	int i;
> @@ -275,6 +282,8 @@ void fw_free_paged_buf(struct fw_priv *fw_priv)
>  	if (!fw_priv->pages)
>  		return;
>  
> +	vunmap(fw_priv->data);
> +
>  	for (i = 0; i < fw_priv->nr_pages; i++)
>  		__free_page(fw_priv->pages[i]);
>  	kvfree(fw_priv->pages);
> @@ -328,10 +337,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
> -- 
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., 
> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

Re: [PATCH v3] firmware_loader: fix memory leak for paged buffer

[Greg KH]

On Mon, Aug 24, 2020 at 10:19:50AM +0200, Takashi Iwai wrote:
> On Thu, 20 Aug 2020 22:57:50 +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.
> > 
> > Fixes: ddaf29fd9bb6 ("firmware: Free temporary page table after vmapping")
> > Signed-off-by: Prateek Sood <prsood@codeaurora.org>
> > Reviewed-by: Takashi Iwai <tiwai@suse.de>
> > Cc: stable@vger.kernel.org
> 
> Greg, could you review and merge this one please?

It's in my to-review queue, thanks.

greg k-h