Skip to contentSkip to conversation

jbd2: audit and convert legacy J_ASSERT usage

PATCH Applied Milos Nikic 3 participants 6 messages Mar 4 – Apr 10 Journalling layer for block devices (JBD2)

lore
Share & tools
lore

Milos Nikic

author
#0 lore raw

v5 cover jbd2: audit and convert legacy J_ASSERT usage

Hello Jan and the ext4 team,

This patch series follows up on the previous discussion regarding converting hard J_ASSERT panics into graceful journal aborts.

In v1, we addressed a specific panic on unlock. Per Jan's suggestion, I have audited fs/jbd2/transaction.c for other low-hanging fruit where state machine invariants are enforced by J_ASSERT inside functions that natively support error returns.

Changes in v5:
Patch 2: Folded a redundant if check into the WARN_ON_ONCE block in jbd2_journal_dirty_metadata per Andreas's suggestion. Carried over Reviewed-by tags from Jan, Andreas and Zhang.

Changes in v4:
Patch 2: Fixed a build test WARNING by initializing a variable journal earlier in jbd2_journal_dirty_metadata().

Changes in v3:

Patch 2: Added pr_err() statements inside the ambiguous WARN_ON_ONCE() blocks (where multiple conditions are checked via logical OR/AND) to explicitly dump the b_transaction, b_next_transaction, and j_committing_transaction pointers. This provides necessary context for debugging state machine corruptions from the dmesg stack trace.

Changes in v2:

Patch 1: Unmodified from v1. Collected Reviewed-by tags.

Patch 2: New patch resulting from the broader audit. Systematically replaces J_ASSERTs with WARN_ON_ONCE and graceful -EINVAL returns across 6 core transaction lifecycle functions. Careful attention was paid to ensuring spinlocks are safely dropped before triggering jbd2_journal_abort(), and no memory is leaked on the error paths.

Milos Nikic (2):
jbd2: gracefully abort instead of panicking on unlocked buffer
jbd2: gracefully abort on transaction state corruptions

 fs/jbd2/transaction.c | 121 ++++++++++++++++++++++++++++++++----------
 1 file changed, 92 insertions(+), 29 deletions(-)

--
2.53.0

author
#1 lore raw

PATCH v5 1/2 jbd2: gracefully abort instead of panicking on unlocked buffer

In jbd2_journal_get_create_access(), if the caller passes an unlocked buffer, the code currently triggers a fatal J_ASSERT.

While an unlocked buffer here is a clear API violation and a bug in the caller, crashing the entire system is an overly severe response. It brings down the whole machine for a localized filesystem inconsistency.

Replace the J_ASSERT with a WARN_ON_ONCE to capture the offending caller's stack trace, and return an error (-EINVAL). This allows the journal to gracefully abort the transaction, protecting data integrity without causing a kernel panic.

Signed-off-by Milos Nikic <[email protected]>
Reviewed-by Zhang Yi <[email protected]>
Reviewed-by Jan Kara <[email protected]>
Reviewed-by Andreas Dilger <[email protected]>
---
 fs/jbd2/transaction.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/jbd2/transaction.c +6 -1
index dca4b5d8aaaa..04d17a5f2a82 100644
@@ -1302,7 +1302,12 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh)
 		goto out;
 	}
 
