firmware_loader: fix memory leak for paged buffer
Subsystems (3)
PATCH 1/1 firmware_loader: fix memory leak for paged buffer
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.
--- drivers/base/firmware_loader/firmware.h | 2 ++ drivers/base/firmware_loader/main.c | 17 +++++++++++------ 2 files changed, 13 insertions(+), 6 deletions(-)
drivers/base/firmware_loader/firmware.h +2 -0
@@ -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 */
drivers/base/firmware_loader/main.c +11 -6
@@ -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
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 9 lines · Prateek Sood
vfree()is being called on paged buffer allocated usingalloc_page()and mapped usingvmap().Freeing of pages in
vfree()relies onnr_pagesof structvm_struct.vmap()does not updatenr_pages. It can lead to memory leaks.Signed-off-by Prateek Sood <[email protected]>
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:
drivers/base/firmware_loader/firmware.h static inline void fw_state_done(struct fw_priv *fw_priv) · line 145
Quoted 7 lines
drivers/base/firmware_loader/firmware.h +1
-0@@ -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:
thanks,
Takashi
drivers/base/firmware_loader/main.c +2 -6
@@ -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
Quoted 27 lines · Takashi Iwai
Quoted 8 lines · Prateek Sood
vfree()is being called on paged buffer allocated usingalloc_page()and mapped usingvmap().Freeing of pages in
vfree()relies onnr_pagesof structvm_struct.vmap()does not updatenr_pages. It can lead to memory leaks.Signed-off-by Prateek Sood <[email protected]>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:drivers/base/firmware_loader/firmware.h static inline void fw_state_done(struct fw_priv *fw_priv) · line 145
Quoted 7 lines
drivers/base/firmware_loader/firmware.h +1
-0@@ -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()andvfree(); thenfw_priv->is_paged_bufis referred only infw_free_paged_buf().
That is, something like below.
Quoted 2 lines
In anyway, take my review tag:
Reviewed-by Takashi Iwai <[email protected]>
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.
Quoted 38 lines
thanks,
Takashi
drivers/base/firmware_loader/main.c +2
-6@@ -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
Signature · 3 lines
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 1/1 firmware_loader: fix memory leak for paged buffer
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.
--- drivers/base/firmware_loader/firmware.h | 2 ++ drivers/base/firmware_loader/main.c | 17 +++++++++++------ 2 files changed, 13 insertions(+), 6 deletions(-)
drivers/base/firmware_loader/firmware.h +2 -0
@@ -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 */
drivers/base/firmware_loader/main.c +11 -6
@@ -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
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 11 lines · Prateek Sood
vfree()is being called on paged buffer allocated
usingalloc_page()and mapped usingvmap().Freeing of pages in
vfree()relies onnr_pagesof structvm_struct.vmap()does not updatenr_pages. It can lead to memory leaks.Fixes ddaf29fd9bb6 ("firmware: Free temporary page table after vmapping")Signed-off-by Prateek Sood <[email protected]>Reviewed-by Takashi Iwai <[email protected]>
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
Quoted 76 lines
--- drivers/base/firmware_loader/firmware.h | 2 ++ drivers/base/firmware_loader/main.c | 17 +++++++++++------ 2 files changed, 13 insertions(+), 6 deletions(-)drivers/base/firmware_loader/firmware.h +2
-0@@ -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 */ drivers/base/firmware_loader/main.c +11
-6@@ -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 Signature · 2 lines
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 1/1 firmware_loader: fix memory leak for paged buffer
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.
--- drivers/base/firmware_loader/firmware.h | 2 ++ drivers/base/firmware_loader/main.c | 17 +++++++++++------ 2 files changed, 13 insertions(+), 6 deletions(-)
drivers/base/firmware_loader/firmware.h +2 -0
@@ -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 */
drivers/base/firmware_loader/main.c +11 -6
@@ -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
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 4 lines · Prateek Sood
vfree()is being called on paged buffer allocated
usingalloc_page()and mapped usingvmap().
Quoted 8 lines
Freeing of pages in
vfree()relies onnr_pagesof structvm_struct.vmap()does not updatenr_pages. It can lead to memory leaks.Fixes ddaf29fd9bb6 ("firmware: Free temporary page table after vmapping")Signed-off-by Prateek Sood <[email protected]>Reviewed-by Takashi Iwai <[email protected]>
Greg, could you review and merge this one please?
thanks,
Takashi
Quoted 76 lines
--- drivers/base/firmware_loader/firmware.h | 2 ++ drivers/base/firmware_loader/main.c | 17 +++++++++++------ 2 files changed, 13 insertions(+), 6 deletions(-)drivers/base/firmware_loader/firmware.h +2
-0@@ -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 */ drivers/base/firmware_loader/main.c +11
-6@@ -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 Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Greg KH
maintainerQuoted 10 lines · Takashi Iwai
Quoted 8 lines · Prateek Sood
vfree()is being called on paged buffer allocated
usingalloc_page()and mapped usingvmap().Freeing of pages in
vfree()relies onnr_pagesof structvm_struct.vmap()does not updatenr_pages. It can lead to memory leaks.
Quoted 6 lines
Quoted 4 lines
Fixes ddaf29fd9bb6 ("firmware: Free temporary page table after vmapping")Signed-off-by Prateek Sood <[email protected]>Reviewed-by Takashi Iwai <[email protected]>Greg, could you review and merge this one please?
It's in my to-review queue, thanks.
greg k-h