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.

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: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 fs/ext4/super.c | 6 ++++++
 1 file changed, 6 insertions(+)

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:

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: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>

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

Hello, Jan

On 26-08-20 19:08, Jan Kara wrote:
> Yes, I think orphan replay is the easiest way how to have running
> transaction in the journal in this shutdown path.

Understood, thanks — I'll describe the trigger in terms of orphan
replay in v2.

> 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.

Agreed — keeping the journal shutdown at its existing point is safer.
I'll send a v2 that calls ext4_force_commit() at failed_mount6 before
ext4_mb_release(), and adds disable_work() after the existing
flush_work() in ext4_mb_release().

One detail I noticed while checking the ordering: jbd2 publishes
j_commit_sequence before invoking the commit callback, so
ext4_force_commit() can return before that callback has finished.  The
callback can therefore queue s_discard_work after flush_work() returns.
disable_work() cancels such a queued instance, but does not wait if it
has already started executing in that small window.  I have kept
disable_work() as you suggested; please let me know if you would prefer
disable_work_sync() for that case.

Thanks for the review!
Fan