Re: [PATCH] ext4: avoid buffer/folio lock inversion in __ext4_get_inode_loc()

[Jan Kara]

On Sun 06-09-26 17:48:41, ThangNN99 wrote:
> The itable-block bh is locked, then the "is bitmap cached?" probe calls
> sb_getblk(), which can block on that bitmap block's folio lock. A
> concurrent block_read_full_folio() on the same bdev folio locks buffers
> in the opposite order (folio lock, then each bh), so the two tasks can
> deadlock on each other's lock. Use the non-blocking cache lookup here
> instead; a miss already falls back to make_io exactly as before.
> 
> Only ext4_reserve_inode_write() reaches this probe with a real inode
> (ext4_iget() passes NULL, which skips it), and it normally runs right
> after the read that loaded that same inode, so the itable buffer is
> still warm and the early "already uptodate" return skips the probe.
> The window needs the folio reclaimed between load and writeback, which
> is why this is rare and why syzbot's bisection could not pin it down.

Thanks for the analysis! Maybe the description would me more understandable
as:

__ext4_get_inode_loc() looks up inode bitmap bh under the lock of inode
table bh like:

__ext4_get_inode_loc()
  lock_buffer(itable block)
  sb_getblk(inode bitmap block)
    __find_get_block_nonatomic()
      folio_lock(bdev folio for bitmap block)

OTOH block_read_full_folio() does:
   folio_lock(some folio)
     lock_buffer(bh in folio)

block_read_full_folio() can be executed for example from userspace by
reading bdev inode.

Now generally (in particular if folio size == block size) the itable block
is not in the same folio as the inode bitmap block so this isn't really a
problem. But when folio size > block size, it can happen that the itable
block is in the same folio as inode bitmap block and this can the deadlock.

--

Is my understanding correct?

Also it seems this is a general problem for places where we call
sb_getblk() for one buffer while having locked another buffer. I suspect we
may have more places in the code than just this one. We'll need to
investigate.

> Reproduction status: root-caused from source and confirmed against
> both syzbot stacks (inode.c:__ext4_get_inode_loc vs.
> buffer.c:block_read_full_folio); the lock_buffer()/reserve_inode_write
> path was exercised live (orphan cleanup on mount) to confirm reachability
> and to confirm this patch introduces no regression there. The deadlock
> itself was not reproduced locally -- doing so needs the itable buffer
> genuinely reclaimed between inode load and writeback, which a small
> single-shot QEMU test doesn't naturally produce.
> 
> Reported-by: syzbot+03afbb29537f0336b7ad@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=03afbb29537f0336b7ad
> Signed-off-by: ThangNN99 <ngocthang2710.1999@xxxxxxxxx>
> Co-Authored-By: Claude Sonnet 5 <noreply@xxxxxxxxxxxxx>

LLMs cannot be authors. Please just put here tag:

Assisted-by: LLM

Otherwise the fix for this problem looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/ext4/inode.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index bd4b778df9eb..13e3cb829461 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4942,8 +4942,12 @@ static int __ext4_get_inode_loc(struct super_block *sb, unsigned long ino,
>  
>  		start = inode_offset & ~(inodes_per_block - 1);
>  
> -		/* Is the inode bitmap in cache? */
> -		bitmap_bh = sb_getblk(sb, ext4_inode_bitmap(sb, gdp));
> +		/*
> +		 * Is the inode bitmap in cache? Non-blocking lookup: bh above
> +		 * is locked, and blocking here would folio_lock() against a
> +		 * block_read_full_folio() that locks bh the other way round.
> +		 */
> +		bitmap_bh = sb_find_get_block(sb, ext4_inode_bitmap(sb, gdp));
>  		if (unlikely(!bitmap_bh))
>  			goto make_io;
>  
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR