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

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

[ThangNN99]

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.

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=03afbb29537f0336b7ad
Signed-off-by: ThangNN99 <ngocthang2710.1999@gmail.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---
 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

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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=03afbb29537f0336b7ad
> Signed-off-by: ThangNN99 <ngocthang2710.1999@gmail.com>
> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

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@suse.cz>

								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@suse.com>
SUSE Labs, CR

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

[ThangNN99]

Hi Jan,

Yes, your understanding is correct: with foliosize == blocksize the
itable block and the bitmap block can never share a folio, so the two
lock orders can't overlap. It's only once foliosize > blocksize that
the itable block being written back can land in the same folio as the
bitmap block being probed, opening the window for the deadlock.

Thanks for the clearer write-up of the two lock orders, I've folded it
into the v2 changelog, along with your Reviewed-by tag and the
Assisted-by: LLM tag in place of the previous Co-Authored-By line.

Agreed on the broader concern -- sb_getblk() called while holding
another buffer locked is a general hazard, not specific to this one
call site. I don't have another confirmed instance yet; happy to help
grep for more if useful, but wanted to get this one fix out first
rather than block it on a wider audit.

Sending v2 now.

Thanks,
Thang

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

[ThangNN99]

__ext4_get_inode_loc() looks up the inode bitmap bh while holding the
lock on the inode table bh:

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

whereas block_read_full_folio() (e.g. userspace reading the bdev inode
directly) takes the same two locks in the opposite order:

  block_read_full_folio()
    folio_lock(some folio)
      lock_buffer(bh in folio)

With blocksize == foliosize this can't overlap, but once foliosize >
blocksize the inode table block can land in the same folio as the
inode bitmap block, and the two orders deadlock on each other's lock.
Use the non-blocking cache lookup for the bitmap probe 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.

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=03afbb29537f0336b7ad
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: ThangNN99 <ngocthang2710.1999@gmail.com>
Assisted-by: LLM
---
 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

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

[Nguyen Ngoc Thang]

Hi Ted,

Gentle ping on this one -- v2 addressed Jan's comments and carries his
Reviewed-by. Let me know if anything else is needed.

Thanks,
Thang