Re: [PATCH next] buffer: guard folio_test_dropbehind() against a NULL b_folio in __bh_submit()

[Jan Kara]

On Tue 08-09-26 10:36:00, Ridong Chen wrote:
> From: Ridong Chen <chenridong@xxxxxxxxxx>
> 
> Commit 8deae2284976 ("buffer: allow a buffer_head to point at memory
> outside the page cache") made bh->b_folio optional: jbd2 submits a shadow
> buffer_head whose data lives in slab (the frozen/escaped copy of a metadata
> block), and such a buffer carries no folio at all - b_folio stays NULL from
> alloc_buffer_head() onwards (see the comment in
> jbd2_journal_write_metadata_buffer()).  That commit audited __bh_submit()
> and taught bio_add_folio_nofail() and wbc_account_cgroup_owner() to check
> for a NULL folio, but it missed an earlier dereference:
> 
> 	if (folio_test_dropbehind(bh->b_folio) && op_is_write(opf))
> 		bio_set_flag(bio, BIO_COMPLETE_IN_TASK);
> 
> That test was added by commit a2c924c240e7 ("buffer: set
> BIO_COMPLETE_IN_TASK for dropbehind writeback"), which predates the
> NULL-b_folio semantics and so dereferenced b_folio unconditionally.  Once
> b_folio may be NULL, folio_test_dropbehind() reads NULL->flags and the
> kernel takes a NULL pointer dereference.
> 
> This is reached from the jbd2 commit path (bh_submit() -> __bh_submit())
> whenever a shadow buffer is submitted, i.e. when a metadata block has to be
> escaped or was copied out, so it is data/timing dependent rather than seen
> on every commit.  When it does hit, it is deterministic and independent of
> the dropbehind bit, since the flag test dereferences the folio before
> examining it.
> 
> Reproduced on x86_64 with KASAN under filesystem write pressure:
> 
>   KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
>   RIP: 0010:__bh_submit+0x21b/0x9d0
>   Call Trace:
>    bh_submit+0x15/0x30
>    jbd2_journal_commit_transaction+0x1c19/0x5ac0
>    kjournald2+0x1cf/0x760
> 
> Guard the flag test with a b_folio check, matching the other folio
> accesses in the same function.
> 
> Fixes: 8deae2284976 ("buffer: allow a buffer_head to point at memory outside the page cache")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Ridong Chen <chenridong@xxxxxxxxxx>

A similar fix was already posted here [1] and is on its way to upstream.

								Honza

[1] https://lore.kernel.org/all/20260902013357.2815214-1-joseph.qi@xxxxxxxxxxxxxxxxx/

> ---
>  fs/buffer.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 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))
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR