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

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