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

[Jan Kara]

On Mon 02-03-26 07:58:41, Milos Nikic wrote:
> 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).
> 
> Many 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?

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:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> 
> Thanks,
> Milos
> 
> On Mon, Mar 2, 2026 at 5:08 AM Jan Kara <jack@xxxxxxx> wrote:
> 
> > 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@xxxxxxxxx>
> >
> > 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@xxxxxxxx>
> > SUSE Labs, CR
> >
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR