Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 29 Oct 2019 10:20:57 +0100
From:   Sascha Hauer <s.hauer@pengutronix.de>
To:     Zhihao Cheng <chengzhihao1@huawei.com>
Cc:     richard@nod.at, dedekind1@gmail.com, yi.zhang@huawei.com,
        linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ubifs: do_kill_orphans: Fix a memory leak bug
Message-ID: <20191029092057.fsklsibqrmbmacar@pengutronix.de>
References: <1572339670-68694-1-git-send-email-chengzhihao1@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1572339670-68694-1-git-send-email-chengzhihao1@huawei.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 |