jbd2: gracefully abort instead of panicking on unlocked buffer

[PATCH] jbd2: gracefully abort instead of panicking on unlocked buffer

Posted by Milos Nikic 1 month, 2 weeks ago

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>
---
 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

Re: [PATCH] jbd2: gracefully abort instead of panicking on unlocked buffer

Posted by Jan Kara 1 month, 2 weeks ago

On Sun 01-03-26 16:31:35, Milos Nikic 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>

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 | 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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] jbd2: gracefully abort instead of panicking on unlocked buffer

Posted by Milos Nikic 1 month, 2 weeks ago

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

Re: [PATCH] jbd2: gracefully abort instead of panicking on unlocked buffer

Posted by Zhang Yi 1 month, 2 weeks ago

On 3/2/2026 8:31 AM, Milos Nikic 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>

Thank you for the patch, this makes sense to me.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

> ---
>  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) {
>  		/*