[PATCH] staging: mt29f_spinand: fix memory leak while programming pages

[PATCH] staging: mt29f_spinand: fix memory leak while programming pages

[Jheng-Jhong Wu]

In spinand_program_page(), it uses devm_kzalloc() to allocate memory to
wbuf dynamically if internal ECC is on, but it doesn't free memory
allocated to wbuf at the end of this function. This leads to a memory leak
issue when internal ECC is on.

Signed-off-by: Jheng-Jhong Wu <goodwater.wu@gmail.com>
---
 drivers/staging/mt29f_spinand/mt29f_spinand.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/mt29f_spinand/mt29f_spinand.c b/drivers/staging/mt29f_spinand/mt29f_spinand.c
index e389009..cf51ca8 100644
--- a/drivers/staging/mt29f_spinand/mt29f_spinand.c
+++ b/drivers/staging/mt29f_spinand/mt29f_spinand.c
@@ -553,6 +553,8 @@ static int spinand_program_page(struct spi_device *spi_nand,
 		}
 		enable_hw_ecc = 0;
 	}
+
+	devm_kfree(&spi_nand->dev, wbuf);
 #endif
 
 	return 0;
-- 
2.7.4

Re: [PATCH] staging: mt29f_spinand: fix memory leak while programming pages

[Dan Carpenter]

devm_ resources are freed automatically when the device is removed.
The name devm_ stands for "device" and "managed".

regards,
dan carpenter

Re: [PATCH] staging: mt29f_spinand: fix memory leak while programming pages

[Jheng-Jhong Wu]

Dear Dan,

I know what you wrote, but before the spinand device is removed and
freed memory automatically, programming pages may do many many times.
Assume we erase and rewrite a large part of the flash, then
spinand_program_page() might exhaust memory if memory is not large
enough.
In fact, OOM indeed occured when I tested programming multi-pages by
mtd_debug tool.
If OOM was not caused by devm_kzalloc() in spinand_program_page(),
what may exhaust memory?

Best Regards,
─────────────────────────
Jheng-Jhong Wu (Victor Wu)
E-mail: goodwater.wu@gmail.com
─────────────────────────

Re: [PATCH] staging: mt29f_spinand: fix memory leak while programming pages

[Dan Carpenter]

On Thu, Aug 02, 2018 at 11:42:30AM +0800, Jheng-Jhong Wu wrote:
> Dear Dan,
> 
> I know what you wrote, but before the spinand device is removed and
> freed memory automatically, programming pages may do many many times.
> Assume we erase and rewrite a large part of the flash, then
> spinand_program_page() might exhaust memory if memory is not large
> enough.
> In fact, OOM indeed occured when I tested programming multi-pages by
> mtd_debug tool.
> If OOM was not caused by devm_kzalloc() in spinand_program_page(),
> what may exhaust memory?
> 

Ok.  That makes sense.  I didn't look at it in context.  You should say
that sort of thing in your changelog.  Looking at it now, the devm_
model isn't right for this function so we should change it to use normal
kzalloc().

We should fix all the error paths as well.  It looks like if this
function starts returning errors, we are probably toasted, but it's
still good practice to avoid slow leaks.

regards,
dan carpenter

[PATCH v2] staging: mt29f_spinand: fix memory leak while programming pages

[Jheng-Jhong Wu]

In spinand_program_page(), it uses devm_kzalloc() to allocate memory to
wbuf dynamically if internal ECC is on, but it doesn't free memory
allocated to wbuf at the end of this function. Before the spinand device
is removed and frees memory automatically, programming pages may run many
times. This leads to a memory leak issue when internal ECC is on.

Changelog:

v2:
- use kzalloc()/kfree() to replace devm_kzalloc()/devm_kfree()
- add some descriptions to commit message

Signed-off-by: Jheng-Jhong Wu <goodwater.wu@gmail.com>
---
 drivers/staging/mt29f_spinand/mt29f_spinand.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/mt29f_spinand/mt29f_spinand.c b/drivers/staging/mt29f_spinand/mt29f_spinand.c
index e389009..d740c76 100644
--- a/drivers/staging/mt29f_spinand/mt29f_spinand.c
+++ b/drivers/staging/mt29f_spinand/mt29f_spinand.c
@@ -492,7 +492,7 @@ static int spinand_program_page(struct spi_device *spi_nand,
 #ifdef CONFIG_MTD_SPINAND_ONDIEECC
 	unsigned int i, j;
 
-	wbuf = devm_kzalloc(&spi_nand->dev, CACHE_BUF, GFP_KERNEL);
+	wbuf = kzalloc(CACHE_BUF, GFP_KERNEL);
 	if (!wbuf)
 		return -ENOMEM;
 
@@ -553,6 +553,8 @@ static int spinand_program_page(struct spi_device *spi_nand,
 		}
 		enable_hw_ecc = 0;
 	}
+
+	kfree(wbuf);
 #endif
 
 	return 0;
-- 
2.7.4

Re: [PATCH v2] staging: mt29f_spinand: fix memory leak while programming pages

[Greg Kroah-Hartman]

On Thu, Aug 02, 2018 at 03:51:30PM +0800, Jheng-Jhong Wu wrote:
> In spinand_program_page(), it uses devm_kzalloc() to allocate memory to
> wbuf dynamically if internal ECC is on, but it doesn't free memory
> allocated to wbuf at the end of this function. Before the spinand device
> is removed and frees memory automatically, programming pages may run many
> times. This leads to a memory leak issue when internal ECC is on.

How is this a memory leak?  The memory will be freed when the struct
device is removed from the system.  How did you test that there was a
leak?

> Changelog:
> 
> v2:
> - use kzalloc()/kfree() to replace devm_kzalloc()/devm_kfree()
> - add some descriptions to commit message

this changelog goes below the --- line.

thanks,

greg k-h

Re: [PATCH v2] staging: mt29f_spinand: fix memory leak while programming pages

[Jheng-Jhong Wu]

Dear greg k-h,

Before device is removed and freed memory automatically, programming
pages may run many many times.
Assume we erase and rewrite a large part of the flash, then
spinand_program_page() might exhaust memory if memory is not large
enough.
We may not remove and re-add the device between each programming page, right?
In fact, OOM indeed occurred when I tested programming multi-pages by
mtd_debug tool.
Erased first, then programmed pages.

Best Regards,
─────────────────────────
Jheng-Jhong Wu (Victor Wu)
E-mail: goodwater.wu@gmail.com
─────────────────────────
Greg Kroah-Hartman <gregkh@linuxfoundation.org> 於 2018年8月2日 週四 下午4:03寫道：
>
> On Thu, Aug 02, 2018 at 03:51:30PM +0800, Jheng-Jhong Wu wrote:
> > In spinand_program_page(), it uses devm_kzalloc() to allocate memory to
> > wbuf dynamically if internal ECC is on, but it doesn't free memory
> > allocated to wbuf at the end of this function. Before the spinand device
> > is removed and frees memory automatically, programming pages may run many
> > times. This leads to a memory leak issue when internal ECC is on.
>
> How is this a memory leak?  The memory will be freed when the struct
> device is removed from the system.  How did you test that there was a
> leak?
>
> > Changelog:
> >
> > v2:
> > - use kzalloc()/kfree() to replace devm_kzalloc()/devm_kfree()
> > - add some descriptions to commit message
>
> this changelog goes below the --- line.
>
> thanks,
>
> greg k-h

Re: [PATCH v2] staging: mt29f_spinand: fix memory leak while programming pages

[Greg KH]

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Thu, Aug 02, 2018 at 04:31:40PM +0800, Jheng-Jhong Wu wrote:
> Dear greg k-h,
> 
> Before device is removed and freed memory automatically, programming
> pages may run many many times.

What do you mean by "programming pages"?

> Assume we erase and rewrite a large part of the flash, then
> spinand_program_page() might exhaust memory if memory is not large
> enough.

I do not understand what you mean here.

> We may not remove and re-add the device between each programming page, right?

I have no idea how your hardware works :)

> In fact, OOM indeed occurred when I tested programming multi-pages by
> mtd_debug tool.
> Erased first, then programmed pages.

Please provide more information like this in the patch when you resend
it, spelling out exactly why you are making this change.  It is not that
the current code is "leaking" anything, it is that you want the
functionality to be different from what it currently is.

thanks,

greg k-h