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@xxxxxxxxxxxxxxx # v6.10+
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>

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

Reviewed-by: Jan Kara <jack@xxxxxxx>

>  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@xxxxxxxx>
SUSE Labs, CR