-	J_ASSERT_JH(jh, buffer_locked(jh2bh(jh)));
+	if (WARN_ON_ONCE(!buffer_locked(jh2bh(jh)))) {
+		err = -EINVAL;
+		spin_unlock(&jh->b_state_lock);
+		jbd2_journal_abort(journal, err);
+		goto out;
+	}
 
 	if (jh->b_transaction == NULL) {
 		/*
Signature · 1 line
2.53.0
author
#2 lore raw

Re: [PATCH v5 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer

Hello,

Just sending a gentle ping on this v5 series from March 4th. It looks like both patches have collected Reviewed-by tags from Jan, Andreas and Zhang.

Please let me know if there is anything else you need from my side, or if this is good to be queued up in the ext4 tree for the next merge window.

Thanks, Milos

Quoted 42 lines · Milos Nikic

In jbd2_journal_get_create_access(), if the caller passes an unlocked buffer, the code currently triggers a fatal J_ASSERT.

While an unlocked buffer here is a clear API violation and a bug in the caller, crashing the entire system is an overly severe response. It brings down the whole machine for a localized filesystem inconsistency.

Replace the J_ASSERT with a WARN_ON_ONCE to capture the offending caller's stack trace, and return an error (-EINVAL). This allows the journal to gracefully abort the transaction, protecting data integrity without causing a kernel panic.

Signed-off-by Milos Nikic <[email protected]>
Reviewed-by Zhang Yi <[email protected]>
Reviewed-by Jan Kara <[email protected]>
Reviewed-by Andreas Dilger <[email protected]>
---
 fs/jbd2/transaction.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/jbd2/transaction.c +6 -2
index dca4b5d8aaaa..04d17a5f2a82 100644
@@ -1302,7 +1302,12 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh)
                goto out;
        }
 
-       J_ASSERT_JH(jh, buffer_locked(jh2bh(jh)));
+       if (WARN_ON_ONCE(!buffer_locked(jh2bh(jh)))) {
+               err = -EINVAL;
+               spin_unlock(&jh->b_state_lock);
+               jbd2_journal_abort(journal, err);
+               goto out;
+       }
 
        if (jh->b_transaction == NULL) {
                /*
--

2.53.0

#3 lore raw

Re: [PATCH v5 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer

Hello!

Quoted 7 lines · Milos Nikic

Just sending a gentle ping on this v5 series from March 4th. It looks like both patches have collected Reviewed-by tags from Jan, Andreas and Zhang.

Please let me know if there is anything else you need from my side, or if this is good to be queued up in the ext4 tree for the next merge window.

Ted Tso is generally picking jbd2 patches. Since this is a cleanup, I expect him to pick up this patch for the next merge window - i.e., in a week or two :)

Honza

Quoted 46 lines

Thanks, Milos

Quoted 42 lines · Milos Nikic

In jbd2_journal_get_create_access(), if the caller passes an unlocked buffer, the code currently triggers a fatal J_ASSERT.

While an unlocked buffer here is a clear API violation and a bug in the caller, crashing the entire system is an overly severe response. It brings down the whole machine for a localized filesystem inconsistency.

Replace the J_ASSERT with a WARN_ON_ONCE to capture the offending caller's stack trace, and return an error (-EINVAL). This allows the journal to gracefully abort the transaction, protecting data integrity without causing a kernel panic.

Signed-off-by Milos Nikic <[email protected]>
Reviewed-by Zhang Yi <[email protected]>
Reviewed-by Jan Kara <[email protected]>
Reviewed-by Andreas Dilger <[email protected]>
---
 fs/jbd2/transaction.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/jbd2/transaction.c +6 -2
index dca4b5d8aaaa..04d17a5f2a82 100644
@@ -1302,7 +1302,12 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh)
                goto out;
        }
 
-       J_ASSERT_JH(jh, buffer_locked(jh2bh(jh)));
+       if (WARN_ON_ONCE(!buffer_locked(jh2bh(jh)))) {
+               err = -EINVAL;
+               spin_unlock(&jh->b_state_lock);
+               jbd2_journal_abort(journal, err);
+               goto out;
+       }
 
        if (jh->b_transaction == NULL) {
                /*
--

2.53.0

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
author
#4 lore raw

PATCH v5 2/2 jbd2: gracefully abort on transaction state corruptions

In reply to Milos Nikic

Auditing the jbd2 codebase reveals several legacy J_ASSERT calls that enforce internal state machine invariants (e.g., verifying jh->b_transaction or jh->b_next_transaction pointers).

When these invariants are broken, the journal is in a corrupted state. However, triggering a fatal panic brings down the entire system for a localized filesystem error.

This patch targets a specific class of these asserts: those residing inside functions that natively return integer error codes, booleans, or error pointers. It replaces the hard J_ASSERTs with WARN_ON_ONCE to capture the offending stack trace, safely drops any held locks, gracefully aborts the journal, and returns -EINVAL.

This prevents a catastrophic kernel panic while ensuring the corrupted journal state is safely contained and upstream callers (like ext4 or ocfs2) can gracefully handle the aborted handle.

Functions modified in fs/jbd2/transaction.c:
- jbd2__journal_start()
- do_get_write_access()
- jbd2_journal_dirty_metadata()
- jbd2_journal_forget()
- jbd2_journal_try_to_free_buffers()
- jbd2_journal_file_inode()

Signed-off-by Milos Nikic <[email protected]>
Reviewed-by Zhang Yi <[email protected]>
Reviewed-by Jan Kara <[email protected]>
Reviewed-by Andreas Dilger <[email protected]>
---
 fs/jbd2/transaction.c | 114 +++++++++++++++++++++++++++++++-----------
 1 file changed, 86 insertions(+), 28 deletions(-)
fs/jbd2/transaction.c +86 -28
index 04d17a5f2a82..02cb87dc6fa8 100644
@@ -474,7 +474,8 @@ handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int rsv_blocks,
 		return ERR_PTR(-EROFS);
 
 	if (handle) {
-		J_ASSERT(handle->h_transaction->t_journal == journal);
+		if (WARN_ON_ONCE(handle->h_transaction->t_journal != journal))
+			return ERR_PTR(-EINVAL);
 		handle->h_ref++;
 		return handle;
 	}

@@ -1036,7 +1037,13 @@ do_get_write_access(handle_t *handle, struct journal_head *jh,
 	 */
 	if (!jh->b_transaction) {
 		JBUFFER_TRACE(jh, "no transaction");
-		J_ASSERT_JH(jh, !jh->b_next_transaction);
+		if (WARN_ON_ONCE(jh->b_next_transaction)) {
+			spin_unlock(&jh->b_state_lock);
+			unlock_buffer(bh);
+			error = -EINVAL;
+			jbd2_journal_abort(journal, error);
+			goto out;
+		}
 		JBUFFER_TRACE(jh, "file as BJ_Reserved");
 		/*
 		 * Make sure all stores to jh (b_modified, b_frozen_data) are

@@ -1069,13 +1076,27 @@ do_get_write_access(handle_t *handle, struct journal_head *jh,
 	 */
 	if (jh->b_frozen_data) {
 		JBUFFER_TRACE(jh, "has frozen data");
-		J_ASSERT_JH(jh, jh->b_next_transaction == NULL);
+		if (WARN_ON_ONCE(jh->b_next_transaction)) {
+			spin_unlock(&jh->b_state_lock);
+			error = -EINVAL;
+			jbd2_journal_abort(journal, error);
+			goto out;
+		}
 		goto attach_next;
 	}
 
 	JBUFFER_TRACE(jh, "owned by older transaction");
-	J_ASSERT_JH(jh, jh->b_next_transaction == NULL);
-	J_ASSERT_JH(jh, jh->b_transaction == journal->j_committing_transaction);
+	if (WARN_ON_ONCE(jh->b_next_transaction ||
+			 jh->b_transaction !=
+			 journal->j_committing_transaction)) {
+		pr_err("JBD2: %s: assertion failure: b_next_transaction=%p b_transaction=%p j_committing_transaction=%p\n",
+		       journal->j_devname, jh->b_next_transaction,
+		       jh->b_transaction, journal->j_committing_transaction);
+		spin_unlock(&jh->b_state_lock);
+		error = -EINVAL;
+		jbd2_journal_abort(journal, error);
+		goto out;
+	}
 
 	/*
 	 * There is one case we have to be very careful about.  If the

@@ -1496,7 +1517,7 @@ void jbd2_buffer_abort_trigger(struct journal_head *jh,
 int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
 {
 	transaction_t *transaction = handle->h_transaction;
-	journal_t *journal;
+	journal_t *journal = transaction->t_journal;
 	struct journal_head *jh;
 	int ret = 0;
 

@@ -1520,8 +1541,14 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
 	if (data_race(jh->b_transaction != transaction &&
 	    jh->b_next_transaction != transaction)) {
 		spin_lock(&jh->b_state_lock);
-		J_ASSERT_JH(jh, jh->b_transaction == transaction ||
-				jh->b_next_transaction == transaction);
+		if (WARN_ON_ONCE(jh->b_transaction != transaction &&
+				 jh->b_next_transaction != transaction)) {
+			pr_err("JBD2: %s: assertion failure: b_transaction=%p transaction=%p b_next_transaction=%p\n",
+			       journal->j_devname, jh->b_transaction,
+			       transaction, jh->b_next_transaction);
+			ret = -EINVAL;
+			goto out_unlock_bh;
+		}
 		spin_unlock(&jh->b_state_lock);
 	}
 	if (data_race(jh->b_modified == 1)) {

@@ -1529,15 +1556,15 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
 		if (data_race(jh->b_transaction == transaction &&
 		    jh->b_jlist != BJ_Metadata)) {
 			spin_lock(&jh->b_state_lock);
-			if (jh->b_transaction == transaction &&
-			    jh->b_jlist != BJ_Metadata)
-				pr_err("JBD2: assertion failure: h_type=%u "
-				       "h_line_no=%u block_no=%llu jlist=%u\n",
+			if (WARN_ON_ONCE(jh->b_transaction == transaction &&
+					 jh->b_jlist != BJ_Metadata)) {
+				pr_err("JBD2: assertion failure: h_type=%u h_line_no=%u block_no=%llu jlist=%u\n",
 				       handle->h_type, handle->h_line_no,
 				       (unsigned long long) bh->b_blocknr,
 				       jh->b_jlist);
-			J_ASSERT_JH(jh, jh->b_transaction != transaction ||
-					jh->b_jlist == BJ_Metadata);
+				ret = -EINVAL;
+				goto out_unlock_bh;
+			}
 			spin_unlock(&jh->b_state_lock);
 		}
 		goto out;

@@ -1557,8 +1584,6 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
 		goto out_unlock_bh;
 	}
 
-	journal = transaction->t_journal;
-
 	if (jh->b_modified == 0) {
 		/*
 		 * This buffer's got modified and becoming part

@@ -1636,7 +1661,10 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
 	}
 
 	/* That test should have eliminated the following case: */
-	J_ASSERT_JH(jh, jh->b_frozen_data == NULL);
+	if (WARN_ON_ONCE(jh->b_frozen_data)) {
+		ret = -EINVAL;
+		goto out_unlock_bh;
+	}
 
 	JBUFFER_TRACE(jh, "file as BJ_Metadata");
 	spin_lock(&journal->j_list_lock);

@@ -1675,6 +1703,7 @@ int jbd2_journal_forget(handle_t *handle, struct buffer_head *bh)
 	int err = 0;
 	int was_modified = 0;
 	int wait_for_writeback = 0;
+	int abort_journal = 0;
 
 	if (is_handle_aborted(handle))
 		return -EROFS;

@@ -1708,7 +1737,11 @@ int jbd2_journal_forget(handle_t *handle, struct buffer_head *bh)
 	jh->b_modified = 0;
 
 	if (jh->b_transaction == transaction) {
-		J_ASSERT_JH(jh, !jh->b_frozen_data);
+		if (WARN_ON_ONCE(jh->b_frozen_data)) {
+			err = -EINVAL;
+			abort_journal = 1;
+			goto drop;
+		}
 
 		/* If we are forgetting a buffer which is already part
 		 * of this transaction, then we can just drop it from

@@ -1747,8 +1780,11 @@ int jbd2_journal_forget(handle_t *handle, struct buffer_head *bh)
 		}
 		spin_unlock(&journal->j_list_lock);
 	} else if (jh->b_transaction) {
-		J_ASSERT_JH(jh, (jh->b_transaction ==
-				 journal->j_committing_transaction));
+		if (WARN_ON_ONCE(jh->b_transaction != journal->j_committing_transaction)) {
+			err = -EINVAL;
+			abort_journal = 1;
+			goto drop;
+		}
 		/* However, if the buffer is still owned by a prior
 		 * (committing) transaction, we can't drop it yet... */
 		JBUFFER_TRACE(jh, "belongs to older transaction");

@@ -1766,7 +1802,11 @@ int jbd2_journal_forget(handle_t *handle, struct buffer_head *bh)
 			jh->b_next_transaction = transaction;
 			spin_unlock(&journal->j_list_lock);
 		} else {
-			J_ASSERT(jh->b_next_transaction == transaction);
+			if (WARN_ON_ONCE(jh->b_next_transaction != transaction)) {
+				err = -EINVAL;
+				abort_journal = 1;
+				goto drop;
+			}
 
 			/*
 			 * only drop a reference if this transaction modified

@@ -1812,6 +1852,8 @@ int jbd2_journal_forget(handle_t *handle, struct buffer_head *bh)
 drop:
 	__brelse(bh);
 	spin_unlock(&jh->b_state_lock);
+	if (abort_journal)
+		jbd2_journal_abort(journal, err);
 	if (wait_for_writeback)
 		wait_on_buffer(bh);
 	jbd2_journal_put_journal_head(jh);

@@ -2136,7 +2178,8 @@ bool jbd2_journal_try_to_free_buffers(journal_t *journal, struct folio *folio)
 	struct buffer_head *bh;
 	bool ret = false;
 
-	J_ASSERT(folio_test_locked(folio));
+	if (WARN_ON_ONCE(!folio_test_locked(folio)))
+		return false;
 
 	head = folio_buffers(folio);
 	bh = head;

@@ -2651,6 +2694,8 @@ static int jbd2_journal_file_inode(handle_t *handle, struct jbd2_inode *jinode,
 {
 	transaction_t *transaction = handle->h_transaction;
 	journal_t *journal;
+	int err = 0;
+	int abort_transaction = 0;
 
 	if (is_handle_aborted(handle))
 		return -EROFS;

@@ -2685,20 +2730,33 @@ static int jbd2_journal_file_inode(handle_t *handle, struct jbd2_inode *jinode,
 	/* On some different transaction's list - should be
 	 * the committing one */
 	if (jinode->i_transaction) {
-		J_ASSERT(jinode->i_next_transaction == NULL);
-		J_ASSERT(jinode->i_transaction ==
-					journal->j_committing_transaction);
+		if (WARN_ON_ONCE(jinode->i_next_transaction ||
+				 jinode->i_transaction !=
+				 journal->j_committing_transaction)) {
+			pr_err("JBD2: %s: assertion failure: i_next_transaction=%p i_transaction=%p j_committing_transaction=%p\n",
+			       journal->j_devname, jinode->i_next_transaction,
+			       jinode->i_transaction,
+			       journal->j_committing_transaction);
+			err = -EINVAL;
+			abort_transaction = 1;
+			goto done;
+		}
 		jinode->i_next_transaction = transaction;
 		goto done;
 	}
 	/* Not on any transaction list... */
-	J_ASSERT(!jinode->i_next_transaction);
+	if (WARN_ON_ONCE(jinode->i_next_transaction)) {
+		err = -EINVAL;
+		abort_transaction = 1;
+		goto done;
+	}
 	jinode->i_transaction = transaction;
 	list_add(&jinode->i_list, &transaction->t_inode_list);
 done:
 	spin_unlock(&journal->j_list_lock);
-
-	return 0;
+	if (abort_transaction)
+		jbd2_journal_abort(journal, err);
+	return err;
 }
 
 int jbd2_journal_inode_ranged_write(handle_t *handle,
Signature · 1 line
2.53.0
maintainer Applied
#5 lore raw
In reply to Milos Nikic
Quoted 9 lines · Milos Nikic

This patch series follows up on the previous discussion regarding converting hard J_ASSERT panics into graceful journal aborts.

In v1, we addressed a specific panic on unlock. Per Jan's suggestion, I have audited fs/jbd2/transaction.c for other low-hanging fruit where state machine invariants are enforced by J_ASSERT inside functions that natively support error returns.

[...]

Applied, thanks!

[1/2] jbd2: gracefully abort instead of panicking on unlocked buffer

      commit: 64924362f833fd15d75d2b8fc771eff9646c0933
[2/2] jbd2: gracefully abort on transaction state corruptions
      commit: f7fc28b014ebb00796f99f12f0583caab23276e3

Best regards,

Signature · 1 line
Theodore Ts'o <[email protected]>