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

[Joseph Qi <jiangqi903@gmail.com>]

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@infradead.org>
> Acked-by: Weidong Zhu <weizhu@fiu.edu>
> Signed-off-by: Chao Shi <coshi036@gmail.com>
> ---
>  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