Re: [PATCH v1] jbd2: check transaction state before stopping handle

[Jan Kara]

On Thu 05-03-26 20:57:18, Baolin Liu wrote:
> At 2026-03-05 20:54:02, "Baolin Liu" <liubaolin12138@xxxxxxx> wrote:
> >From: Baolin Liu <liubaolin@xxxxxxxxxx>
> >
> >When a transaction enters T_FLUSH or later states,
> >handle->h_transaction may still point to it.
> >If jbd2_journal_stop() or jbd2__journal_restart() is called,
> >stop_this_handle() checks t_updates > 0, but t_updates is
> >already 0 for these states, causing a kernel BUG.

Which bug please? Do you mean

J_ASSERT(atomic_read(&transaction->t_updates) > 0)

? Anyway this just doesn't make sense. When stop_this_handle() the caller
is holding t_updates reference which stop_this_handle() is supposed to drop
and the transaction should never transition past T_LOCKED state. If you
have a handle that's pointing to a transaction past T_LOCKED state, there's
a bug somewhere and that bug needs to be fixed, not paper over it like you
do in this patch. More details about reproducer etc. would be useful.

								Honza

> >
> >Fix by checking transaction->t_state in jbd2_journal_stop()
> >and jbd2__journal_restart() before calling stop_this_handle().
> >If the transaction is not in T_RUNNING or T_LOCKED state,
> >clear handle->h_transaction and skip stop_this_handle().
> >
> >Crash stack:
> >  Call trace:
> >  stop_this_handle+0x148/0x158
> >  jbd2_journal_stop+0x198/0x388
> >  __ext4_journal_stop+0x70/0xf0
> >  ext4_create+0x12c/0x188
> >  lookup_open+0x214/0x6d8
> >  do_last+0x364/0x878
> >  path_openat+0x6c/0x280
> >  do_filp_open+0x70/0xe8
> >  do_sys_open+0x178/0x200
> >  sys_openat+0x3c/0x50
> >  el0_svc_naked+0x44/0x48
> >
> >Signed-off-by: Baolin Liu <liubaolin@xxxxxxxxxx>
> >---
> > fs/jbd2/transaction.c | 25 +++++++++++++++++++++++--
> > 1 file changed, 23 insertions(+), 2 deletions(-)
> >
> >diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> >index dca4b5d8aaaa..3779382dbb80 100644
> >--- a/fs/jbd2/transaction.c
> >+++ b/fs/jbd2/transaction.c
> >@@ -772,14 +772,25 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, int revoke_records,
> > 	journal = transaction->t_journal;
> > 	tid = transaction->t_tid;
> > 
> >+	jbd2_debug(2, "restarting handle %p\n", handle);
> >+
> >+	/* Check if transaction is in invalid state */
> >+	if (transaction->t_state != T_RUNNING &&
> >+		transaction->t_state != T_LOCKED) {
> >+		if (current->journal_info == handle)
> >+			current->journal_info = NULL;
> >+		handle->h_transaction = NULL;
> >+		memalloc_nofs_restore(handle->saved_alloc_context);
> >+		goto skip_stop;
> >+	}
> >+
> > 	/*
> > 	 * First unlink the handle from its current transaction, and start the
> > 	 * commit on that.
> > 	 */
> >-	jbd2_debug(2, "restarting handle %p\n", handle);
> > 	stop_this_handle(handle);
> > 	handle->h_transaction = NULL;
> >-
> >+skip_stop:
> > 	/*
> > 	 * TODO: If we use READ_ONCE / WRITE_ONCE for j_commit_request we can
> >  	 * get rid of pointless j_state_lock traffic like this.
> >@@ -1856,6 +1867,16 @@ int jbd2_journal_stop(handle_t *handle)
> > 		memalloc_nofs_restore(handle->saved_alloc_context);
> > 		goto free_and_exit;
> > 	}
> >+	/* Check if transaction is in invalid state */
> >+	if (transaction->t_state != T_RUNNING &&
> >+		transaction->t_state != T_LOCKED) {
> >+		if (current->journal_info == handle)
> >+			current->journal_info = NULL;
> >+		handle->h_transaction = NULL;
> >+		memalloc_nofs_restore(handle->saved_alloc_context);
> >+		goto free_and_exit;
> >+	}
> >+
> > 	journal = transaction->t_journal;
> > 	tid = transaction->t_tid;
> > 
> >-- 
> >2.39.2
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR