Re: [PATCH v2 03/21] jbd2: point the shadow buffer at the frozen data directly

[Chris S]

Thank you for fixing this. I appreciate it. Looks the problem is I
didn't run it in linux-next to check.
Sorry about that. Appreciate your help!

Thanks,
Chao

On Mon, Aug 31, 2026 at 11:37 PM Joseph Qi <jiangqi903@xxxxxxxxx> wrote:
>
>
>
> On 8/7/26 12:58 AM, Chao Shi wrote:
> > When a metadata buffer has to be copied out before it can be journalled,
> > jbd2_journal_write_metadata_buffer() writes jh->b_frozen_data rather than
> > the page cache copy.  b_frozen_data is kmalloc()ed, so folio_set_bh() makes
> > the shadow buffer point at a slab folio.
> >
> > That is not something the buffer_head layer can reason about.  A slab folio
> > overloads ->mapping, so a shadow buffer looks like it belongs to an
> > address_space when it does not.  buffer_set_crypto_ctx() already has to
> > work around this, and it is the reason mark_buffer_write_io_error() cannot
> > be called on a shadow buffer today.
> >
> > Point the shadow buffer at the frozen data itself instead: leave b_folio
> > NULL, which it already is out of alloc_buffer_head(), and set b_data.  The
> > previous patch taught fs/buffer.c to submit such a buffer.  folio_set_bh()
> > is now needed on only one path - the one that journals the page cache copy
> > directly - so it moves there, and new_folio, new_offset and the flag that
> > used to pick between them all go away.
> >
> > The two commit-path checksum helpers reach the shadow buffer's contents
> > through a new kmap_local_bh()/kunmap_local_bh() pair, which handle a buffer
> > with or without a folio.  Memory outside the page cache is always mapped,
> > so for those there is nothing to map or unmap.  Mapping it anyway would be
> > worse than pointless: with CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP,
> > kmap_local_page() hands back a one page mapping even for such memory, which
> > is not enough for a buffer bigger than a page.
> >
> > Tested with ext4 mounted data=journal,journal_checksum on a metadata_csum
> > filesystem, writing files whose every block begins with the JBD2 magic so
> > that escaping forces the copy-out, then crashing with sysrq-b without
> > unmounting and replaying the journal on the next mount.  Recovery
> > completed, the file contents matched, e2fsck -fn was clean, and an
> > instrumented build confirmed the b_folio == NULL path was taken.
> >
> > Suggested-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> > Acked-by: Weidong Zhu <weizhu@xxxxxxx>
> > Signed-off-by: Chao Shi <coshi036@xxxxxxxxx>
> > ---
> >  fs/jbd2/commit.c            |  8 ++++----
> >  fs/jbd2/journal.c           | 29 +++++++++++++++++------------
> >  include/linux/buffer_head.h | 29 +++++++++++++++++++++++++++++
> >  3 files changed, 50 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> > index 3029cb6f6d64..0c85af91f9b2 100644
> > --- a/fs/jbd2/commit.c
> > +++ b/fs/jbd2/commit.c
> > @@ -330,9 +330,9 @@ static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)
> >       char *addr;
> >       __u32 checksum;
> >
> > -     addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
> > +     addr = kmap_local_bh(bh);
> >       checksum = crc32_be(crc32_sum, addr, bh->b_size);
> > -     kunmap_local(addr);
> > +     kunmap_local_bh(bh, addr);
> >
> >       return checksum;
> >  }
> > @@ -357,10 +357,10 @@ static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
> >               return;
> >
> >       seq = cpu_to_be32(sequence);
> > -     addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
> > +     addr = kmap_local_bh(bh);
> >       csum32 = jbd2_chksum(j->j_csum_seed, (__u8 *)&seq, sizeof(seq));
> >       csum32 = jbd2_chksum(csum32, addr, bh->b_size);
> > -     kunmap_local(addr);
> > +     kunmap_local_bh(bh, addr);
> >
> >       if (jbd2_has_feature_csum3(j))
> >               tag3->t_checksum = cpu_to_be32(csum32);
> > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> > index 09efa337649e..6e05dc47e20a 100644
> > --- a/fs/jbd2/journal.c
> > +++ b/fs/jbd2/journal.c
> > @@ -327,8 +327,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> >  {
> >       int do_escape = 0;
> >       struct buffer_head *new_bh;
> > -     struct folio *new_folio;
> > -     unsigned int new_offset;
> >       struct buffer_head *bh_in = jh2bh(jh_in);
> >       journal_t *journal = transaction->t_journal;
> >
> > @@ -348,24 +346,31 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> >       /* keep subsequent assertions sane */
> >       atomic_set(&new_bh->b_count, 1);
> >
> > +     /*
> > +      * b_frozen_data is slab memory, not page cache, so when we use it the
> > +      * shadow buffer gets no folio at all: b_folio stays NULL from the
> > +      * allocation and b_data points straight at the copy.  Pointing it at
> > +      * the slab folio instead would hand its overloaded ->mapping to
> > +      * anything that goes looking for an address_space.
> > +      */
> > +
> >       spin_lock(&jh_in->b_state_lock);
> >       /*
> >        * If a new transaction has already done a buffer copy-out, then
> >        * we use that version of the data for the commit.
> >        */
> >       if (jh_in->b_frozen_data) {
> > -             new_folio = virt_to_folio(jh_in->b_frozen_data);
> > -             new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
> >               do_escape = jbd2_data_needs_escaping(jh_in->b_frozen_data);
> >               if (do_escape)
> >                       jbd2_data_do_escape(jh_in->b_frozen_data);
> > +             new_bh->b_data = jh_in->b_frozen_data;
> >       } else {
> > +             struct folio *folio = bh_in->b_folio;
> > +             unsigned int offset = offset_in_folio(folio, bh_in->b_data);
> >               char *tmp;
> >               char *mapped_data;
> >
> > -             new_folio = bh_in->b_folio;
> > -             new_offset = offset_in_folio(new_folio, bh_in->b_data);
> > -             mapped_data = kmap_local_folio(new_folio, new_offset);
> > +             mapped_data = kmap_local_folio(folio, offset);
> >               /*
> >                * Fire data frozen trigger if data already wasn't frozen. Do
> >                * this before checking for escaping, as the trigger may modify
> > @@ -379,8 +384,10 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> >               /*
> >                * Do we need to do a data copy?
> >                */
> > -             if (!do_escape)
> > +             if (!do_escape) {
> > +                     folio_set_bh(new_bh, folio, offset);
> >                       goto escape_done;
> > +             }
> >
> >               spin_unlock(&jh_in->b_state_lock);
> >               tmp = kmalloc(bh_in->b_size, GFP_NOFS | __GFP_NOFAIL);
> > @@ -391,7 +398,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> >               }
> >
> >               jh_in->b_frozen_data = tmp;
> > -             memcpy_from_folio(tmp, new_folio, new_offset, bh_in->b_size);
> > +             memcpy_from_folio(tmp, folio, offset, bh_in->b_size);
> >               /*
> >                * This isn't strictly necessary, as we're using frozen
> >                * data for the escaping, but it keeps consistency with
> > @@ -400,13 +407,11 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> >               jh_in->b_frozen_triggers = jh_in->b_triggers;
> >
> >  copy_done:
> > -             new_folio = virt_to_folio(jh_in->b_frozen_data);
> > -             new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
> >               jbd2_data_do_escape(jh_in->b_frozen_data);
> > +             new_bh->b_data = jh_in->b_frozen_data;
> >       }
> >
> >  escape_done:
> > -     folio_set_bh(new_bh, new_folio, new_offset);
> >       new_bh->b_size = bh_in->b_size;
> >       new_bh->b_bdev = journal->j_dev;
> >       new_bh->b_blocknr = blocknr;
> > diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> > index 699970b4bbf2..20b8fca1abfa 100644
> > --- a/include/linux/buffer_head.h
> > +++ b/include/linux/buffer_head.h
> > @@ -172,6 +172,35 @@ static inline unsigned long bh_offset(const struct buffer_head *bh)
> >       return (unsigned long)(bh)->b_data & (folio_size(bh->b_folio) - 1);
> >  }
> >
> > +/**
> > + * kmap_local_bh - Map the data of a buffer.
> > + * @bh: The buffer.
> > + *
> > + * Buffers usually live in the page cache, but a few are built over memory
> > + * which is not.  Those carry no folio and b_data is already a kernel address
> > + * which is always mapped, so there is nothing to do for them.  Pair with
> > + * kunmap_local_bh().
> > + *
> > + * Return: A pointer to the buffer's data.
> > + */
> > +static inline void *kmap_local_bh(const struct buffer_head *bh)
> > +{
> > +     if (!bh->b_folio)
> > +             return bh->b_data;
> > +     return kmap_local_folio(bh->b_folio, bh_offset(bh));
> > +}
> > +
> > +/**
> > + * kunmap_local_bh - Unmap the data of a buffer.
> > + * @bh: The buffer.
> > + * @addr: The address returned by kmap_local_bh().
> > + */
> > +static inline void kunmap_local_bh(const struct buffer_head *bh, void *addr)
> > +{
> > +     if (bh->b_folio)
> > +             kunmap_local(addr);
> > +}
> > +
> >  /* If we *know* page->private refers to buffer_heads */
> >  #define page_buffers(page)                                   \
> >       ({                                                      \
>
> When tested ocfs2 on next-20260831, I've encountered the following NULL
> pointer dereference:
>
>   BUG: kernel NULL pointer dereference, address: 0000000000000000
>   RIP: 0010:__bh_submit.constprop.0+0x87/0x120
>   Call Trace:
>     jbd2_journal_commit_transaction+0x932/0x1b10
>     kjournald2+0xb2/0x250
>
> Commit a2c924c240e7 ("buffer: set BIO_COMPLETE_IN_TASK for dropbehind
> writeback") added an unconditional folio_test_dropbehind(bh->b_folio) in
> __bh_submit(). But jbd2 shadow buffers have a NULL b_folio since commit
> 5febcba29792 ("jbd2: point the shadow buffer at the frozen data
> directly") made them point b_data at the kmalloced frozen data rather
> than a folio. So submitting such a buffer during journal commit oopses.
>
> A simple fix:
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 427d8a817cd5..f46fa6413032 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1106,7 +1106,8 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
>
>         bio = bio_alloc(bh->b_bdev, 1, opf, GFP_NOIO);
>
> -       if (folio_test_dropbehind(bh->b_folio) && op_is_write(opf))
> +       if (bh->b_folio && folio_test_dropbehind(bh->b_folio) &&
> +           op_is_write(opf))
>                 bio_set_flag(bio, BIO_COMPLETE_IN_TASK);
>
>         if (IS_ENABLED(CONFIG_FS_ENCRYPTION))
>
> Thanks,
> Joseph