[PATCH] jffs2: fix memory leak if the sector was successfully erased

[PATCH] jffs2: fix memory leak if the sector was successfully erased

[Ni@m]

Memory allocated for erase instruction is not freed if the sector was
successfully erased.

Signed-off-by: Dmytro Milinevskyy <niam.niam@gmail.com>
---
 fs/jffs2/erase.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
index b47679b..c0a5604 100644
--- a/fs/jffs2/erase.c
+++ b/fs/jffs2/erase.c
@@ -74,8 +74,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
        ((struct erase_priv_struct *)instr->priv)->c = c;

        ret = c->mtd->erase(c->mtd, instr);
-       if (!ret)
+       if (!ret) {
+        kfree(instr);
                return;
+    }

        bad_offset = instr->fail_addr;
        kfree(instr);

[PATCH] jffs2: fix memory leak if the sector was successfully erased

[Russell King - ARM Linux]

On Mon, Feb 22, 2010 at 06:26:22PM +0200, Ni at m wrote:
> Memory allocated for erase instruction is not freed if the sector was
> successfully erased.

I can not help you.  Please direct your message to the jffs2 developers.

[PATCH] jffs2: fix memory leak if the sector was successfully erased

[Daniel Mack]

On Mon, Feb 22, 2010 at 06:26:22PM +0200, Ni at m wrote:
> Memory allocated for erase instruction is not freed if the sector was
> successfully erased.
> 
> Signed-off-by: Dmytro Milinevskyy <niam.niam@gmail.com>

Can you forward that to the right people please? Call
scripts/get_maintainer.pl to get a list of maintainers.

Daniel



> ---
>  fs/jffs2/erase.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
> index b47679b..c0a5604 100644
> --- a/fs/jffs2/erase.c
> +++ b/fs/jffs2/erase.c
> @@ -74,8 +74,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
>         ((struct erase_priv_struct *)instr->priv)->c = c;
> 
>         ret = c->mtd->erase(c->mtd, instr);
> -       if (!ret)
> +       if (!ret) {
> +        kfree(instr);
>                 return;
> +    }
> 
>         bad_offset = instr->fail_addr;
>         kfree(instr);
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] jffs2: fix memory leak if the sector was successfully erased

[niam]

Resending patch to proper people/mailing list.

Memory allocated for erase instruction is not freed if the sector was
successfully erased.

Signed-off-by: Dmytro Milinevskyy <niam.niam@gmail.com>
---
 fs/jffs2/erase.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
index b47679b..c0a5604 100644
--- a/fs/jffs2/erase.c
+++ b/fs/jffs2/erase.c
@@ -74,8 +74,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
       ((struct erase_priv_struct *)instr->priv)->c = c;

       ret = c->mtd->erase(c->mtd, instr);
-       if (!ret)
+       if (!ret) {
+        kfree(instr);
               return;
+    }

       bad_offset = instr->fail_addr;
       kfree(instr);

[PATCH] jffs2: fix memory leak if the sector was successfully erased

[Michael Trimarchi]

Hi,

Daniel Mack wrote:
> On Mon, Feb 22, 2010 at 06:26:22PM +0200, Ni at m wrote:
>   
>> Memory allocated for erase instruction is not freed if the sector was
>> successfully erased.
>>
>> Signed-off-by: Dmytro Milinevskyy <niam.niam@gmail.com>
>>     
>
> Can you forward that to the right people please? Call
> scripts/get_maintainer.pl to get a list of maintainers.
>
> Daniel
>
>
>
>   
>> ---
>>  fs/jffs2/erase.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
>> index b47679b..c0a5604 100644
>> --- a/fs/jffs2/erase.c
>> +++ b/fs/jffs2/erase.c
>> @@ -74,8 +74,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
>>         ((struct erase_priv_struct *)instr->priv)->c = c;
>>
>>         ret = c->mtd->erase(c->mtd, instr);
>> -       if (!ret)
>> +       if (!ret) {
>> +        kfree(instr);
>>                 return;
>> +    }
>>
>>         bad_offset = instr->fail_addr;
>>         kfree(instr);
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>     
>
>   
static void jffs2_erase_callback(struct erase_info *instr)
{
        struct erase_priv_struct *priv = (void *)instr->priv;

        if(instr->state != MTD_ERASE_DONE) {  
                printk(KERN_WARNING "Erase at 0x%08x finished, but state 
!= MTD_ERASE_DONE. State is 0x%x instead.\n", instr->addr, instr->state);
                jffs2_erase_failed(priv->c, priv->jeb, instr->fail_addr);
        } else {
                jffs2_erase_succeeded(priv->c, priv->jeb);
        }
        kfree(instr);
}

instr->callback = jffs2_erase_callback;

Maybe the free is here  :)

Michael
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>   

[PATCH] jffs2: fix memory leak if the sector was successfully erased

[niam]

Yes, indeed, you are right!

Thank you Michael!

Thank you to everybody and I'm sorry for doing that much noise here.

Next time I will try to send more correct patches.

However freeing memory in the callback is not obvious. Isn't better to
do this out of it?

-- Dima

On Mon, Feb 22, 2010 at 7:28 PM, Michael Trimarchi
<trimarchi@gandalf.sssup.it> wrote:
> Hi,
>
> Daniel Mack wrote:
>>
>> On Mon, Feb 22, 2010 at 06:26:22PM +0200, Ni at m wrote:
>>
>>>
>>> Memory allocated for erase instruction is not freed if the sector was
>>> successfully erased.
>>>
>>> Signed-off-by: Dmytro Milinevskyy <niam.niam@gmail.com>
>>>
>>
>> Can you forward that to the right people please? Call
>> scripts/get_maintainer.pl to get a list of maintainers.
>>
>> Daniel
>>
>>
>>
>>
>>>
>>> ---
>>> ?fs/jffs2/erase.c | ? ?4 +++-
>>> ?1 files changed, 3 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
>>> index b47679b..c0a5604 100644
>>> --- a/fs/jffs2/erase.c
>>> +++ b/fs/jffs2/erase.c
>>> @@ -74,8 +74,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
>>> ? ? ? ?((struct erase_priv_struct *)instr->priv)->c = c;
>>>
>>> ? ? ? ?ret = c->mtd->erase(c->mtd, instr);
>>> - ? ? ? if (!ret)
>>> + ? ? ? if (!ret) {
>>> + ? ? ? ?kfree(instr);
>>> ? ? ? ? ? ? ? ?return;
>>> + ? ?}
>>>
>>> ? ? ? ?bad_offset = instr->fail_addr;
>>> ? ? ? ?kfree(instr);
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
>>
>
> static void jffs2_erase_callback(struct erase_info *instr)
> {
> ? ? ? struct erase_priv_struct *priv = (void *)instr->priv;
>
> ? ? ? if(instr->state != MTD_ERASE_DONE) {
> printk(KERN_WARNING "Erase at 0x%08x finished, but state != MTD_ERASE_DONE.
> State is 0x%x instead.\n", instr->addr, instr->state);
> ? ? ? ? ? ? ? jffs2_erase_failed(priv->c, priv->jeb, instr->fail_addr);
> ? ? ? } else {
> ? ? ? ? ? ? ? jffs2_erase_succeeded(priv->c, priv->jeb);
> ? ? ? }
> ? ? ? kfree(instr);
> }
>
> instr->callback = jffs2_erase_callback;
>
> Maybe the free is here ?:)
>
> Michael
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>>
>
>

Re: [PATCH] jffs2: fix memory leak if the sector was successfully erased

[Joakim Tjernlund]

niam <niam.niam@gmail.com> wrote on 2010/02/22 17:40:11:
>
> Resending patch to proper people/mailing list.
>
> Memory allocated for erase instruction is not freed if the sector was
> successfully erased.

NAK, jffs2_erase_callback() will free it so
you have added a double free I think. Did you measure a memory leak?

>
> Signed-off-by: Dmytro Milinevskyy <niam.niam@gmail.com>
> ---
>  fs/jffs2/erase.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
> index b47679b..c0a5604 100644
> --- a/fs/jffs2/erase.c
> +++ b/fs/jffs2/erase.c
> @@ -74,8 +74,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
>        ((struct erase_priv_struct *)instr->priv)->c = c;
>
>        ret = c->mtd->erase(c->mtd, instr);
> -       if (!ret)
> +       if (!ret) {
> +        kfree(instr);
>                return;
> +    }
>
>        bad_offset = instr->fail_addr;
>        kfree(instr);
>

Re: [PATCH] jffs2: fix memory leak if the sector was successfully erased

[Joakim Tjernlund]

niam <niam.niam@gmail.com> wrote on 2010/02/22 17:40:11:
>
> Resending patch to proper people/mailing list.
>
> Memory allocated for erase instruction is not freed if the sector was
> successfully erased.

NAK, jffs2_erase_callback() will free it so
you have added a double free I think. Did you measure a memory leak?

>
> Signed-off-by: Dmytro Milinevskyy <niam.niam@gmail.com>
> ---
>  fs/jffs2/erase.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
> index b47679b..c0a5604 100644
> --- a/fs/jffs2/erase.c
> +++ b/fs/jffs2/erase.c
> @@ -74,8 +74,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
>        ((struct erase_priv_struct *)instr->priv)->c = c;
>
>        ret = c->mtd->erase(c->mtd, instr);
> -       if (!ret)
> +       if (!ret) {
> +        kfree(instr);
>                return;
> +    }
>
>        bad_offset = instr->fail_addr;
>        kfree(instr);
>

Re: [PATCH] jffs2: fix memory leak if the sector was successfully erased

[niam]

Yes, you are right.
Michael Trimarchi also responded that it's freed by the callback.

My concern now that it's not obvious to free erase instruction in
jffs2_erase_callback.
Why not to free it the same piece of code where it was allocated?

--Dima

On Mon, Feb 22, 2010 at 7:49 PM, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:
> niam <niam.niam@gmail.com> wrote on 2010/02/22 17:40:11:
>>
>> Resending patch to proper people/mailing list.
>>
>> Memory allocated for erase instruction is not freed if the sector was
>> successfully erased.
>
> NAK, jffs2_erase_callback() will free it so
> you have added a double free I think. Did you measure a memory leak?
>
>>
>> Signed-off-by: Dmytro Milinevskyy <niam.niam@gmail.com>
>> ---
>>  fs/jffs2/erase.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
>> index b47679b..c0a5604 100644
>> --- a/fs/jffs2/erase.c
>> +++ b/fs/jffs2/erase.c
>> @@ -74,8 +74,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
>>        ((struct erase_priv_struct *)instr->priv)->c = c;
>>
>>        ret = c->mtd->erase(c->mtd, instr);
>> -       if (!ret)
>> +       if (!ret) {
>> +        kfree(instr);
>>                return;
>> +    }
>>
>>        bad_offset = instr->fail_addr;
>>        kfree(instr);
>>
>
>

Re: [PATCH] jffs2: fix memory leak if the sector was successfully erased

[niam]

Yes, you are right.
Michael Trimarchi also responded that it's freed by the callback.

My concern now that it's not obvious to free erase instruction in
jffs2_erase_callback.
Why not to free it the same piece of code where it was allocated?

--Dima

On Mon, Feb 22, 2010 at 7:49 PM, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:
> niam <niam.niam@gmail.com> wrote on 2010/02/22 17:40:11:
>>
>> Resending patch to proper people/mailing list.
>>
>> Memory allocated for erase instruction is not freed if the sector was
>> successfully erased.
>
> NAK, jffs2_erase_callback() will free it so
> you have added a double free I think. Did you measure a memory leak?
>
>>
>> Signed-off-by: Dmytro Milinevskyy <niam.niam@gmail.com>
>> ---
>>  fs/jffs2/erase.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
>> index b47679b..c0a5604 100644
>> --- a/fs/jffs2/erase.c
>> +++ b/fs/jffs2/erase.c
>> @@ -74,8 +74,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
>>        ((struct erase_priv_struct *)instr->priv)->c = c;
>>
>>        ret = c->mtd->erase(c->mtd, instr);
>> -       if (!ret)
>> +       if (!ret) {
>> +        kfree(instr);
>>                return;
>> +    }
>>
>>        bad_offset = instr->fail_addr;
>>        kfree(instr);
>>
>
>