jbd2: gracefully abort instead of panicking on unlocked buffer
PATCH 1/1 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.
--- fs/jbd2/transaction.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
fs/jbd2/transaction.c +6 -1
@@ -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
Quoted 7 lines · Milos Nikic
In
jbd2_journal_get_create_access(), if the caller passes an unlocked buffer, the code currently triggers a fatalJ_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.
Quoted 6 lines
Replace the
J_ASSERTwith aWARN_ON_ONCEto 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]>
Thank you for the patch, this makes sense to me.
Quoted 22 lines
--- fs/jbd2/transaction.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)fs/jbd2/transaction.c +6
-1@@ -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 ) {/*
Quoted 7 lines · Milos Nikic
In
jbd2_journal_get_create_access(), if the caller passes an unlocked buffer, the code currently triggers a fatalJ_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.
Quoted 6 lines
Replace the
J_ASSERTwith aWARN_ON_ONCEto 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]>
In principle I'm fine with this however we have lots of similar asserts in the code. So how is this one special? Did you somehow trigger it?
Honza
Quoted 25 lines
--- fs/jbd2/transaction.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)fs/jbd2/transaction.c +6
-1@@ -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
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
A message that is not in the archive (sent off-list or not yet received): CAOeJtk_JmASoSZQ_Y=qY155vQCXGnpqtXy8EPc3HA5yj4QFsJQ@mail.gmail.com
Jan Kara
Reviewed-byQuoted 17 lines · Milos Nikic
Hi Jan,
No, I didn't trigger this in the wild. I have been auditing jbd2 for
J_ASSERTusage to see where we could proactively swap hard panics for graceful journal aborts.
You are right that there are many similar asserts, but I focused on this one because it belongs to a specific, easily-actionable group: it resides in a function that already returns an error code (int).Many of the other
J_ASSERTsare buried inside void functions. Converting those to return errors would require cascading API changes and rewriting caller error-handling paths across the subsystem, which is a much bigger and riskier lift.
My goal was just to target the "low-hanging fruit"—the asserts where the function signature already supports returning an error (-EINVAL/-EIO) and aborting the journal safely without changing the API.
Quoted 3 lines
If you are open to it, I can audit the codebase for the rest of the asserts that fit this exact profile and submit them.
Would you prefer them grouped into a single patch, or a short series?
OK, thanks for explanation. So if you can submit a patch addressing these easy cases (possibly split to one patch per file in case the patch would be too big) that would be good. For this patch feel free to add:
Honza
Quoted 58 lines
Thanks,
MilosQuoted 52 lines · Jan Kara
Quoted 5 lines · Milos Nikic
In
jbd2_journal_get_create_access(), if the caller passes an unlocked buffer, the code currently triggers a fatalJ_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
Quoted 3 lines
down the whole machine for a localized filesystem inconsistency.
Replace the
J_ASSERTwith aWARN_ON_ONCEto capture the offendingcaller's
Quoted 5 lines
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]>In principle I'm fine with this however we have lots of similar asserts in the code. So how is this one special? Did you somehow trigger it?
Honza
fs/jbd2/transaction.c int jbd2_journal_get_create_access(handle_t
Quoted 9 lines
--- fs/jbd2/transaction.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)fs/jbd2/transaction.c +0
-0@@ -1302,7 +1302,12 @@ int jbd2_journal_get_create_access(handle_t*handle, struct
buffer_head*bh)Quoted 14 lines
fs/jbd2/transaction.c +6
-2goto 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 ) {/* -- Quoted 2 lines
2.53.0
--
Jan Kara <[email protected]>
SUSE Labs, CR
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Hi Jan,
No, I didn't trigger this in the wild. I have been auditing jbd2 for J_ASSERT usage to see where we could proactively swap hard panics for graceful journal aborts.
You are right that there are many similar asserts, but I focused on this one because it belongs to a specific, easily-actionable group: it resides in a function that already returns an error code (int).
Some of the other J_ASSERTs are buried inside void functions. Converting those to return errors would require cascading API changes and rewriting caller error-handling paths across the subsystem, which is a much bigger and riskier lift.
My goal was just to target the "low-hanging fruit" - the asserts where the function signature already supports returning an error (-EINVAL/-EIO) and aborting the journal safely without changing the API.
If you are open to it, I can audit the codebase for the rest of the asserts that fit this exact profile and submit them. Would you prefer them grouped into a single patch, or a short series?
Thanks,
Milos