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

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

[Fan Wu]

ext4_put_super() shuts the journal down before it releases the mballoc
structures, but the failure unwind of __ext4_fill_super() runs the two
steps in the opposite order: failed_mount6 calls ext4_mb_release(),
which flushes sbi->s_discard_work, and the journal is destroyed only
later, just above failed_mount3a.

With -o discard, that journal destroy re-arms the 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 running transaction can be live at that point: replaying the orphan
list is the easiest way to get one, and the quota paths on
failed_mount8/failed_mount9 can leave one too. The final force commit
is not necessarily 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.

Force the commit at failed_mount6, before ext4_mb_release(), so that
the discard work its commit callback queues is normally already
pending when the flush_work() in ext4_mb_release() runs.
disable_work_sync() there then makes this airtight: it also drains an
instance the flush missed, and no later commit, including the force
commit inside the journal destroy further down the unwind, can
requeue the work. The journal destroy stays at its existing position.

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 # v6.10+
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
v2: per review from Jan Kara, keep the journal shutdown at its existing
    point instead of destroying the journal at failed_mount6: force the
    outstanding transaction there so the re-arm happens before the
    ext4_mb_release() flush, and disable_work_sync() the discard work
    after the flush. jbd2 publishes j_commit_sequence before running
    the commit callback, so the callback can still queue the work
    after ext4_force_commit() and the flush have both returned;
    disable_work_sync() drains that instance as well and keeps the
    later journal destroy from requeueing the work.
v1: https://lore.kernel.org/linux-ext4/20260820052102.4616-1-fanwu01@zju.edu.cn/
---
 fs/ext4/mballoc.c | 2 ++
 fs/ext4/super.c   | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index ed1bd00e11cd..874b71931ab6 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3897,6 +3897,8 @@ void ext4_mb_release(struct super_block *sb)
 	 * wait the discard work to drain all of ext4_free_data
 	 */
 	flush_work(&sbi->s_discard_work);
+	/* Prevent the later journal teardown from requeueing discard work. */
+	disable_work_sync(&sbi->s_discard_work);
 	WARN_ON_ONCE(!list_empty(&sbi->s_discard_list));
 
 	group_info = rcu_access_pointer(sbi->s_group_info);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 4b6112e5d6c5..62d1930e9783 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5761,6 +5761,15 @@ failed_mount8: __maybe_unused
 failed_mount7:
 	ext4_unregister_li_request(sb);
 failed_mount6:
+	/*
+	 * Flush any running transaction: its commit callback may queue
+	 * s_discard_work, which the flush_work() in ext4_mb_release()
+	 * below drains; disable_work_sync() there also drains an
+	 * instance queued after that, and keeps the journal destroy
+	 * further down the unwind from requeueing the work.
+	 */
+	if (sbi->s_journal)
+		ext4_force_commit(sb);
 	ext4_mb_release(sb);
 	ext4_flex_groups_free(sbi);
 failed_mount5:

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] disable_work_sync() on s_discard_work leaks ext4_free_data entries due to a race with the JBD2 commit callback
--

commit ed64edc319791fc718daf68ae4f355888a8e7de0
Author: Fan Wu <fanwu01@zju.edu.cn>

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

This commit fixes a use-after-free of sbi->s_discard_work on failed mounts.
It forces a journal commit before ext4_mb_release() and uses
disable_work_sync() to prevent the journal teardown from requeueing the
discard work.

> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index bca0dc87d0b7c..672aec4c3616f 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5776,6 +5776,15 @@ failed_mount8: __maybe_unused
>  failed_mount7:
>  	ext4_unregister_li_request(sb);
>  failed_mount6:
> +	/*
> +	 * Flush any running transaction: its commit callback may queue
> +	 * s_discard_work, which the flush_work() in ext4_mb_release()
> +	 * below drains; disable_work_sync() there also drains an
> +	 * instance queued after that, and keeps the journal destroy
> +	 * further down the unwind from requeueing the work.
> +	 */
> +	if (sbi->s_journal)
> +		ext4_force_commit(sb);
>  	ext4_mb_release(sb);
>  	ext4_flex_groups_free(sbi);
>  failed_mount5:

[Severity: Medium]
Can ext4_force_commit() return before the JBD2 commit callback fully
completes?

If JBD2 updates j_commit_sequence before executing the callback, it appears
ext4_force_commit() can return early. This would allow the mount thread to
proceed to ext4_mb_release() while the JBD2 thread is concurrently running
the commit callback.

> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 06171a11db125..3f99efe5e2d5c 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3898,6 +3898,8 @@ void ext4_mb_release(struct super_block *sb)
>  	 * wait the discard work to drain all of ext4_free_data
>  	 */
>  	flush_work(&sbi->s_discard_work);
> +	/* Prevent the later journal teardown from requeueing discard work. */
> +	disable_work_sync(&sbi->s_discard_work);
>  	WARN_ON_ONCE(!list_empty(&sbi->s_discard_list));

[Severity: Medium]
Does this sequence leak ext4_free_data structures if it races with the
JBD2 commit callback?

If flush_work() returns immediately (because the work isn't queued yet),
and disable_work_sync() is called while the JBD2 thread is executing
ext4_process_freed_data():

fs/ext4/mballoc.c:ext4_process_freed_data() {
	...
		list_splice_tail(&freed_data_list, &sbi->s_discard_list);
		spin_unlock(&sbi->s_md_lock);
		if (wake)
			queue_work(system_dfl_wq, &sbi->s_discard_work);
	...
}

Since the work is now disabled, queue_work() fails. Are the ext4_free_data
items spliced onto sbi->s_discard_list permanently stranded, leaking memory
and triggering the WARN_ON_ONCE() immediately following disable_work_sync()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909054124.657782-1-fanwu01@zju.edu.cn?part=1

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

[Jan Kara]

On Wed 09-09-26 05:41:24, Fan Wu wrote:
> ext4_put_super() shuts the journal down before it releases the mballoc
> structures, but the failure unwind of __ext4_fill_super() runs the two
> steps in the opposite order: failed_mount6 calls ext4_mb_release(),
> which flushes sbi->s_discard_work, and the journal is destroyed only
> later, just above failed_mount3a.
> 
> With -o discard, that journal destroy re-arms the 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 running transaction can be live at that point: replaying the orphan
> list is the easiest way to get one, and the quota paths on
> failed_mount8/failed_mount9 can leave one too. The final force commit
> is not necessarily 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.
> 
> Force the commit at failed_mount6, before ext4_mb_release(), so that
> the discard work its commit callback queues is normally already
> pending when the flush_work() in ext4_mb_release() runs.
> disable_work_sync() there then makes this airtight: it also drains an
> instance the flush missed, and no later commit, including the force
> commit inside the journal destroy further down the unwind, can
> requeue the work. The journal destroy stays at its existing position.
> 
> 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 # v6.10+
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>

Looks good to me. Just two nits below. After fixing them feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

>  failed_mount6:
> +	/*
> +	 * Flush any running transaction: its commit callback may queue
> +	 * s_discard_work, which the flush_work() in ext4_mb_release()
> +	 * below drains; disable_work_sync() there also drains an
> +	 * instance queued after that, and keeps the journal destroy
> +	 * further down the unwind from requeueing the work.
> +	 */

I think this comment is a bit too detailed. Maybe just:
	/*
	 * We can have a running transaction from orphan replay or quota
	 * setup. Commit it so that discard work after commit runs before
	 * we shutdown mballoc.
	 */

> +	if (sbi->s_journal)

No need for this check. ext4_force_commit() -> ext4_journal_force_commit()
does it on its own.

> +		ext4_force_commit(sb);
>  	ext4_mb_release(sb);
>  	ext4_flex_groups_free(sbi);
>  failed_mount5:

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR