On Thu, Feb 10, 2022 at 09:07:11PM +0530, Ritesh Harjani wrote:
> jbd2_journal_wait_updates() is called with j_state_lock held. But if
> there is a commit in progress, then this transaction might get committed
> and freed via jbd2_journal_commit_transaction() ->
> jbd2_journal_free_transaction(), when we release j_state_lock.
> So check for journal->j_running_transaction everytime we release and
> acquire j_state_lock to avoid use-after-free issue.
>
> Fixes: 4f98186848707f53 ("jbd2: refactor wait logic for transaction updates into a common function")
> Reported-and-tested-by: syzbot+afa2ca5171d93e44b348@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Ritesh Harjani <riteshh@xxxxxxxxxxxxx>
Hi Ritesh,
Looking at the refactor in the commit this fixes, I believe the same
issue is present prior to the refactor, so this would apply before 5.17
as well.
I've posted a backport for 4.9-4.19 and 5.4-5.16 to stable here:
https://lore.kernel.org/stable/20220426182702.716304-1-samjonas@xxxxxxxxxx/T/#t
Please have a look and let me know if you agree.
Cheers,
Sam Mendoza-Jonas
> ---
> fs/jbd2/transaction.c | 41 +++++++++++++++++++++++++----------------
> 1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 8e2f8275a253..259e00046a8b 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -842,27 +842,38 @@ EXPORT_SYMBOL(jbd2_journal_restart);
> */
> void jbd2_journal_wait_updates(journal_t *journal)
> {
> - transaction_t *commit_transaction = journal->j_running_transaction;
> + DEFINE_WAIT(wait);
>
> - if (!commit_transaction)
> - return;
> + while (1) {
> + /*
> + * Note that the running transaction can get freed under us if
> + * this transaction is getting committed in
> + * jbd2_journal_commit_transaction() ->
> + * jbd2_journal_free_transaction(). This can only happen when we
> + * release j_state_lock -> schedule() -> acquire j_state_lock.
> + * Hence we should everytime retrieve new j_running_transaction
> + * value (after j_state_lock release acquire cycle), else it may
> + * lead to use-after-free of old freed transaction.
> + */
> + transaction_t *transaction = journal->j_running_transaction;
>
> - spin_lock(&commit_transaction->t_handle_lock);
> - while (atomic_read(&commit_transaction->t_updates)) {
> - DEFINE_WAIT(wait);
> + if (!transaction)
> + break;
>
> + spin_lock(&transaction->t_handle_lock);
> prepare_to_wait(&journal->j_wait_updates, &wait,
> - TASK_UNINTERRUPTIBLE);
> - if (atomic_read(&commit_transaction->t_updates)) {
> - spin_unlock(&commit_transaction->t_handle_lock);
> - write_unlock(&journal->j_state_lock);
> - schedule();
> - write_lock(&journal->j_state_lock);
> - spin_lock(&commit_transaction->t_handle_lock);
> + TASK_UNINTERRUPTIBLE);
> + if (!atomic_read(&transaction->t_updates)) {
> + spin_unlock(&transaction->t_handle_lock);
> + finish_wait(&journal->j_wait_updates, &wait);
> + break;
> }
> + spin_unlock(&transaction->t_handle_lock);
> + write_unlock(&journal->j_state_lock);
> + schedule();
> finish_wait(&journal->j_wait_updates, &wait);
> + write_lock(&journal->j_state_lock);
> }
> - spin_unlock(&commit_transaction->t_handle_lock);
> }
>
> /**
> @@ -877,8 +888,6 @@ void jbd2_journal_wait_updates(journal_t *journal)
> */
> void jbd2_journal_lock_updates(journal_t *journal)
> {
> - DEFINE_WAIT(wait);
> -
> jbd2_might_wait_for_commit(journal);
>
> write_lock(&journal->j_state_lock);
> --
> 2.31.1
>
