[PATCH] ubifs: do_kill_orphans: Fix a memory leak bug

[PATCH] ubifs: do_kill_orphans: Fix a memory leak bug

[Zhihao Cheng]

If there are more than one valid snod on the sleb->nodes list,
do_kill_orphans will malloc ino more than once without releasing
previous ino's memory. Finally, it will trigger memory leak.

Fixes: ee1438ce5dc4 ("ubifs: Check link count of inodes when...")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/ubifs/orphan.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/ubifs/orphan.c b/fs/ubifs/orphan.c
index 3b4b411..f211ed3 100644
--- a/fs/ubifs/orphan.c
+++ b/fs/ubifs/orphan.c
@@ -673,9 +673,11 @@ static int do_kill_orphans(struct ubifs_info *c, struct ubifs_scan_leb *sleb,
 		if (first)
 			first = 0;
 
-		ino = kmalloc(UBIFS_MAX_INO_NODE_SZ, GFP_NOFS);
-		if (!ino)
-			return -ENOMEM;
+		if (!ino) {
+			ino = kmalloc(UBIFS_MAX_INO_NODE_SZ, GFP_NOFS);
+			if (!ino)
+				return -ENOMEM;
+		}
 
 		n = (le32_to_cpu(orph->ch.len) - UBIFS_ORPH_NODE_SZ) >> 3;
 		for (i = 0; i < n; i++) {
-- 
2.7.4


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] ubifs: do_kill_orphans: Fix a memory leak bug

[Sascha Hauer]

On Tue, Oct 29, 2019 at 05:01:10PM +0800, Zhihao Cheng wrote:
> If there are more than one valid snod on the sleb->nodes list,
> do_kill_orphans will malloc ino more than once without releasing
> previous ino's memory. Finally, it will trigger memory leak.
> 
> Fixes: ee1438ce5dc4 ("ubifs: Check link count of inodes when...")
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> ---
>  fs/ubifs/orphan.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ubifs/orphan.c b/fs/ubifs/orphan.c
> index 3b4b411..f211ed3 100644
> --- a/fs/ubifs/orphan.c
> +++ b/fs/ubifs/orphan.c
> @@ -673,9 +673,11 @@ static int do_kill_orphans(struct ubifs_info *c, struct ubifs_scan_leb *sleb,
>  		if (first)
>  			first = 0;
>  
> -		ino = kmalloc(UBIFS_MAX_INO_NODE_SZ, GFP_NOFS);
> -		if (!ino)
> -			return -ENOMEM;
> +		if (!ino) {
> +			ino = kmalloc(UBIFS_MAX_INO_NODE_SZ, GFP_NOFS);
> +			if (!ino)
> +				return -ENOMEM;
> +		}

This solves only part of the problem. There are several places in the
loop that just return instead of jumping to out_free. These need to be
fixed as well.
I am not sure if it's worth it to allocate ino on demand. It would be
more straight forward to allocate it once initially before the loop.
Not sure though what Richard prefers.

Regards,
  Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] ubifs: do_kill_orphans: Fix a memory leak bug

[Richard Weinberger]

On Tue, Oct 29, 2019 at 10:21 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Tue, Oct 29, 2019 at 05:01:10PM +0800, Zhihao Cheng wrote:
> > If there are more than one valid snod on the sleb->nodes list,
> > do_kill_orphans will malloc ino more than once without releasing
> > previous ino's memory. Finally, it will trigger memory leak.
> >
> > Fixes: ee1438ce5dc4 ("ubifs: Check link count of inodes when...")
> > Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> > ---
> >  fs/ubifs/orphan.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ubifs/orphan.c b/fs/ubifs/orphan.c
> > index 3b4b411..f211ed3 100644
> > --- a/fs/ubifs/orphan.c
> > +++ b/fs/ubifs/orphan.c
> > @@ -673,9 +673,11 @@ static int do_kill_orphans(struct ubifs_info *c, struct ubifs_scan_leb *sleb,
> >               if (first)
> >                       first = 0;
> >
> > -             ino = kmalloc(UBIFS_MAX_INO_NODE_SZ, GFP_NOFS);
> > -             if (!ino)
> > -                     return -ENOMEM;
> > +             if (!ino) {
> > +                     ino = kmalloc(UBIFS_MAX_INO_NODE_SZ, GFP_NOFS);
> > +                     if (!ino)
> > +                             return -ENOMEM;
> > +             }
>
> This solves only part of the problem. There are several places in the
> loop that just return instead of jumping to out_free. These need to be
> fixed as well.
> I am not sure if it's worth it to allocate ino on demand. It would be
> more straight forward to allocate it once initially before the loop.
> Not sure though what Richard prefers.

Yeah, allocating it outside the loop once would be the best solution.
I don't know why I did it in the loop. ;-\

-- 
Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/