[PATCH] jbd2: Fix memory leak in journal_init_common()

[PATCH] jbd2: Fix memory leak in journal_init_common()

[Li Zetao]

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(+)

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

Re: [PATCH] jbd2: Fix memory leak in journal_init_common()

[Ritesh Harjani]

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

Re: [PATCH] jbd2: Fix memory leak in journal_init_common()

[Zhang Yi]

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);
> 

Re: [PATCH] jbd2: Fix memory leak in journal_init_common()

[Jan Kara]

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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] jbd2: Fix memory leak in journal_init_common()

[Theodore Ts'o]

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,
-- 
Theodore Ts'o <tytso@mit.edu>