Re: [PATCH v2 1/2] ubifs: dbg_check_idx_size: Fix kmemleak if loading znode failed

From: Richard Weinberger <richard@nod.at>
To: chengzhihao1 <chengzhihao1@huawei.com>
Cc: terrelln@fb.com, Eric Biggers <ebiggers@google.com>,
	 linux-fscrypt <linux-fscrypt@vger.kernel.org>,
	 linux-mtd <linux-mtd@lists.infradead.org>
Subject: Re: [PATCH v2 1/2] ubifs: dbg_check_idx_size: Fix kmemleak if loading znode failed
Date: Sat, 6 Jan 2024 23:34:13 +0100 (CET)	[thread overview]
Message-ID: <533956769.204398.1704580453236.JavaMail.zimbra@nod.at> (raw)
In-Reply-To: <20231222085446.781838-2-chengzhihao1@huawei.com>

----- Ursprüngliche Mail -----
> Von: "chengzhihao1" <chengzhihao1@huawei.com>
> An: "richard" <richard@nod.at>, terrelln@fb.com, "Eric Biggers" <ebiggers@google.com>
> CC: "linux-fscrypt" <linux-fscrypt@vger.kernel.org>, "linux-mtd" <linux-mtd@lists.infradead.org>
> Gesendet: Freitag, 22. Dezember 2023 09:54:45
> Betreff: [PATCH v2 1/2] ubifs: dbg_check_idx_size: Fix kmemleak if loading znode failed

> If function dbg_check_idx_size() failed by loading znode in mounting
> process, there are two problems:
>  1. Allocated znodes won't be freed, which causes kmemleak in kernel:
>     ubifs_mount
>      dbg_check_idx_size
>       dbg_walk_index
>        c->zroot.znode = ubifs_load_znode
>	child = ubifs_load_znode // failed
>	// Loaded znodes won't be freed in error handling path.
>  2. Global variable ubifs_clean_zn_cnt is not decreased, because
>     ubifs_tnc_close() is not invoked in error handling path, which
>     triggers a warning in ubifs_exit():
>      WARNING: CPU: 1 PID: 1576 at fs/ubifs/super.c:2486 ubifs_exit
>      Modules linked in: zstd ubifs(-) ubi nandsim
>      CPU: 1 PID: 1576 Comm: rmmod Not tainted 6.7.0-rc6
>      Call Trace:
>	ubifs_exit+0xca/0xc70 [ubifs]
>	__do_sys_delete_module+0x29a/0x4a0
>	do_syscall_64+0x6f/0x140
> 
> Fix it by invoking destroy_journal() if dbg_check_idx_size() failed.
> 
> Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
> fs/ubifs/super.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 09e270d6ed02..eabb0f44ea3e 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1449,7 +1449,7 @@ static int mount_ubifs(struct ubifs_info *c)
> 
> 	err = dbg_check_idx_size(c, c->bi.old_idx_sz);
> 	if (err)
> -		goto out_lpt;
> +		goto out_journal;

While it is technically not wrong, calling destroy_journal() before ubifs_replay_journal() is highly confusing
to future readers of the source code.
It seem to work because ubifs_replay_journal() can deal with the fact the journal is not yet initialized.
I'd rather suggest to add a distinct function which undoes what dbg_check_idx_size() did.
Alternatively, dbg_check_idx_size() could also cleanup itself after failure.

Thanks,
//richard

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