| Message ID | 20230911025138.983101-1-lizetao1@huawei.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | jbd2: Fix memory leak in journal_init_common() | expand |
Li Zetao <lizetao1@huawei.com> writes: > There is a memory leak reported by kmemleak: > > unreferenced object 0xff11000105903b80 (size 64): > comm "mount", pid 3382, jiffies 4295032021 (age 27.826s) > hex dump (first 32 bytes): > 04 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 ................ > ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<ffffffffae86ac40>] __kmalloc_node+0x50/0x160 > [<ffffffffaf2486d8>] crypto_alloc_tfmmem.isra.0+0x38/0x110 > [<ffffffffaf2498e5>] crypto_create_tfm_node+0x85/0x2f0 > [<ffffffffaf24a92c>] crypto_alloc_tfm_node+0xfc/0x210 > [<ffffffffaedde777>] journal_init_common+0x727/0x1ad0 > [<ffffffffaede1715>] jbd2_journal_init_inode+0x2b5/0x500 > [<ffffffffaed786b5>] ext4_load_and_init_journal+0x255/0x2440 > [<ffffffffaed8b423>] ext4_fill_super+0x8823/0xa330 > ... > > The root cause was traced to an error handing path in journal_init_common() > when malloc memory failed in register_shrinker(). The checksum driver is > used to reference to checksum algorithm via cryptoapi and the user should > release the memory when the driver is no longer needed or the journal > initialization failed. > > Fix it by calling crypto_free_shash() on the "err_cleanup" error handing > path in journal_init_common(). > > Fixes: c30713084ba5 ("jbd2: move load_superblock() into journal_init_common()") > Signed-off-by: Li Zetao <lizetao1@huawei.com> Thanks for the fix. I looked at your patch and it looks correct to me. The patch mentioned in the "Fixes" tag moved the "load_superblock" function into journal_init_common() and from there onwards the cleanup routine in case of an error was not properly handled for crypto_alloc_shash()/j_chcksum_driver. IMO too, the correct place to handle the cleanup routine is journal_init_common() which is where this patch adds the cleanup. Looks good to me. Feel free to add:- Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > --- > fs/jbd2/journal.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c > index 768fa05bcbed..30dec2bd2ecc 100644 > --- a/fs/jbd2/journal.c > +++ b/fs/jbd2/journal.c > @@ -1601,6 +1601,8 @@ static journal_t *journal_init_common(struct block_device *bdev, > > err_cleanup: > percpu_counter_destroy(&journal->j_checkpoint_jh_count); > + if (journal->j_chksum_driver) > + crypto_free_shash(journal->j_chksum_driver); > kfree(journal->j_wbuf); > jbd2_journal_destroy_revoke(journal); > journal_fail_superblock(journal); > -- > 2.34.1
On 2023/9/11 10:51, Li Zetao wrote: > There is a memory leak reported by kmemleak: > > unreferenced object 0xff11000105903b80 (size 64): > comm "mount", pid 3382, jiffies 4295032021 (age 27.826s) > hex dump (first 32 bytes): > 04 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 ................ > ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<ffffffffae86ac40>] __kmalloc_node+0x50/0x160 > [<ffffffffaf2486d8>] crypto_alloc_tfmmem.isra.0+0x38/0x110 > [<ffffffffaf2498e5>] crypto_create_tfm_node+0x85/0x2f0 > [<ffffffffaf24a92c>] crypto_alloc_tfm_node+0xfc/0x210 > [<ffffffffaedde777>] journal_init_common+0x727/0x1ad0 > [<ffffffffaede1715>] jbd2_journal_init_inode+0x2b5/0x500 > [<ffffffffaed786b5>] ext4_load_and_init_journal+0x255/0x2440 > [<ffffffffaed8b423>] ext4_fill_super+0x8823/0xa330 > ... > > The root cause was traced to an error handing path in journal_init_common() > when malloc memory failed in register_shrinker(). The checksum driver is > used to reference to checksum algorithm via cryptoapi and the user should > release the memory when the driver is no longer needed or the journal > initialization failed. > > Fix it by calling crypto_free_shash() on the "err_cleanup" error handing > path in journal_init_common(). > > Fixes: c30713084ba5 ("jbd2: move load_superblock() into journal_init_common()") > Signed-off-by: Li Zetao <lizetao1@huawei.com> Thanks for the fix, it looks good to me. Reviewed-by: Zhang Yi <yi.zhang@huawei.com> > --- > fs/jbd2/journal.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c > index 768fa05bcbed..30dec2bd2ecc 100644 > --- a/fs/jbd2/journal.c > +++ b/fs/jbd2/journal.c > @@ -1601,6 +1601,8 @@ static journal_t *journal_init_common(struct block_device *bdev, > > err_cleanup: > percpu_counter_destroy(&journal->j_checkpoint_jh_count); > + if (journal->j_chksum_driver) > + crypto_free_shash(journal->j_chksum_driver); > kfree(journal->j_wbuf); > jbd2_journal_destroy_revoke(journal); > journal_fail_superblock(journal); >
On Mon 11-09-23 10:51:38, Li Zetao wrote: > There is a memory leak reported by kmemleak: > > unreferenced object 0xff11000105903b80 (size 64): > comm "mount", pid 3382, jiffies 4295032021 (age 27.826s) > hex dump (first 32 bytes): > 04 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 ................ > ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<ffffffffae86ac40>] __kmalloc_node+0x50/0x160 > [<ffffffffaf2486d8>] crypto_alloc_tfmmem.isra.0+0x38/0x110 > [<ffffffffaf2498e5>] crypto_create_tfm_node+0x85/0x2f0 > [<ffffffffaf24a92c>] crypto_alloc_tfm_node+0xfc/0x210 > [<ffffffffaedde777>] journal_init_common+0x727/0x1ad0 > [<ffffffffaede1715>] jbd2_journal_init_inode+0x2b5/0x500 > [<ffffffffaed786b5>] ext4_load_and_init_journal+0x255/0x2440 > [<ffffffffaed8b423>] ext4_fill_super+0x8823/0xa330 > ... > > The root cause was traced to an error handing path in journal_init_common() > when malloc memory failed in register_shrinker(). The checksum driver is > used to reference to checksum algorithm via cryptoapi and the user should > release the memory when the driver is no longer needed or the journal > initialization failed. > > Fix it by calling crypto_free_shash() on the "err_cleanup" error handing > path in journal_init_common(). > > Fixes: c30713084ba5 ("jbd2: move load_superblock() into journal_init_common()") > Signed-off-by: Li Zetao <lizetao1@huawei.com> Thanks for the fixup! Looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/jbd2/journal.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c > index 768fa05bcbed..30dec2bd2ecc 100644 > --- a/fs/jbd2/journal.c > +++ b/fs/jbd2/journal.c > @@ -1601,6 +1601,8 @@ static journal_t *journal_init_common(struct block_device *bdev, > > err_cleanup: > percpu_counter_destroy(&journal->j_checkpoint_jh_count); > + if (journal->j_chksum_driver) > + crypto_free_shash(journal->j_chksum_driver); > kfree(journal->j_wbuf); > jbd2_journal_destroy_revoke(journal); > journal_fail_superblock(journal); > -- > 2.34.1 >
On Mon, 11 Sep 2023 10:51:38 +0800, Li Zetao wrote: > There is a memory leak reported by kmemleak: > > unreferenced object 0xff11000105903b80 (size 64): > comm "mount", pid 3382, jiffies 4295032021 (age 27.826s) > hex dump (first 32 bytes): > 04 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 ................ > ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<ffffffffae86ac40>] __kmalloc_node+0x50/0x160 > [<ffffffffaf2486d8>] crypto_alloc_tfmmem.isra.0+0x38/0x110 > [<ffffffffaf2498e5>] crypto_create_tfm_node+0x85/0x2f0 > [<ffffffffaf24a92c>] crypto_alloc_tfm_node+0xfc/0x210 > [<ffffffffaedde777>] journal_init_common+0x727/0x1ad0 > [<ffffffffaede1715>] jbd2_journal_init_inode+0x2b5/0x500 > [<ffffffffaed786b5>] ext4_load_and_init_journal+0x255/0x2440 > [<ffffffffaed8b423>] ext4_fill_super+0x8823/0xa330 > ... > > [...] Applied, thanks! [1/1] jbd2: Fix memory leak in journal_init_common() commit: 414f73db6ce825b7264cacb9407581b87da60aeb Best regards,
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 768fa05bcbed..30dec2bd2ecc 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -1601,6 +1601,8 @@ static journal_t *journal_init_common(struct block_device *bdev, err_cleanup: percpu_counter_destroy(&journal->j_checkpoint_jh_count); + if (journal->j_chksum_driver) + crypto_free_shash(journal->j_chksum_driver); kfree(journal->j_wbuf); jbd2_journal_destroy_revoke(journal); journal_fail_superblock(journal);
There is a memory leak reported by kmemleak: unreferenced object 0xff11000105903b80 (size 64): comm "mount", pid 3382, jiffies 4295032021 (age 27.826s) hex dump (first 32 bytes): 04 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 ................ ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<ffffffffae86ac40>] __kmalloc_node+0x50/0x160 [<ffffffffaf2486d8>] crypto_alloc_tfmmem.isra.0+0x38/0x110 [<ffffffffaf2498e5>] crypto_create_tfm_node+0x85/0x2f0 [<ffffffffaf24a92c>] crypto_alloc_tfm_node+0xfc/0x210 [<ffffffffaedde777>] journal_init_common+0x727/0x1ad0 [<ffffffffaede1715>] jbd2_journal_init_inode+0x2b5/0x500 [<ffffffffaed786b5>] ext4_load_and_init_journal+0x255/0x2440 [<ffffffffaed8b423>] ext4_fill_super+0x8823/0xa330 ... The root cause was traced to an error handing path in journal_init_common() when malloc memory failed in register_shrinker(). The checksum driver is used to reference to checksum algorithm via cryptoapi and the user should release the memory when the driver is no longer needed or the journal initialization failed. Fix it by calling crypto_free_shash() on the "err_cleanup" error handing path in journal_init_common(). Fixes: c30713084ba5 ("jbd2: move load_superblock() into journal_init_common()") Signed-off-by: Li Zetao <lizetao1@huawei.com> --- fs/jbd2/journal.c | 2 ++ 1 file changed, 2 insertions(+)