Re: [PATCH] ext4: fix discard work use-after-free on failed mount

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Incorrect teardown ordering causes Use-After-Free of s_group_info and unjournaled metadata modifications during mount failure.
--

--- Patch [1]: [PATCH] ext4: fix discard work use-after-free on failed mount ---
commit 7b05e08bede19859144c19d2964e5f55dbc626c5
Author: Fan Wu <[email protected]>

ext4: fix discard work use-after-free on failed mount

This commit addresses a use-after-free during a failed mount by ensuring
the journal is destroyed before ext4_mb_release() drains the discard work.

> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index bca0dc87d0b7c..5ec1d8f54e917 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5776,6 +5776,12 @@ failed_mount8: __maybe_unused
>  failed_mount7:
>  	ext4_unregister_li_request(sb);
>  failed_mount6:
> +	/*
> +	 * The last commit can re-arm s_discard_work, so destroy the
> +	 * journal before ext4_mb_release() flushes it.
> +	 */
> +	if (sbi->s_journal)
> +		ext4_journal_destroy(sbi, sbi->s_journal);
>  	ext4_mb_release(sb);
>  	ext4_flex_groups_free(sbi);
>  failed_mount5:

[Severity: Critical]
This is a pre-existing issue, but does this error unwind sequence leave
sbi->s_group_info exposed to a use-after-free further down the failure path?

If a filesystem mount fails after journal replay (for example, if
ext4_enable_quotas() fails), the error unwind reaches failed_mount6 and
calls ext4_mb_release(), which frees sbi->s_group_info.

The unwind then continues down to failed_mount_wq:

failed_mount_wq:
        flush_delayed_work(&sbi->s_ea_inode_work);

If journal replay had queued EA inodes for eviction, the flush worker
will process them:

ext4_evict_inode()
   ext4_truncate()
      ext4_free_blocks()
         ext4_mb_clear_bb()
            ext4_get_group_info()

Can this lead to ext4_mb_clear_bb() dereferencing the freed
sbi->s_group_info array?

Additionally, because this patch moves ext4_journal_destroy() to run before
the flush, sbi->s_journal is now NULL when ext4_evict_inode() runs. Will
this cause ext4_journal_start() to return a nojournal handle, resulting in
unjournaled metadata modifications?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1