[PATCH] jbd2: validate transaction state before dropping from journal

[PATCH] jbd2: validate transaction state before dropping from journal

[Milos Nikic]

Currently, __jbd2_journal_drop_transaction() unlinks the transaction
from the journal's checkpoint lists and only then proceeds to validate the
transaction's internal state using a series of J_ASSERTs.

There is no need to 'mutate before validate'. If we are going to halt the
system that makes manipulating corrupted pointers in memory irrelevant.

Move the state validation block above the pointer manipulation. This
ensures the transaction is entirely valid before modifying the journal's
internal lists, modernizing the function's logic and paving the way
for future graceful degradation of these assertions.

Signed-off-by: Milos Nikic <nikic.milos@gmail.com>
---
 fs/jbd2/checkpoint.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
index 1508e2f54462..c82b6bedd27b 100644
--- a/fs/jbd2/checkpoint.c
+++ b/fs/jbd2/checkpoint.c
@@ -703,6 +703,15 @@ void __jbd2_journal_drop_transaction(journal_t *journal, transaction_t *transact
 {
 	assert_spin_locked(&journal->j_list_lock);
 
+	J_ASSERT(transaction->t_state == T_FINISHED);
+	J_ASSERT(transaction->t_buffers == NULL);
+	J_ASSERT(transaction->t_forget == NULL);
+	J_ASSERT(transaction->t_shadow_list == NULL);
+	J_ASSERT(transaction->t_checkpoint_list == NULL);
+	J_ASSERT(atomic_read(&transaction->t_updates) == 0);
+	J_ASSERT(journal->j_committing_transaction != transaction);
+	J_ASSERT(journal->j_running_transaction != transaction);
+
 	journal->j_shrink_transaction = NULL;
 	if (transaction->t_cpnext) {
 		transaction->t_cpnext->t_cpprev = transaction->t_cpprev;
@@ -714,15 +723,6 @@ void __jbd2_journal_drop_transaction(journal_t *journal, transaction_t *transact
 			journal->j_checkpoint_transactions = NULL;
 	}
 
-	J_ASSERT(transaction->t_state == T_FINISHED);
-	J_ASSERT(transaction->t_buffers == NULL);
-	J_ASSERT(transaction->t_forget == NULL);
-	J_ASSERT(transaction->t_shadow_list == NULL);
-	J_ASSERT(transaction->t_checkpoint_list == NULL);
-	J_ASSERT(atomic_read(&transaction->t_updates) == 0);
-	J_ASSERT(journal->j_committing_transaction != transaction);
-	J_ASSERT(journal->j_running_transaction != transaction);
-
 	trace_jbd2_drop_transaction(journal, transaction);
 
 	jbd2_debug(1, "Dropping transaction %d, all done\n", transaction->t_tid);
-- 
2.53.0

Re: [PATCH] jbd2: validate transaction state before dropping from journal

[Jan Kara]

On Mon 13-04-26 11:08:24, Milos Nikic wrote:
> Currently, __jbd2_journal_drop_transaction() unlinks the transaction
> from the journal's checkpoint lists and only then proceeds to validate the
> transaction's internal state using a series of J_ASSERTs.
> 
> There is no need to 'mutate before validate'. If we are going to halt the
> system that makes manipulating corrupted pointers in memory irrelevant.
> 
> Move the state validation block above the pointer manipulation. This
> ensures the transaction is entirely valid before modifying the journal's
> internal lists, modernizing the function's logic and paving the way
> for future graceful degradation of these assertions.

Either you have a poetry gift or you should tell your AI agent to keep the
tone more to the point :). Anyway I think this is really just a pointless
churn as it doesn't really matter whether we crash the kernel one way or
another...

								Honza

> 
> Signed-off-by: Milos Nikic <nikic.milos@gmail.com>
> ---
>  fs/jbd2/checkpoint.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 1508e2f54462..c82b6bedd27b 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -703,6 +703,15 @@ void __jbd2_journal_drop_transaction(journal_t *journal, transaction_t *transact
>  {
>  	assert_spin_locked(&journal->j_list_lock);
>  
> +	J_ASSERT(transaction->t_state == T_FINISHED);
> +	J_ASSERT(transaction->t_buffers == NULL);
> +	J_ASSERT(transaction->t_forget == NULL);
> +	J_ASSERT(transaction->t_shadow_list == NULL);
> +	J_ASSERT(transaction->t_checkpoint_list == NULL);
> +	J_ASSERT(atomic_read(&transaction->t_updates) == 0);
> +	J_ASSERT(journal->j_committing_transaction != transaction);
> +	J_ASSERT(journal->j_running_transaction != transaction);
> +
>  	journal->j_shrink_transaction = NULL;
>  	if (transaction->t_cpnext) {
>  		transaction->t_cpnext->t_cpprev = transaction->t_cpprev;
> @@ -714,15 +723,6 @@ void __jbd2_journal_drop_transaction(journal_t *journal, transaction_t *transact
>  			journal->j_checkpoint_transactions = NULL;
>  	}
>  
> -	J_ASSERT(transaction->t_state == T_FINISHED);
> -	J_ASSERT(transaction->t_buffers == NULL);
> -	J_ASSERT(transaction->t_forget == NULL);
> -	J_ASSERT(transaction->t_shadow_list == NULL);
> -	J_ASSERT(transaction->t_checkpoint_list == NULL);
> -	J_ASSERT(atomic_read(&transaction->t_updates) == 0);
> -	J_ASSERT(journal->j_committing_transaction != transaction);
> -	J_ASSERT(journal->j_running_transaction != transaction);
> -
>  	trace_jbd2_drop_transaction(journal, transaction);
>  
>  	jbd2_debug(1, "Dropping transaction %d, all done\n", transaction->t_tid);
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR