From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ww0-f49.google.com ([74.125.82.49]) by bombadil.infradead.org with esmtp (Exim 4.69 #1 (Red Hat Linux)) id 1NjcXX-00040z-Vj for linux-mtd@lists.infradead.org; Mon, 22 Feb 2010 17:57:59 +0000 Received: by wwd20 with SMTP id 20so438756wwd.36 for ; Mon, 22 Feb 2010 09:57:50 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: From: niam Date: Mon, 22 Feb 2010 19:57:29 +0200 Message-ID: Subject: Re: [PATCH] jffs2: fix memory leak if the sector was successfully erased To: Joakim Tjernlund Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Joe Perches , linux-mtd@lists.infradead.org, David Woodhouse , linux-kernel@vger.kernel.org, Michael Trimarchi List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 wrote: > niam 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 >> --- >> =A0fs/jffs2/erase.c | =A0 =A04 +++- >> =A01 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= , >> =A0 =A0 =A0 =A0((struct erase_priv_struct *)instr->priv)->c =3D c; >> >> =A0 =A0 =A0 =A0ret =3D c->mtd->erase(c->mtd, instr); >> - =A0 =A0 =A0 if (!ret) >> + =A0 =A0 =A0 if (!ret) { >> + =A0 =A0 =A0 =A0kfree(instr); >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return; >> + =A0 =A0} >> >> =A0 =A0 =A0 =A0bad_offset =3D instr->fail_addr; >> =A0 =A0 =A0 =A0kfree(instr); >> > > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753506Ab0BVR5y (ORCPT ); Mon, 22 Feb 2010 12:57:54 -0500 Received: from mail-ew0-f228.google.com ([209.85.219.228]:60791 "EHLO mail-ew0-f228.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752554Ab0BVR5w convert rfc822-to-8bit (ORCPT ); Mon, 22 Feb 2010 12:57:52 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:content-transfer-encoding; b=jV5KSfvRykqeQHgHv1gcpF07hNoNF6RyBi13JFjuNv6jNwNJTzERPe5IZsSRZNkYrM u4k5ZdQSuVUynhbTxCg1Eqi4VkM+4uNflSJzopQ9DWMGfIN6B4HAcJPXS1t0uduop8av /1UQY7EUGdbmYV0gXWFhjz9L9JuFe6hV6i+Us= MIME-Version: 1.0 In-Reply-To: References: From: niam Date: Mon, 22 Feb 2010 19:57:29 +0200 Message-ID: Subject: Re: [PATCH] jffs2: fix memory leak if the sector was successfully erased To: Joakim Tjernlund Cc: David Woodhouse , Joe Perches , linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org, Michael Trimarchi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 wrote: > niam 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 >> --- >>  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); >> > >