* [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage
@ 2026-03-03 0:55 Milos Nikic
2026-03-03 0:55 ` [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer Milos Nikic
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Milos Nikic @ 2026-03-03 0:55 UTC (permalink / raw)
To: jack; +Cc: tytso, linux-ext4, linux-kernel, Milos Nikic
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 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 | 115 +++++++++++++++++++++++++++++++++---------
1 file changed, 91 insertions(+), 24 deletions(-)
--
2.53.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer 2026-03-03 0:55 [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage Milos Nikic @ 2026-03-03 0:55 ` Milos Nikic 2026-03-03 1:05 ` Andreas Dilger 2026-03-03 0:55 ` [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions Milos Nikic 2026-03-03 2:11 ` [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage yebin (H) 2 siblings, 1 reply; 6+ messages in thread From: Milos Nikic @ 2026-03-03 0:55 UTC (permalink / raw) To: jack; +Cc: tytso, linux-ext4, linux-kernel, Milos Nikic, Zhang Yi 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 <nikic.milos@gmail.com> Reviewed-by: Zhang Yi <yi.zhang@huawei.com> Reviewed-by: Jan Kara <jack@suse.cz> --- fs/jbd2/transaction.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index dca4b5d8aaaa..04d17a5f2a82 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer 2026-03-03 0:55 ` [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer Milos Nikic @ 2026-03-03 1:05 ` Andreas Dilger 0 siblings, 0 replies; 6+ messages in thread From: Andreas Dilger @ 2026-03-03 1:05 UTC (permalink / raw) To: Milos Nikic; +Cc: jack, tytso, linux-ext4, linux-kernel, Zhang Yi On Mar 2, 2026, at 17:55, Milos Nikic <nikic.milos@gmail.com> wrote: > > 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 <nikic.milos@gmail.com> > Reviewed-by: Zhang Yi <yi.zhang@huawei.com> > Reviewed-by: Jan Kara <jack@suse.cz> Reviewed-by: Andreas Dilger <adilger@dilger.ca <mailto:adilger@dilger.ca>> > --- > fs/jbd2/transaction.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c > index dca4b5d8aaaa..04d17a5f2a82 100644 > --- a/fs/jbd2/transaction.c > +++ b/fs/jbd2/transaction.c > @@ -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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions 2026-03-03 0:55 [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage Milos Nikic 2026-03-03 0:55 ` [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer Milos Nikic @ 2026-03-03 0:55 ` Milos Nikic 2026-03-03 6:16 ` kernel test robot 2026-03-03 2:11 ` [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage yebin (H) 2 siblings, 1 reply; 6+ messages in thread From: Milos Nikic @ 2026-03-03 0:55 UTC (permalink / raw) To: jack; +Cc: tytso, linux-ext4, linux-kernel, 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 <nikic.milos@gmail.com> --- fs/jbd2/transaction.c | 108 +++++++++++++++++++++++++++++++++--------- 1 file changed, 85 insertions(+), 23 deletions(-) diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index 04d17a5f2a82..3cb4e524e0a6 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -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 @@ -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)) { @@ -1531,13 +1558,15 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh) 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", + 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); + if (WARN_ON_ONCE(jh->b_transaction == transaction && + jh->b_jlist != BJ_Metadata)) { + ret = -EINVAL; + goto out_unlock_bh; + } spin_unlock(&jh->b_state_lock); } goto out; @@ -1636,7 +1665,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 +1707,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 +1741,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 +1784,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 +1806,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 +1856,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 +2182,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 +2698,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 +2734,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, -- 2.53.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions 2026-03-03 0:55 ` [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions Milos Nikic @ 2026-03-03 6:16 ` kernel test robot 0 siblings, 0 replies; 6+ messages in thread From: kernel test robot @ 2026-03-03 6:16 UTC (permalink / raw) To: Milos Nikic, jack Cc: llvm, oe-kbuild-all, tytso, linux-ext4, linux-kernel, Milos Nikic Hi Milos, kernel test robot noticed the following build warnings: [auto build test WARNING on brauner-vfs/vfs.all] [also build test WARNING on tytso-ext4/dev next-20260302] [cannot apply to linus/master v6.16-rc1] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Milos-Nikic/jbd2-gracefully-abort-instead-of-panicking-on-unlocked-buffer/20260303-085946 base: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all patch link: https://lore.kernel.org/r/20260303005502.337108-3-nikic.milos%40gmail.com patch subject: [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions config: x86_64-kexec (https://download.01.org/0day-ci/archive/20260303/202603030706.NpyA7pqe-lkp@intel.com/config) compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260303/202603030706.NpyA7pqe-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202603030706.NpyA7pqe-lkp@intel.com/ All warnings (new ones prefixed by >>): >> fs/jbd2/transaction.c:1547:11: warning: variable 'journal' is uninitialized when used here [-Wuninitialized] 1547 | journal->j_devname, jh->b_transaction, | ^~~~~~~ include/linux/printk.h:554:33: note: expanded from macro 'pr_err' 554 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__) | ^~~~~~~~~~~ include/linux/printk.h:511:60: note: expanded from macro 'printk' 511 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) | ^~~~~~~~~~~ include/linux/printk.h:483:19: note: expanded from macro 'printk_index_wrap' 483 | _p_func(_fmt, ##__VA_ARGS__); \ | ^~~~~~~~~~~ fs/jbd2/transaction.c:1520:20: note: initialize the variable 'journal' to silence this warning 1520 | journal_t *journal; | ^ | = NULL 1 warning generated. vim +/journal +1547 fs/jbd2/transaction.c 1493 1494 /** 1495 * jbd2_journal_dirty_metadata() - mark a buffer as containing dirty metadata 1496 * @handle: transaction to add buffer to. 1497 * @bh: buffer to mark 1498 * 1499 * mark dirty metadata which needs to be journaled as part of the current 1500 * transaction. 1501 * 1502 * The buffer must have previously had jbd2_journal_get_write_access() 1503 * called so that it has a valid journal_head attached to the buffer 1504 * head. 1505 * 1506 * The buffer is placed on the transaction's metadata list and is marked 1507 * as belonging to the transaction. 1508 * 1509 * Returns error number or 0 on success. 1510 * 1511 * Special care needs to be taken if the buffer already belongs to the 1512 * current committing transaction (in which case we should have frozen 1513 * data present for that commit). In that case, we don't relink the 1514 * buffer: that only gets done when the old transaction finally 1515 * completes its commit. 1516 */ 1517 int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh) 1518 { 1519 transaction_t *transaction = handle->h_transaction; 1520 journal_t *journal; 1521 struct journal_head *jh; 1522 int ret = 0; 1523 1524 if (!buffer_jbd(bh)) 1525 return -EUCLEAN; 1526 1527 /* 1528 * We don't grab jh reference here since the buffer must be part 1529 * of the running transaction. 1530 */ 1531 jh = bh2jh(bh); 1532 jbd2_debug(5, "journal_head %p\n", jh); 1533 JBUFFER_TRACE(jh, "entry"); 1534 1535 /* 1536 * This and the following assertions are unreliable since we may see jh 1537 * in inconsistent state unless we grab bh_state lock. But this is 1538 * crucial to catch bugs so let's do a reliable check until the 1539 * lockless handling is fully proven. 1540 */ 1541 if (data_race(jh->b_transaction != transaction && 1542 jh->b_next_transaction != transaction)) { 1543 spin_lock(&jh->b_state_lock); 1544 if (WARN_ON_ONCE(jh->b_transaction != transaction && 1545 jh->b_next_transaction != transaction)) { 1546 pr_err("JBD2: %s: assertion failure: b_transaction=%p transaction=%p b_next_transaction=%p\n", > 1547 journal->j_devname, jh->b_transaction, 1548 transaction, jh->b_next_transaction); 1549 ret = -EINVAL; 1550 goto out_unlock_bh; 1551 } 1552 spin_unlock(&jh->b_state_lock); 1553 } 1554 if (data_race(jh->b_modified == 1)) { 1555 /* If it's in our transaction it must be in BJ_Metadata list. */ 1556 if (data_race(jh->b_transaction == transaction && 1557 jh->b_jlist != BJ_Metadata)) { 1558 spin_lock(&jh->b_state_lock); 1559 if (jh->b_transaction == transaction && 1560 jh->b_jlist != BJ_Metadata) 1561 pr_err("JBD2: assertion failure: h_type=%u h_line_no=%u block_no=%llu jlist=%u\n", 1562 handle->h_type, handle->h_line_no, 1563 (unsigned long long) bh->b_blocknr, 1564 jh->b_jlist); 1565 if (WARN_ON_ONCE(jh->b_transaction == transaction && 1566 jh->b_jlist != BJ_Metadata)) { 1567 ret = -EINVAL; 1568 goto out_unlock_bh; 1569 } 1570 spin_unlock(&jh->b_state_lock); 1571 } 1572 goto out; 1573 } 1574 1575 spin_lock(&jh->b_state_lock); 1576 1577 if (is_handle_aborted(handle)) { 1578 /* 1579 * Check journal aborting with @jh->b_state_lock locked, 1580 * since 'jh->b_transaction' could be replaced with 1581 * 'jh->b_next_transaction' during old transaction 1582 * committing if journal aborted, which may fail 1583 * assertion on 'jh->b_frozen_data == NULL'. 1584 */ 1585 ret = -EROFS; 1586 goto out_unlock_bh; 1587 } 1588 1589 journal = transaction->t_journal; 1590 1591 if (jh->b_modified == 0) { 1592 /* 1593 * This buffer's got modified and becoming part 1594 * of the transaction. This needs to be done 1595 * once a transaction -bzzz 1596 */ 1597 if (WARN_ON_ONCE(jbd2_handle_buffer_credits(handle) <= 0)) { 1598 ret = -ENOSPC; 1599 goto out_unlock_bh; 1600 } 1601 jh->b_modified = 1; 1602 handle->h_total_credits--; 1603 } 1604 1605 /* 1606 * fastpath, to avoid expensive locking. If this buffer is already 1607 * on the running transaction's metadata list there is nothing to do. 1608 * Nobody can take it off again because there is a handle open. 1609 * I _think_ we're OK here with SMP barriers - a mistaken decision will 1610 * result in this test being false, so we go in and take the locks. 1611 */ 1612 if (jh->b_transaction == transaction && jh->b_jlist == BJ_Metadata) { 1613 JBUFFER_TRACE(jh, "fastpath"); 1614 if (unlikely(jh->b_transaction != 1615 journal->j_running_transaction)) { 1616 printk(KERN_ERR "JBD2: %s: " 1617 "jh->b_transaction (%llu, %p, %u) != " 1618 "journal->j_running_transaction (%p, %u)\n", 1619 journal->j_devname, 1620 (unsigned long long) bh->b_blocknr, 1621 jh->b_transaction, 1622 jh->b_transaction ? jh->b_transaction->t_tid : 0, 1623 journal->j_running_transaction, 1624 journal->j_running_transaction ? 1625 journal->j_running_transaction->t_tid : 0); 1626 ret = -EINVAL; 1627 } 1628 goto out_unlock_bh; 1629 } 1630 1631 set_buffer_jbddirty(bh); 1632 1633 /* 1634 * Metadata already on the current transaction list doesn't 1635 * need to be filed. Metadata on another transaction's list must 1636 * be committing, and will be refiled once the commit completes: 1637 * leave it alone for now. 1638 */ 1639 if (jh->b_transaction != transaction) { 1640 JBUFFER_TRACE(jh, "already on other transaction"); 1641 if (unlikely(((jh->b_transaction != 1642 journal->j_committing_transaction)) || 1643 (jh->b_next_transaction != transaction))) { 1644 printk(KERN_ERR "jbd2_journal_dirty_metadata: %s: " 1645 "bad jh for block %llu: " 1646 "transaction (%p, %u), " 1647 "jh->b_transaction (%p, %u), " 1648 "jh->b_next_transaction (%p, %u), jlist %u\n", 1649 journal->j_devname, 1650 (unsigned long long) bh->b_blocknr, 1651 transaction, transaction->t_tid, 1652 jh->b_transaction, 1653 jh->b_transaction ? 1654 jh->b_transaction->t_tid : 0, 1655 jh->b_next_transaction, 1656 jh->b_next_transaction ? 1657 jh->b_next_transaction->t_tid : 0, 1658 jh->b_jlist); 1659 WARN_ON(1); 1660 ret = -EINVAL; 1661 } 1662 /* And this case is illegal: we can't reuse another 1663 * transaction's data buffer, ever. */ 1664 goto out_unlock_bh; 1665 } 1666 1667 /* That test should have eliminated the following case: */ 1668 if (WARN_ON_ONCE(jh->b_frozen_data)) { 1669 ret = -EINVAL; 1670 goto out_unlock_bh; 1671 } 1672 1673 JBUFFER_TRACE(jh, "file as BJ_Metadata"); 1674 spin_lock(&journal->j_list_lock); 1675 __jbd2_journal_file_buffer(jh, transaction, BJ_Metadata); 1676 spin_unlock(&journal->j_list_lock); 1677 out_unlock_bh: 1678 spin_unlock(&jh->b_state_lock); 1679 out: 1680 JBUFFER_TRACE(jh, "exit"); 1681 return ret; 1682 } 1683 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage 2026-03-03 0:55 [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage Milos Nikic 2026-03-03 0:55 ` [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer Milos Nikic 2026-03-03 0:55 ` [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions Milos Nikic @ 2026-03-03 2:11 ` yebin (H) 2 siblings, 0 replies; 6+ messages in thread From: yebin (H) @ 2026-03-03 2:11 UTC (permalink / raw) To: jack; +Cc: tytso, linux-ext4, linux-kernel The macro `J_ASSERT_JH` is a rather troublesome implementation. There are numerous calls to `J_ASSERT_JH` within `jbd2_journal_commit_transaction()`, and after compilation, these may all jump to the same address for execution, making it difficult to determine exactly where the assertion is being triggered. If there is a functional issue in just a single file system, using `BUG_ON` to handle it seems a bit too aggressive. I wonder if you all have any good ideas or suggestions. On 2026/3/3 8:55, Milos Nikic wrote: > 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 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 | 115 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 91 insertions(+), 24 deletions(-) > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-03-03 6:17 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-03-03 0:55 [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage Milos Nikic 2026-03-03 0:55 ` [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer Milos Nikic 2026-03-03 1:05 ` Andreas Dilger 2026-03-03 0:55 ` [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions Milos Nikic 2026-03-03 6:16 ` kernel test robot 2026-03-03 2:11 ` [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage yebin (H)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox