| Message ID | 20231222085446.781838-2-chengzhihao1@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ubifs: Fix two kmemleaks in error path | expand |
----- 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
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; err = ubifs_replay_journal(c); if (err)
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(-)