ubifs: do_kill_orphans: Fix a memory leak bug
diff mbox series

Message ID 1572339670-68694-1-git-send-email-chengzhihao1@huawei.com
State Superseded
Headers show
Series
  • ubifs: do_kill_orphans: Fix a memory leak bug
Related show

Commit Message

Zhihao Cheng Oct. 29, 2019, 9:01 a.m. UTC
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(-)

Comments

Sascha Hauer Oct. 29, 2019, 9:20 a.m. UTC | #1
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
Richard Weinberger Oct. 29, 2019, 10:25 a.m. UTC | #2
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. ;-\

Patch
diff mbox series

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++) {