----- Ursprüngliche Mail -----
> Von: "chengzhihao1" <chengzhihao1@xxxxxxxxxx>
> An: "richard" <richard@xxxxxx>, terrelln@xxxxxx, "Eric Biggers" <ebiggers@xxxxxxxxxx>
> CC: "linux-fscrypt" <linux-fscrypt@xxxxxxxxxxxxxxx>, "linux-mtd" <linux-mtd@xxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> 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
|