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

Hello!

On Thu 20-08-26 05:21:02, Fan Wu wrote:
> ext4_put_super() destroys the journal before it calls
> ext4_mb_release(), but the failure unwind of __ext4_fill_super() runs
> the two steps in the opposite order: failed_mount6 calls
> ext4_mb_release(), and the journal is destroyed only later, just above
> failed_mount3a.
> 
> ext4_mb_release() flushes sbi->s_discard_work before releasing the
> mballoc structures, a drain made unconditional in commit 9ee29d20aab2
> ("ext4: always drain queued discard work in ext4_mb_release()").  With
> -o discard, the journal destroy that runs afterwards re-arms that work
> after the flush: ext4_journal_destroy() calls ext4_force_commit(), and
> the commit callback, registered once mballoc is initialized, queues
> s_discard_work whenever the discard option is set, even with an empty
> freed-data list.  A quota-enabled mount reaches this with a live
> transaction (ext4_enable_quotas() failing into failed_mount8,
> ext4_quotas_off() on the failed_mount9 path), so the final force
> commit is not a no-op.

Yes, I think orphan replay is the easiest way how to have running
transaction in the journal in this shutdown path.

> Nothing drains s_discard_work after that point: failed_mount3 flushes
> only s_sb_upd_work and the s_err_report timer, and ext4_fill_super()
> then frees sbi with a plain kfree() through ext4_free_sbi().  If the
> system_dfl_wq worker is delayed across the rest of the unwind,
> ext4_discard_work() then accesses the freed sbi, first through
> sbi->s_sb and then while taking sbi->s_md_lock.
> 
> This is the pattern fixed for the s_err_report timer in commit
> 0ce160c5bdb6 ("ext4: fix timer use-after-free on failed mount"):
> async state armed after the unwind's last drain point.
> 
> Destroy the journal at failed_mount6, before ext4_mb_release().  This
> restores the ext4_put_super() order, so the final commit re-arms
> s_discard_work before the flush_work() in ext4_mb_release() drains
> it.  The journal destroy further down the unwind still covers paths
> that enter below failed_mount6; on the failed_mount6..failed_mount9
> paths it does nothing, because ext4_journal_destroy() clears
> sbi->s_journal.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: 55cdd0af2bc5 ("ext4: get discard out of jbd2 commit kthread contex")
> Cc: [email protected]
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <[email protected]>

Thanks for the analysis and the patch. I agree the problem exists and your
patch fixes it but I just don't like the solution. Firstly, it looks very
suspicious both failed_mount6 and failed_mount4 labels would be shutting
down the journal. It makes me even more nervous that only failed_mount4a
does:
        dput(sb->s_root);
        sb->s_root = NULL;

which drops the root directory inode reference and with your change you'd
already have shutdown journal by that time which could cause problems in
some corner cases. Normal fs unmount path drops sb->s_root reference even
before calling ext4_put_super() BTW...

So to summarize I don't think we should really shutdown the journal
earlier. I think failed_mount6 should just force the commit to flush out
the running transaction before mballoc will be shut down and
ext4_mb_release() can also use disable_work() after flush_work() to make
sure no new s_discard_work can be queued as a safety measure.

								Honza

> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 4b6112e5d6c5..58b210637b57 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5761,6 +5761,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:
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR