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

["Aithal, Srikanth" <sraithal@amd.com>]

On 9/1/2026 9:07 AM, Joseph Qi 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@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

We hit the same oops on next-20260831 during ext4 journal commit on an
AMD VOLCANO machine (jbd2/nvme0n1p2-, RIP __bh_submit+0xa2). Unmodified
next-20260831 reproduces it; next-20260828 on the same box does not.
Joseph's one-liner on top of next-20260831 boots to login with no oops.

Tested-by: Srikanth Aithal <Srikanth.Aithal@amd.com>

Thanks,