[PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[ThangNN99]

hfs_bmap_reserve() calls hfsplus_file_extend() on tree->inode with
tree->tree_lock already held.  When the tree is the extents overflow
B-tree and its own fork already claims more blocks than its eight
fork extents describe, hfsplus_file_extend() calls
hfsplus_ext_read_extent() -> hfs_find_init() on that same tree,
taking tree_lock a second time (lockdep: "possible recursive locking
... &tree->tree_lock/1").

Per the HFS+ format the extents overflow file is fully described by
its eight fork extents, so this state only arises from a corrupted
image. hfsplus_get_block() already refuses it for lookups; do the
same when growing the file.

Reported-by: syzbot+f8ce6c197125ab9d72ce@syzkaller.appspotmail.com
Signed-off-by: ThangNN99 <ngocthang2710.1999@gmail.com>
---
 fs/hfsplus/extents.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index eb7c11524d18..39920b51e08b 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -458,6 +458,15 @@ int hfsplus_file_extend(struct inode *inode, bool zeroout)
 	if (hip->alloc_blocks == hip->first_blocks)
 		goal = hfsplus_ext_lastblock(hip->first_extents);
 	else {
+		/*
+		 * The extents overflow file can't have overflow extents of
+		 * its own; growing it here would re-enter hfs_find_init()
+		 * on the extents tree, whose tree_lock is already held.
+		 */
+		if (inode->i_ino == HFSPLUS_EXT_CNID) {
+			res = -EIO;
+			goto out;
+		}
 		res = hfsplus_ext_read_extent(inode, hip->alloc_blocks);
 		if (res)
 			goto out;
-- 
2.43.0

Re: [PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[Viacheslav Dubeyko]

On Sun, 2026-09-06 at 22:49 +0700, ThangNN99 wrote:
> hfs_bmap_reserve() calls hfsplus_file_extend() on tree->inode with
> tree->tree_lock already held.  When the tree is the extents overflow
> B-tree and its own fork already claims more blocks than its eight
> fork extents describe, hfsplus_file_extend() calls
> hfsplus_ext_read_extent() -> hfs_find_init() on that same tree,
> taking tree_lock a second time (lockdep: "possible recursive locking
> ... &tree->tree_lock/1").

Could you please explain the use-case or workload that is trying to
claim more blocks that fork can include for Extents Overflow file?
Could you please share the call trace for the issue?

Thanks,
Slava.

> 
> Per the HFS+ format the extents overflow file is fully described by
> its eight fork extents, so this state only arises from a corrupted
> image. hfsplus_get_block() already refuses it for lookups; do the
> same when growing the file.
> 
> Reported-by: syzbot+f8ce6c197125ab9d72ce@syzkaller.appspotmail.com
> Signed-off-by: ThangNN99 <ngocthang2710.1999@gmail.com>
> ---
>  fs/hfsplus/extents.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
> index eb7c11524d18..39920b51e08b 100644
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -458,6 +458,15 @@ int hfsplus_file_extend(struct inode *inode,
> bool zeroout)
>  	if (hip->alloc_blocks == hip->first_blocks)
>  		goal = hfsplus_ext_lastblock(hip->first_extents);
>  	else {
> +		/*
> +		 * The extents overflow file can't have overflow
> extents of
> +		 * its own; growing it here would re-enter
> hfs_find_init()
> +		 * on the extents tree, whose tree_lock is already
> held.
> +		 */
> +		if (inode->i_ino == HFSPLUS_EXT_CNID) {
> +			res = -EIO;
> +			goto out;
> +		}
>  		res = hfsplus_ext_read_extent(inode, hip-
> >alloc_blocks);
>  		if (res)
>  			goto out;

Re: [PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[ThangNN99]

Hi Slava,

Thanks for looking at this.

> Could you please explain the use-case or workload that is trying to
> claim more blocks that fork can include for Extents Overflow file?

It is not a normal workload -- it requires a corrupted/adversarial
on-disk volume, e.g. a loop-mounted image (removable media, a
downloaded .img/.dmg, or a fuzzer). Per Apple's TN1150 ("HFS Plus
Volume Format"):

  "The extents overflow file also stores additional extents for the
  special files except for the extents overflow file itself."

So by design the extents overflow file must always be fully described
by the eight extents in its own fork record; it can never legitimately
need an overflow extent of its own. The syzbot reproducer mounts an
image whose volume header sets the Extents File fork's total block
count higher than what its eight direct extents describe, which puts
hip->alloc_blocks != hip->first_blocks for HFSPLUS_EXT_CNID -- a state
the volume header alone can force without the extents tree itself
being touched. hfs_btree_open() doesn't currently validate this fork
against the invariant above.

Once mounted, a plain pwritev2() to a regular file (call it FILE_A)
that already has extents cached from a previous lookup is enough to
hit it:

> Could you please share the call trace for the issue?

  pwritev2
   -> hfsplus_get_block(FILE_A)
   -> hfsplus_file_extend(FILE_A)
   -> hfsplus_ext_read_extent(FILE_A) -> hfs_find_init(ext_tree)   [tree_lock acquired]
   -> __hfsplus_ext_cache_extent(FILE_A): FILE_A's cached extent is dirty
   -> __hfsplus_ext_write_extent(FILE_A): needs to insert a new record
   -> hfs_bmap_reserve(ext_tree): ext_tree itself is out of free nodes
   -> hfsplus_file_extend(ext_tree->inode)          <- now growing the tree's own file
   -> hfsplus_ext_read_extent(ext_tree->inode) -> hfs_find_init(ext_tree)  [tree_lock again -> deadlock]

Full syzbot lockdep report for reference:

WARNING: possible recursive locking detected
6.16.0-rc7-syzkaller-00120-g5f33ebd2018c #0 Not tainted
--------------------------------------------
syz-executor310/5840 is trying to acquire lock:
ffff88807fe920b0 (&tree->tree_lock/1){+.+.}-{4:4}, at: hfsplus_find_init+0x15a/0x1d0 fs/hfsplus/bfind.c:28

but task is already holding lock:
ffff88807fe920b0 (&tree->tree_lock/1){+.+.}-{4:4}, at: hfsplus_find_init+0x15a/0x1d0 fs/hfsplus/bfind.c:28

5 locks held by syz-executor310/5840:
 #0: sb_writers#8, at: vfs_writev+0x288/0x960 fs/read_write.c:1055
 #1: &sb->s_type->i_mutex_key#14, at: generic_file_write_iter+0xe3/0x540 mm/filemap.c:4252
 #2: &hip->extents_lock, at: hfsplus_file_extend+0x1fc/0x1990 fs/hfsplus/extents.c:458
 #3: &tree->tree_lock/1, at: hfsplus_find_init+0x15a/0x1d0 fs/hfsplus/bfind.c:28
 #4: &HFSPLUS_I(inode)->extents_lock, at: hfsplus_file_extend+0x1fc/0x1990 fs/hfsplus/extents.c:458

Call Trace:
 hfsplus_find_init fs/hfsplus/bfind.c:28
 hfsplus_ext_read_extent fs/hfsplus/extents.c:216 [inline]
 hfsplus_file_extend+0x416/0x1990 fs/hfsplus/extents.c:462
 hfsplus_bmap_reserve+0x122/0x500 fs/hfsplus/btree.c:358
 __hfsplus_ext_write_extent+0x28d/0x5b0 fs/hfsplus/extents.c:104
 __hfsplus_ext_cache_extent+0x89/0xe30 fs/hfsplus/extents.c:186
 hfsplus_ext_read_extent fs/hfsplus/extents.c:218 [inline]
 hfsplus_file_extend+0x444/0x1990 fs/hfsplus/extents.c:462
 hfsplus_get_block+0x411/0x1530 fs/hfsplus/extents.c:245
 __block_write_begin_int+0x6b2/0x1900 fs/buffer.c:2151
 block_write_begin fs/buffer.c:2262 [inline]
 cont_write_begin+0x789/0xb50 fs/buffer.c:2601
 hfsplus_write_begin+0x66/0xb0 fs/hfsplus/inode.c:46
 generic_perform_write+0x2c4/0x910 mm/filemap.c:4112
 generic_file_write_iter+0x10f/0x540 mm/filemap.c:4255
 do_iter_readv_writev+0x56b/0x7f0 fs/read_write.c:-1
 vfs_writev+0x31a/0x960 fs/read_write.c:1057
 do_pwritev fs/read_write.c:1153 [inline]
 __se_sys_pwritev2+0x179/0x290 fs/read_write.c:1202

(full report: https://syzkaller.appspot.com/text?tag=CrashReport&x=172748a2580000)

hfsplus_get_block() already refuses HFSPLUS_EXT_CNID for the lookup
direction (extents.c:261: "if (inode->i_ino == HFSPLUS_EXT_CNID)
return -EIO;"). This patch adds the same refusal on the grow path in
hfsplus_file_extend(), which is the one hfs_bmap_reserve() can reach
with tree_lock already held. It doesn't fix the underlying corruption,
just stops it from self-deadlocking the tree_lock; a hfs_btree_open()
check that rejects such a volume outright at mount time would close
the hole earlier and I'm happy to send that as a follow-up if you'd
rather validate it there instead.

I have not personally observed the crash on current mainline: I
rebuilt the syzbot C reproducer, and hfs_btree_open(HFSPLUS_CAT_CNID)
now rejects the image at mount (silently) -- it's a 2022-era fuzzed
image and mainline has since gained catalog b-tree validation
(node-size sanity check, record-offset table validation, etc.) that
this image no longer passes. The analysis above is derived from source
plus the syzbot-provided trace, not from a reproduced local crash.

Thanks,
Thang

Re: [PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[Viacheslav Dubeyko]

On Tue, 2026-09-08 at 00:15 +0700, ThangNN99 wrote:
> Hi Slava,
> 
> Thanks for looking at this.
> 
> > Could you please explain the use-case or workload that is trying to
> > claim more blocks that fork can include for Extents Overflow file?
> 
> It is not a normal workload -- it requires a corrupted/adversarial
> on-disk volume, e.g. a loop-mounted image (removable media, a
> downloaded .img/.dmg, or a fuzzer). Per Apple's TN1150 ("HFS Plus
> Volume Format"):

OK. It sounds that you have corrupted fork in volume header for the
case of Extents Overflow file (and, maybe, for other metadata too).
Could you share the dump/content of the Extents Overflow file's fork?
Could we detect the corruption of the fork during the mount phase? If
we can then we need to mount in Read-Only mode the corrupted volume.

Thanks,
Slava.


> 
>   "The extents overflow file also stores additional extents for the
>   special files except for the extents overflow file itself."
> 
> So by design the extents overflow file must always be fully described
> by the eight extents in its own fork record; it can never
> legitimately
> need an overflow extent of its own. The syzbot reproducer mounts an
> image whose volume header sets the Extents File fork's total block
> count higher than what its eight direct extents describe, which puts
> hip->alloc_blocks != hip->first_blocks for HFSPLUS_EXT_CNID -- a
> state
> the volume header alone can force without the extents tree itself
> being touched. hfs_btree_open() doesn't currently validate this fork
> against the invariant above.
> 
> Once mounted, a plain pwritev2() to a regular file (call it FILE_A)
> that already has extents cached from a previous lookup is enough to
> hit it:
> 
> > Could you please share the call trace for the issue?
> 
>   pwritev2
>    -> hfsplus_get_block(FILE_A)
>    -> hfsplus_file_extend(FILE_A)
>    -> hfsplus_ext_read_extent(FILE_A) -> hfs_find_init(ext_tree)  
> [tree_lock acquired]
>    -> __hfsplus_ext_cache_extent(FILE_A): FILE_A's cached extent is
> dirty
>    -> __hfsplus_ext_write_extent(FILE_A): needs to insert a new
> record
>    -> hfs_bmap_reserve(ext_tree): ext_tree itself is out of free
> nodes
>    -> hfsplus_file_extend(ext_tree->inode)          <- now growing
> the tree's own file
>    -> hfsplus_ext_read_extent(ext_tree->inode) ->
> hfs_find_init(ext_tree)  [tree_lock again -> deadlock]
> 
> Full syzbot lockdep report for reference:
> 
> WARNING: possible recursive locking detected
> 6.16.0-rc7-syzkaller-00120-g5f33ebd2018c #0 Not tainted
> --------------------------------------------
> syz-executor310/5840 is trying to acquire lock:
> ffff88807fe920b0 (&tree->tree_lock/1){+.+.}-{4:4}, at:
> hfsplus_find_init+0x15a/0x1d0 fs/hfsplus/bfind.c:28
> 
> but task is already holding lock:
> ffff88807fe920b0 (&tree->tree_lock/1){+.+.}-{4:4}, at:
> hfsplus_find_init+0x15a/0x1d0 fs/hfsplus/bfind.c:28
> 
> 5 locks held by syz-executor310/5840:
>  #0: sb_writers#8, at: vfs_writev+0x288/0x960 fs/read_write.c:1055
>  #1: &sb->s_type->i_mutex_key#14, at:
> generic_file_write_iter+0xe3/0x540 mm/filemap.c:4252
>  #2: &hip->extents_lock, at: hfsplus_file_extend+0x1fc/0x1990
> fs/hfsplus/extents.c:458
>  #3: &tree->tree_lock/1, at: hfsplus_find_init+0x15a/0x1d0
> fs/hfsplus/bfind.c:28
>  #4: &HFSPLUS_I(inode)->extents_lock, at:
> hfsplus_file_extend+0x1fc/0x1990 fs/hfsplus/extents.c:458
> 
> Call Trace:
>  hfsplus_find_init fs/hfsplus/bfind.c:28
>  hfsplus_ext_read_extent fs/hfsplus/extents.c:216 [inline]
>  hfsplus_file_extend+0x416/0x1990 fs/hfsplus/extents.c:462
>  hfsplus_bmap_reserve+0x122/0x500 fs/hfsplus/btree.c:358
>  __hfsplus_ext_write_extent+0x28d/0x5b0 fs/hfsplus/extents.c:104
>  __hfsplus_ext_cache_extent+0x89/0xe30 fs/hfsplus/extents.c:186
>  hfsplus_ext_read_extent fs/hfsplus/extents.c:218 [inline]
>  hfsplus_file_extend+0x444/0x1990 fs/hfsplus/extents.c:462
>  hfsplus_get_block+0x411/0x1530 fs/hfsplus/extents.c:245
>  __block_write_begin_int+0x6b2/0x1900 fs/buffer.c:2151
>  block_write_begin fs/buffer.c:2262 [inline]
>  cont_write_begin+0x789/0xb50 fs/buffer.c:2601
>  hfsplus_write_begin+0x66/0xb0 fs/hfsplus/inode.c:46
>  generic_perform_write+0x2c4/0x910 mm/filemap.c:4112
>  generic_file_write_iter+0x10f/0x540 mm/filemap.c:4255
>  do_iter_readv_writev+0x56b/0x7f0 fs/read_write.c:-1
>  vfs_writev+0x31a/0x960 fs/read_write.c:1057
>  do_pwritev fs/read_write.c:1153 [inline]
>  __se_sys_pwritev2+0x179/0x290 fs/read_write.c:1202
> 
> (full report:
> https://syzkaller.appspot.com/text?tag=CrashReport&x=172748a2580000)
> 
> hfsplus_get_block() already refuses HFSPLUS_EXT_CNID for the lookup
> direction (extents.c:261: "if (inode->i_ino == HFSPLUS_EXT_CNID)
> return -EIO;"). This patch adds the same refusal on the grow path in
> hfsplus_file_extend(), which is the one hfs_bmap_reserve() can reach
> with tree_lock already held. It doesn't fix the underlying
> corruption,
> just stops it from self-deadlocking the tree_lock; a hfs_btree_open()
> check that rejects such a volume outright at mount time would close
> the hole earlier and I'm happy to send that as a follow-up if you'd
> rather validate it there instead.
> 
> I have not personally observed the crash on current mainline: I
> rebuilt the syzbot C reproducer, and hfs_btree_open(HFSPLUS_CAT_CNID)
> now rejects the image at mount (silently) -- it's a 2022-era fuzzed
> image and mainline has since gained catalog b-tree validation
> (node-size sanity check, record-offset table validation, etc.) that
> this image no longer passes. The analysis above is derived from
> source
> plus the syzbot-provided trace, not from a reproduced local crash.
> 
> Thanks,
> Thang

Re: [PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[ThangNN99]

Hi Slava,

> Could you share the dump/content of the Extents Overflow file's fork?

Decoded the volume header from the syzbot image. The Extents Overflow
fork (offset 192 in the header):

  logicalSize=32768 clumpSize=32768 totalBlocks=32
  extents[0]=(start=3,   count=32)
  extents[1]=(start=0,   count=0)
  extents[2]=(start=0,   count=0)
  extents[3]=(start=0,   count=134217728)   <- garbage
  extents[4]=(start=0,   count=0)
  extents[5]=(start=0,   count=0)
  extents[6]=(start=0,   count=11796736)    <- garbage
  extents[7]=(start=0,   count=0)

Correction to my last mail: it's not totalBlocks exceeding the fork's
extents, it's the reverse and messier. hfsplus_inode_read_fork() sums
all 8 extents' block_count into hip->first_blocks with no validation
(inode.c:569). Slots 3 and 6 have start_block=0 (i.e. "unused") but
garbage non-zero block_count, so first_blocks comes out to 146014496
against a real totalBlocks (hip->alloc_blocks) of 32. Either direction
of that mismatch takes hfsplus_file_extend() down the same
hfsplus_ext_read_extent() path, since the code only tests
alloc_blocks == first_blocks.

> Could we detect the corruption of the fork during the mount phase?
> If we can then we need to mount in Read-Only mode the corrupted
> volume.

Yes. Proposed v2, forcing read-only instead of touching extents.c:

--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -293,6 +293,14 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 		goto free_inode;
 	}

+	/* Per TN1150, the extents file can't have overflow extents of its own. */
+	if (id == HFSPLUS_EXT_CNID &&
+	    HFSPLUS_I(tree->inode)->first_blocks !=
+	    HFSPLUS_I(tree->inode)->alloc_blocks) {
+		pr_warn("extents overflow file has overflow extents of its own, forcing read-only.\n");
+		sb->s_flags |= SB_RDONLY;
+	}
+
 	mapping = tree->inode->i_mapping;
 	page = read_mapping_page(mapping, 0, NULL);
 	if (IS_ERR(page))

One catch: the reproducer mounts MS_RDONLY, then remounts rw via a
bare MS_REMOUNT|MS_MOVE. hfsplus_reconfigure() only re-checks
VOL_UNMNT/SOFTLOCK/JOURNALED before allowing that, and this image sets
VOL_UNMNT, so a read-only-only fix in hfs_btree_open() gets undone by
that remount. Same check needs to go in hfsplus_reconfigure() too:

--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -400,6 +400,12 @@ static int hfsplus_reconfigure(struct fs_context *fc)
 			pr_warn("filesystem is marked journaled, leaving read-only.\n");
 			sb->s_flags |= SB_RDONLY;
 			fc->sb_flags |= SB_RDONLY;
+		} else if (HFSPLUS_I(sbi->ext_tree->inode)->first_blocks !=
+				HFSPLUS_I(sbi->ext_tree->inode)->alloc_blocks) {
+			/* Per TN1150, the extents file can't have overflow extents of its own. */
+			pr_warn("extents overflow file has overflow extents of its own, leaving read-only.\n");
+			sb->s_flags |= SB_RDONLY;
+			fc->sb_flags |= SB_RDONLY;
 		}
 	}
 	return 0;

Both hunks build cleanly here. Want me to send this as v2 replacing
the extents.c hunk, or keep the extents.c guard too as a second line
of defense (it's independent of mount-time state and free)?

Thanks,
Thang

Re: [PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[Viacheslav Dubeyko]

On Tue, 2026-09-08 at 18:54 +0700, ThangNN99 wrote:
> Hi Slava,
> 
> > Could you share the dump/content of the Extents Overflow file's
> > fork?
> 
> Decoded the volume header from the syzbot image. The Extents Overflow
> fork (offset 192 in the header):
> 
>   logicalSize=32768 clumpSize=32768 totalBlocks=32
>   extents[0]=(start=3,   count=32)
>   extents[1]=(start=0,   count=0)
>   extents[2]=(start=0,   count=0)
>   extents[3]=(start=0,   count=134217728)   <- garbage
>   extents[4]=(start=0,   count=0)
>   extents[5]=(start=0,   count=0)
>   extents[6]=(start=0,   count=11796736)    <- garbage
>   extents[7]=(start=0,   count=0)
> 
> Correction to my last mail: it's not totalBlocks exceeding the fork's
> extents, it's the reverse and messier. hfsplus_inode_read_fork() sums
> all 8 extents' block_count into hip->first_blocks with no validation
> (inode.c:569). Slots 3 and 6 have start_block=0 (i.e. "unused") but
> garbage non-zero block_count, so first_blocks comes out to 146014496
> against a real totalBlocks (hip->alloc_blocks) of 32. Either
> direction
> of that mismatch takes hfsplus_file_extend() down the same
> hfsplus_ext_read_extent() path, since the code only tests
> alloc_blocks == first_blocks.

I think we need to have more precise fork check. Because, corruption
could be more severe. The start block of extents could be out of
volume. The total number of block could be not consistent with
calculated one and be bigger than volume itself. Also, we need to check
the fork for all types of btrees (Catalog, Extents, Extended
attributes).

> 
> > Could we detect the corruption of the fork during the mount phase?
> > If we can then we need to mount in Read-Only mode the corrupted
> > volume.
> 
> Yes. Proposed v2, forcing read-only instead of touching extents.c:
> 
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -293,6 +293,14 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id)
>  		goto free_inode;
>  	}
> 
> +	/* Per TN1150, the extents file can't have overflow extents
> of its own. */
> +	if (id == HFSPLUS_EXT_CNID &&
> +	    HFSPLUS_I(tree->inode)->first_blocks !=
> +	    HFSPLUS_I(tree->inode)->alloc_blocks) {
> +		pr_warn("extents overflow file has overflow extents
> of its own, forcing read-only.\n");
> +		sb->s_flags |= SB_RDONLY;
> +	}
> +

The hfs_btree_open() could be called not only during mount or re-mount.
So, I think that it needs to return the error code from this method.
Finally, caller could make decision if it is a proper place to set sb-
>s_flags |= SB_RDONLY. And, again, we need to check not only Extents
Overflow file.

>  	mapping = tree->inode->i_mapping;
>  	page = read_mapping_page(mapping, 0, NULL);
>  	if (IS_ERR(page))
> 
> One catch: the reproducer mounts MS_RDONLY, then remounts rw via a
> bare MS_REMOUNT|MS_MOVE. hfsplus_reconfigure() only re-checks
> VOL_UNMNT/SOFTLOCK/JOURNALED before allowing that, and this image
> sets
> VOL_UNMNT, so a read-only-only fix in hfs_btree_open() gets undone by
> that remount. Same check needs to go in hfsplus_reconfigure() too:
> 
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -400,6 +400,12 @@ static int hfsplus_reconfigure(struct fs_context
> *fc)
>  			pr_warn("filesystem is marked journaled,
> leaving read-only.\n");
>  			sb->s_flags |= SB_RDONLY;
>  			fc->sb_flags |= SB_RDONLY;
> +		} else if (HFSPLUS_I(sbi->ext_tree->inode)-
> >first_blocks !=
> +				HFSPLUS_I(sbi->ext_tree->inode)-
> >alloc_blocks) {
> +			/* Per TN1150, the extents file can't have
> overflow extents of its own. */
> +			pr_warn("extents overflow file has overflow
> extents of its own, leaving read-only.\n");
> +			sb->s_flags |= SB_RDONLY;
> +			fc->sb_flags |= SB_RDONLY;

Frankly speaking, I would like to introduce some method(s) that can
check the btree consistency.

Do we really need to repeat the check on remount? If we detected that
btree is corrupted during mount, then we should set the state of the
btree as inconsistent and simply check such flag(s). Do we have
something like this now? If it is not, then we can simply introduce
one.

>  		}
>  	}
>  	return 0;
> 
> Both hunks build cleanly here. Want me to send this as v2 replacing
> the extents.c hunk, or keep the extents.c guard too as a second line
> of defense (it's independent of mount-time state and free)?

The checking inconsistency is one direction. Another direction is that
we exhausted the volume or volume is so fragmented that we cannot
extend the Extents Overflow file anymore. I think we need to check
before extending the Extents Overflow file that we have free extent
slots or we can add some space into the latest extent. If there is no
such opportunity, then we need to report -ENOSPC. Because, we cannot
add any new data on the volume. The main question here how to add or
modify the logic in safe way.

Thanks,
Slava.

Re: [PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[Nguyen Ngoc Thang]

Hi Slava,

Agreed on all four points, and dropping the btree.c/super.c hunks --
you're right on the specifics too: hfs_btree_open() is also called
from xattr.c when an attributes tree is created lazily, mid-operation,
so it has no business deciding sb->s_flags itself. And re-checking my
own super.c hunk: it dereferences sbi->ext_tree/attr_tree
unconditionally, which NULL-derefs on remount of a volume with no
attributes file (attr_tree is NULL whenever vhdr->attr_file.total_blocks
== 0). Glad that didn't go anywhere.

One clarifying question before I attempt that piece: you wrote both
"it needs to return the error code from this method" and "set the
state of the btree as inconsistent". Those lead to different mounts:

  (a) hfs_btree_open() returns ERR_PTR(-EIO) -> the tree never opens,
      mount fails outright (same as every other check already in that
      function).
  (b) hfs_btree_open() still returns the tree, with a new inconsistency
      flag set on it -> mount can succeed read-only, existing (valid)
      data stays reachable.

I'd lean towards (b) -- read-only recovery only works if the tree
actually opens -- but that's your call, not mine to assume. Which did
you mean, or something else?

For v2 I'm narrowing to just the recursion fix, changed per your ENOSPC
point below:

--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -458,6 +458,14 @@ int hfsplus_file_extend(struct inode *inode, bool zeroout)
 	if (hip->alloc_blocks == hip->first_blocks)
 		goal = hfsplus_ext_lastblock(hip->first_extents);
 	else {
+		/*
+		 * The extents overflow file can't grow past its own fork
+		 * extents: doing so would re-enter hfs_find_init() on the
+		 * extents tree, whose tree_lock is already held here.
+		 */
+		if (inode->i_ino == HFSPLUS_EXT_CNID) {
+			res = -ENOSPC;
+			goto out;
+		}
 		res = hfsplus_ext_read_extent(inode, hip->alloc_blocks);
 		if (res)
 			goto out;

> Another direction is that we exhausted the volume or volume is so
> fragmented that we cannot extend the Extents Overflow file anymore.
> [...] we need to check before extending [...] that we have free
> extent slots or we can add some space into the latest extent. If
> there is no such opportunity, then we need to report -ENOSPC.

Right -- that's the same guard, just under a correct errno. It fires
identically whether the fork is corrupted (this report) or the tree
has genuinely run out of room to describe itself, without needing to
tell those two apart at this call site. Sending this alone as v2 so
the deadlock fix isn't blocked on the larger validator design; happy
to follow up with the fork-bounds/consistency-flag work separately
once (a)/(b) above is settled.

Thanks,
Thang

Re: [PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[Viacheslav Dubeyko]

On Wed, 2026-09-09 at 23:20 +0700, Nguyen Ngoc Thang wrote:
> Hi Slava,
> 
> Agreed on all four points, and dropping the btree.c/super.c hunks --
> you're right on the specifics too: hfs_btree_open() is also called
> from xattr.c when an attributes tree is created lazily, mid-
> operation,
> so it has no business deciding sb->s_flags itself. And re-checking my
> own super.c hunk: it dereferences sbi->ext_tree/attr_tree
> unconditionally, which NULL-derefs on remount of a volume with no
> attributes file (attr_tree is NULL whenever vhdr-
> >attr_file.total_blocks
> == 0). Glad that didn't go anywhere.
> 
> One clarifying question before I attempt that piece: you wrote both
> "it needs to return the error code from this method" and "set the
> state of the btree as inconsistent". Those lead to different mounts:
> 
>   (a) hfs_btree_open() returns ERR_PTR(-EIO) -> the tree never opens,
>       mount fails outright (same as every other check already in that
>       function).
>   (b) hfs_btree_open() still returns the tree, with a new
> inconsistency
>       flag set on it -> mount can succeed read-only, existing (valid)
>       data stays reachable.
> 
> I'd lean towards (b) -- read-only recovery only works if the tree
> actually opens -- but that's your call, not mine to assume. Which did
> you mean, or something else?

Technically speaking, if we have a corrupted fork, then we have no idea
where metadata structure is located on the volume. It means that we
cannot read it and we have nothing instead of metadata structure. So,
this is the situation when FSCK tool needs to work. It sounds like we
cannot construct the valid b-tree metadata structure anyway. We can
only return the error. And if it is the hfsplus_fill_super(), then we
cannot mount file system volume at all. However, we could have not so
severe issue with b-tree metadata structure. I think that if the first
extent looks consistent but the other extents contains garbage, then we
can try to construct the b-tree, mark b-tree as inconsistent, and mount
file system as READ-ONLY. 

> 
> For v2 I'm narrowing to just the recursion fix, changed per your
> ENOSPC
> point below:
> 
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -458,6 +458,14 @@ int hfsplus_file_extend(struct inode *inode,
> bool zeroout)
>  	if (hip->alloc_blocks == hip->first_blocks)
>  		goal = hfsplus_ext_lastblock(hip->first_extents);
>  	else {
> +		/*
> +		 * The extents overflow file can't grow past its own
> fork
> +		 * extents: doing so would re-enter hfs_find_init()
> on the
> +		 * extents tree, whose tree_lock is already held
> here.
> +		 */
> +		if (inode->i_ino == HFSPLUS_EXT_CNID) {
> +			res = -ENOSPC;
> +			goto out;
> +		}

Probably, hfs_bmap_reserve() is the proper place for checking
capability of growing Extents Overflow file. But it needs to take into
account that if fork has empty extents, then we can grow the b-tree. We
have -ENOSPC situation only if we already used all extents in the
fork. 

Thanks,
Slava. 

>  		res = hfsplus_ext_read_extent(inode, hip-
> >alloc_blocks);
>  		if (res)
>  			goto out;
> 
> > Another direction is that we exhausted the volume or volume is so
> > fragmented that we cannot extend the Extents Overflow file anymore.
> > [...] we need to check before extending [...] that we have free
> > extent slots or we can add some space into the latest extent. If
> > there is no such opportunity, then we need to report -ENOSPC.
> 
> Right -- that's the same guard, just under a correct errno. It fires
> identically whether the fork is corrupted (this report) or the tree
> has genuinely run out of room to describe itself, without needing to
> tell those two apart at this call site. Sending this alone as v2 so
> the deadlock fix isn't blocked on the larger validator design; happy
> to follow up with the fork-bounds/consistency-flag work separately
> once (a)/(b) above is settled.
> 
> Thanks,
> Thang

Re: [PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[Nguyen Ngoc Thang]

Hi Slava,

> Probably, hfs_bmap_reserve() is the proper place for checking
> capability of growing Extents Overflow file. But it needs to take
> into account that if fork has empty extents, then we can grow the
> b-tree. We have -ENOSPC situation only if we already used all
> extents in the fork.

Right, and it turns out the existing control flow already computes
exactly that, so I kept the check in extents.c rather than duplicating
fork-layout knowledge in hfs_bmap_reserve(): hfsplus_add_extent()
returns -ENOSPC only when it has walked all eight slots and the last
one can't be extended contiguously (the ++i >= 8 case). If there's an
empty slot, or the last extent can be grown in place, it consumes that
and returns 0 -- hfsplus_file_extend() never reaches the
"insert_extent" label in that case. So arriving at insert_extent
already means the fork is exhausted; no slot scan needed there.

v2, two hunks in the same function:

--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -458,6 +458,15 @@ int hfsplus_file_extend(struct inode *inode, bool zeroout)
 	if (hip->alloc_blocks == hip->first_blocks)
 		goal = hfsplus_ext_lastblock(hip->first_extents);
 	else {
+		/*
+		 * The fork already claims more blocks than its eight extents
+		 * describe (a corrupt on-disk fork): looking up the rest
+		 * would re-enter hfs_find_init() on the extents tree, whose
+		 * tree_lock is already held here.
+		 */
+		if (inode->i_ino == HFSPLUS_EXT_CNID) {
+			res = -ENOSPC;
+			goto out;
+		}
 		res = hfsplus_ext_read_extent(inode, hip->alloc_blocks);
 		if (res)
 			goto out;
@@ -534,6 +543,15 @@ int hfsplus_file_extend(struct inode *inode, bool zeroout)
 	return res;

 insert_extent:
+	/*
+	 * Getting here means the fork's eight extents are exhausted (see
+	 * hfsplus_add_extent()). The extents overflow file can't record
+	 * an overflow extent of its own, so it cannot grow any further.
+	 */
+	if (inode->i_ino == HFSPLUS_EXT_CNID) {
+		res = -ENOSPC;
+		goto out;
+	}
+
 	hfs_dbg("insert new extent\n");
 	res = hfsplus_ext_write_extent_locked(inode);
 	if (res)

First hunk: fork was already inconsistent when read from disk at
mount. Second hunk: fork was consistent but genuinely ran out of the
eight slots during this call -- your ENOSPC case. Both land on the
same tree_lock recursion, so both need the guard.

On the severity split you described (consistent first extent + garbage
elsewhere -> construct + flag inconsistent + read-only; unusable first
extent -> hard error, mount fails): agreed, and that's the direction
I'll take the fork-validator follow-up once this one's in, applying it
to all three trees as you asked.

Both hunks build cleanly here.

Thanks,
Thang

Re: [PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[Viacheslav Dubeyko]

On Thu, 2026-09-10 at 23:01 +0700, Nguyen Ngoc Thang wrote:
> Hi Slava,
> 
> > Probably, hfs_bmap_reserve() is the proper place for checking
> > capability of growing Extents Overflow file. But it needs to take
> > into account that if fork has empty extents, then we can grow the
> > b-tree. We have -ENOSPC situation only if we already used all
> > extents in the fork.
> 
> Right, and it turns out the existing control flow already computes
> exactly that, so I kept the check in extents.c rather than
> duplicating
> fork-layout knowledge in hfs_bmap_reserve(): hfsplus_add_extent()
> returns -ENOSPC only when it has walked all eight slots and the last
> one can't be extended contiguously (the ++i >= 8 case). If there's an
> empty slot, or the last extent can be grown in place, it consumes
> that
> and returns 0 -- hfsplus_file_extend() never reaches the
> "insert_extent" label in that case. So arriving at insert_extent
> already means the fork is exhausted; no slot scan needed there.
> 
> v2, two hunks in the same function:
> 
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -458,6 +458,15 @@ int hfsplus_file_extend(struct inode *inode,
> bool zeroout)
>  	if (hip->alloc_blocks == hip->first_blocks)
>  		goal = hfsplus_ext_lastblock(hip->first_extents);
>  	else {
> +		/*
> +		 * The fork already claims more blocks than its
> eight extents
> +		 * describe (a corrupt on-disk fork): looking up the
> rest
> +		 * would re-enter hfs_find_init() on the extents
> tree, whose
> +		 * tree_lock is already held here.
> +		 */
> +		if (inode->i_ino == HFSPLUS_EXT_CNID) {
> +			res = -ENOSPC;
> +			goto out;
> +		}
>  		res = hfsplus_ext_read_extent(inode, hip-
> >alloc_blocks);

I think your logic here that if we try to read the extent from the
Extents Overflow file's content for the file itself, then something is
going wrong. In this case, we need to place this check into
hfsplus_ext_read_extent(). But I still don't see how we will check the
fork itself because it could be corrupted even without be completely
full? And how could we check the forks of other b-trees? 

>  		if (res)
>  			goto out;
> @@ -534,6 +543,15 @@ int hfsplus_file_extend(struct inode *inode,
> bool zeroout)
>  	return res;
> 
>  insert_extent:
> +	/*
> +	 * Getting here means the fork's eight extents are exhausted
> (see
> +	 * hfsplus_add_extent()). The extents overflow file can't
> record
> +	 * an overflow extent of its own, so it cannot grow any
> further.
> +	 */
> +	if (inode->i_ino == HFSPLUS_EXT_CNID) {
> +		res = -ENOSPC;
> +		goto out;
> +	}
> +

I assume that if we are here, then we already allocated the blocks for
the extent. And if we simply return the error here, then we've lost
these allocated blocks from the free space. Am I right? I think we need
to prevent the blocks allocation, then.

Thanks,
Slava.

>  	hfs_dbg("insert new extent\n");
>  	res = hfsplus_ext_write_extent_locked(inode);
>  	if (res)
> 
> First hunk: fork was already inconsistent when read from disk at
> mount. Second hunk: fork was consistent but genuinely ran out of the
> eight slots during this call -- your ENOSPC case. Both land on the
> same tree_lock recursion, so both need the guard.
> 
> On the severity split you described (consistent first extent +
> garbage
> elsewhere -> construct + flag inconsistent + read-only; unusable
> first
> extent -> hard error, mount fails): agreed, and that's the direction
> I'll take the fork-validator follow-up once this one's in, applying
> it
> to all three trees as you asked.
> 
> Both hunks build cleanly here.
> 
> Thanks,
> Thang

Re: [PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[Nguyen Ngoc Thang]

Hi Slava,

> I assume that if we are here, then we already allocated the blocks
> for the extent. And if we simply return the error here, then we've
> lost these allocated blocks from the free space. Am I right? I
> think we need to prevent the blocks allocation, then.

You're right, that was a real bug -- hfsplus_block_allocate() already
ran by the time we reach insert_extent, so returning straight from
there leaked start..start+len from the free space permanently. Fixed
by freeing them back before returning:

--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -537,6 +537,15 @@ int hfsplus_file_extend(struct inode *inode, bool zeroout)
 	return res;

 insert_extent:
+	/*
+	 * Getting here means the fork's eight extents are exhausted (see
+	 * hfsplus_add_extent()). The extents overflow file can't record
+	 * an overflow extent of its own, so it cannot grow any further;
+	 * give back the blocks just allocated for it above.
+	 */
+	if (inode->i_ino == HFSPLUS_EXT_CNID) {
+		if (hfsplus_block_free(sb, start, len))
+			pr_err("can't free extent: start %u, count %u\n",
+				start, len);
+		res = -ENOSPC;
+		goto out;
+	}
+
 	hfs_dbg("insert new extent\n");
 	res = hfsplus_ext_write_extent_locked(inode);

> I think your logic here that if we try to read the extent from the
> Extents Overflow file's content for the file itself, then something
> is going wrong. In this case, we need to place this check into
> hfsplus_ext_read_extent().

Agreed, moved it there -- it's the one place that actually calls
hfs_find_init() again, so this is now the single point enforcing the
invariant instead of duplicating it at each caller:

--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -216,6 +216,14 @@ static int hfsplus_ext_read_extent(struct inode *inode, u32 block)
 	    block < hip->cached_start + hip->cached_blocks)
 		return 0;

+	/*
+	 * The extents overflow file is fully described by its own fork
+	 * extents; looking up an overflow extent for it would re-enter
+	 * hfs_find_init() on the extents tree, whose tree_lock may already
+	 * be held by the caller.
+	 */
+	if (inode->i_ino == HFSPLUS_EXT_CNID)
+		return -ENOSPC;
+
 	res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);

This retires the guard I'd put in hfsplus_file_extend()'s else branch
(same check, called from the one place that mattered) -- v3 is net
smaller than v2. hfsplus_get_block()'s existing check at extents.c:261
(-EIO, before extents_lock is even taken) stays as-is; different
errno, different purpose -- fast rejection of a read, not an
allocation failure -- not an oversight.

> But I still don't see how we will check the fork itself because it
> could be corrupted even without be completely full? And how could
> we check the forks of other b-trees?

Fair, you've asked this three times now and I keep pushing it to
"follow-up" without saying what's in it, so concretely: a
hfsplus_check_fork(sb, fork, cnid) called from hfsplus_fill_super()
for ext_file/cat_file/attr_file, rejecting a fork where, for any of
the eight extents, block_count == 0 but start_block != 0 (garbage
in a slot that should be blank -- exactly what's in the syzbot image,
slots 3 and 6), or start_block + block_count > sbi->total_blocks
(extent points outside the volume), or a non-zero extent follows a
zero one (a hole in the middle of the used range). Wired into your
severity split: first extent fails those checks -> hfs_btree_open()
returns an error, mount fails; only later extents fail -> open the
tree, mark it inconsistent, force read-only. I'll send that as a
separate patch once this one lands, since it touches mount-time
behavior for all three trees and deserves review on its own.

Both hunks above build cleanly here.

Thanks,
Thang

Re: [PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[Viacheslav Dubeyko]

On Fri, 2026-09-11 at 18:46 +0700, Nguyen Ngoc Thang wrote:
> Hi Slava,
> 
> > I assume that if we are here, then we already allocated the blocks
> > for the extent. And if we simply return the error here, then we've
> > lost these allocated blocks from the free space. Am I right? I
> > think we need to prevent the blocks allocation, then.
> 
> You're right, that was a real bug -- hfsplus_block_allocate() already
> ran by the time we reach insert_extent, so returning straight from
> there leaked start..start+len from the free space permanently. Fixed
> by freeing them back before returning:
> 
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -537,6 +537,15 @@ int hfsplus_file_extend(struct inode *inode,
> bool zeroout)
>  	return res;
> 
>  insert_extent:
> +	/*
> +	 * Getting here means the fork's eight extents are exhausted
> (see
> +	 * hfsplus_add_extent()). The extents overflow file can't
> record
> +	 * an overflow extent of its own, so it cannot grow any
> further;
> +	 * give back the blocks just allocated for it above.
> +	 */
> +	if (inode->i_ino == HFSPLUS_EXT_CNID) {
> +		if (hfsplus_block_free(sb, start, len))
> +			pr_err("can't free extent: start %u, count
> %u\n",
> +				start, len);
> +		res = -ENOSPC;
> +		goto out;
> +	}
> +
>  	hfs_dbg("insert new extent\n");
>  	res = hfsplus_ext_write_extent_locked(inode);


Frankly speaking, I would prefer not to try to allocate at all but to
test the capability to allocate for the case of Extents Overflow file.
If we have free extent slots in the fork, then we can allocate and add
the extent. However, if we already used all extents in the fork, then
probability to find the necessary space is very low. So, we can the
method that tests the fork, something like hfsplus_add_extent() is
doing by without adding anything. If we can see that fork is full of
extents, then we need to be sure that we can extend the latest extent.
And we can simply test that the next adjacent block is free. And only
in this case it makes sense to try to allocate something. Does this
logic makes sense for you?

> 
> > I think your logic here that if we try to read the extent from the
> > Extents Overflow file's content for the file itself, then something
> > is going wrong. In this case, we need to place this check into
> > hfsplus_ext_read_extent().
> 
> Agreed, moved it there -- it's the one place that actually calls
> hfs_find_init() again, so this is now the single point enforcing the
> invariant instead of duplicating it at each caller:
> 
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -216,6 +216,14 @@ static int hfsplus_ext_read_extent(struct inode
> *inode, u32 block)
>  	    block < hip->cached_start + hip->cached_blocks)
>  		return 0;
> 
> +	/*
> +	 * The extents overflow file is fully described by its own
> fork
> +	 * extents; looking up an overflow extent for it would re-
> enter
> +	 * hfs_find_init() on the extents tree, whose tree_lock may
> already
> +	 * be held by the caller.
> +	 */
> +	if (inode->i_ino == HFSPLUS_EXT_CNID)
> +		return -ENOSPC;
> +
>  	res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
> 
> This retires the guard I'd put in hfsplus_file_extend()'s else branch
> (same check, called from the one place that mattered) -- v3 is net
> smaller than v2. hfsplus_get_block()'s existing check at
> extents.c:261
> (-EIO, before extents_lock is even taken) stays as-is; different
> errno, different purpose -- fast rejection of a read, not an
> allocation failure -- not an oversight.
> 
> > But I still don't see how we will check the fork itself because it
> > could be corrupted even without be completely full? And how could
> > we check the forks of other b-trees?
> 
> Fair, you've asked this three times now and I keep pushing it to
> "follow-up" without saying what's in it, so concretely: a
> hfsplus_check_fork(sb, fork, cnid) called from hfsplus_fill_super()
> for ext_file/cat_file/attr_file, rejecting a fork where, for any of
> the eight extents, block_count == 0 but start_block != 0 (garbage
> in a slot that should be blank -- exactly what's in the syzbot image,
> slots 3 and 6), or start_block + block_count > sbi->total_blocks
> (extent points outside the volume), or a non-zero extent follows a
> zero one (a hole in the middle of the used range). Wired into your
> severity split: first extent fails those checks -> hfs_btree_open()
> returns an error, mount fails; only later extents fail -> open the
> tree, mark it inconsistent, force read-only. I'll send that as a
> separate patch once this one lands, since it touches mount-time
> behavior for all three trees and deserves review on its own.

The bug can be treated as fixed only if the whole solution is in place.
So, please, send the whole pathset at once.

Thanks,
Slava.

> 
> Both hunks above build cleanly here.
> 
> Thanks,
> Thang

[PATCH v4 0/2] hfsplus: fix the extents overflow file recursive tree_lock and its root cause

[Nguyen Ngoc Thang]

syzbot: WARNING: possible recursive locking in hfsplus_find_init
https://syzkaller.appspot.com/bug?extid=f8ce6c197125ab9d72ce

This is the whole solution Slava asked for, in one series:

Patch 1 fixes the recursive tree_lock itself: hfsplus_ext_read_extent()
now refuses to look up an overflow extent for the extents overflow
file's own inode (it can never have one), and hfsplus_file_extend()
tests whether the fork actually has room before allocating blocks for
it, instead of allocating and undoing the allocation on failure.

Patch 2 addresses "how would we check the fork itself" and "how could
we check the forks of other b-trees", asked across the last few
rounds: hfsplus_check_fork() validates the eight fork extents of the
extents, catalog and attributes trees at hfs_btree_open() time,
following the severity split discussed -- first extent unusable ->
mount fails; a later extent corrupt (the syzbot image's case) -> open
read-only.

v3 -> v4:
 - hfsplus_file_extend() tests fork capacity before allocating instead
   of allocating and freeing on -ENOSPC (Slava)
 - added hfsplus_check_fork(), called from hfs_btree_open() for all
   three trees, with the mount-fail/read-only severity split (Slava)
 - hfsplus_reconfigure() and hfsplus_fill_super() check the resulting
   per-tree corrupt flag instead of re-deriving it on every remount

Nguyen Ngoc Thang (2):
  hfsplus: fix recursive tree_lock in hfsplus_file_extend()
  hfsplus: validate b-tree fork extents at mount time

 fs/hfsplus/btree.c      | 12 ++++++
 fs/hfsplus/extents.c    | 96 +++++++++++++++++++++++++++++++++++++++--
 fs/hfsplus/hfsplus_fs.h |  4 ++
 fs/hfsplus/super.c      |  9 ++++
 4 files changed, 117 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH v4 1/2] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[Nguyen Ngoc Thang]

hfs_bmap_reserve() calls hfsplus_file_extend() on tree->inode with
tree->tree_lock already held. For the extents overflow B-tree's own
inode, growing it can call hfsplus_ext_read_extent() -> hfs_find_init()
on that same tree, taking tree_lock a second time (lockdep: "possible
recursive locking ... &tree->tree_lock/1"). This happens two ways:

 - the fork already claims more blocks than its eight extents
   describe (a corrupted on-disk fork), so hfsplus_ext_read_extent()
   is called immediately to look up the rest; or
 - the fork's eight extents get exhausted during this call, and
   inserting a new overflow extent record for the file would need
   the same lookup.

Per the HFS+ format the extents overflow file is fully described by
its eight fork extents and can never legitimately have overflow
extents of its own, so both cases mean it cannot grow any further.

Move the check into hfsplus_ext_read_extent() itself, the one place
that actually re-enters hfs_find_init(), rather than duplicating it at
each caller, and report -ENOSPC.

For the second case, don't allocate blocks on the chance the fork
still has room and undo it if not: hfsplus_ext_fork_full() tests the
fork first. If it does have a free extent slot, any free space works,
same as before. If it's already full, the only way to grow is a
contiguous extension of the last extent, so only search for free
space starting exactly at the block right after it, and fail with
-ENOSPC immediately if that block isn't free -- nothing gets
allocated in that case, so there's nothing to undo. The prior
allocate-then-free-on-failure code stays at the insert_extent label
as a backstop, in case this reasoning has a gap.

Reported-by: syzbot+f8ce6c197125ab9d72ce@syzkaller.appspotmail.com
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---
 fs/hfsplus/extents.c | 59 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 4 deletions(-)

diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index eb7c11524d18..236f2d9a7a2d 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -84,6 +84,17 @@ static u32 hfsplus_ext_lastblock(struct hfsplus_extent *ext)
 	return be32_to_cpu(ext->start_block) + be32_to_cpu(ext->block_count);
 }
 
+/* True if all eight extents of a fork are in use (no free slot left) */
+static bool hfsplus_ext_fork_full(struct hfsplus_extent *ext)
+{
+	int i;
+
+	for (i = 0; i < 8; ext++, i++)
+		if (!ext->block_count)
+			return false;
+	return true;
+}
+
 static int __hfsplus_ext_write_extent(struct inode *inode,
 		struct hfs_find_data *fd)
 {
@@ -217,6 +228,15 @@ static int hfsplus_ext_read_extent(struct inode *inode, u32 block)
 	    block < hip->cached_start + hip->cached_blocks)
 		return 0;
 
+	/*
+	 * The extents overflow file is fully described by its own fork
+	 * extents; looking up an overflow extent for it would re-enter
+	 * hfs_find_init() on the extents tree, whose tree_lock may already
+	 * be held by the caller.
+	 */
+	if (inode->i_ino == HFSPLUS_EXT_CNID)
+		return -ENOSPC;
+
 	res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
 	if (!res) {
 		res = __hfsplus_ext_cache_extent(&fd, inode, block);
@@ -465,13 +485,30 @@ int hfsplus_file_extend(struct inode *inode, bool zeroout)
 	}
 
 	len = hip->clump_blocks;
-	start = hfsplus_block_allocate(sb, sbi->total_blocks, goal, &len);
-	if (start >= sbi->total_blocks) {
-		start = hfsplus_block_allocate(sb, goal, 0, &len);
-		if (start >= goal) {
+	if (inode->i_ino == HFSPLUS_EXT_CNID &&
+	    hip->alloc_blocks == hip->first_blocks &&
+	    hfsplus_ext_fork_full(hip->first_extents)) {
+		/*
+		 * No free slot is left in the fork, and the extents overflow
+		 * file can't record an overflow extent of its own: the only
+		 * way to grow it is a contiguous extension of the last
+		 * extent, so only accept free space starting exactly at
+		 * goal instead of allocating anywhere and having to undo it.
+		 */
+		start = hfsplus_block_allocate(sb, goal + 1, goal, &len);
+		if (start != goal) {
 			res = -ENOSPC;
 			goto out;
 		}
+	} else {
+		start = hfsplus_block_allocate(sb, sbi->total_blocks, goal, &len);
+		if (start >= sbi->total_blocks) {
+			start = hfsplus_block_allocate(sb, goal, 0, &len);
+			if (start >= goal) {
+				res = -ENOSPC;
+				goto out;
+			}
+		}
 	}
 
 	if (zeroout) {
@@ -526,6 +563,20 @@ int hfsplus_file_extend(struct inode *inode, bool zeroout)
 	return res;
 
 insert_extent:
+	/*
+	 * The fork-full precheck above keeps the extents overflow file's
+	 * own inode from ever landing here with blocks already allocated;
+	 * this is a backstop, so still free what was allocated rather
+	 * than leak it.
+	 */
+	if (inode->i_ino == HFSPLUS_EXT_CNID) {
+		if (hfsplus_block_free(sb, start, len))
+			pr_err("can't free extent: start %u, count %u\n",
+				start, len);
+		res = -ENOSPC;
+		goto out;
+	}
+
 	hfs_dbg("insert new extent\n");
 	res = hfsplus_ext_write_extent_locked(inode);
 	if (res)
-- 
2.43.0

[PATCH v4 2/2] hfsplus: validate b-tree fork extents at mount time

[Nguyen Ngoc Thang]

A hfsplus_check_fork() pass over a special file's eight fork extents,
called from hfs_btree_open() for the extents, catalog and attributes
trees:

 - block_count == 0 but start_block != 0: garbage left in a slot that
   should be blank (this is what the syzbot-reported image has in the
   extents overflow file's fork, slots 3 and 6);
 - start_block + block_count > sbi->total_blocks: an extent pointing
   past the end of the volume;
 - a non-zero extent following a zero one: a hole in the used range.

If the first extent itself fails these checks, the b-tree's location
on disk is unknown and there is nothing to recover, so hfs_btree_open()
fails as it already does for the other structural checks in that
function, and the mount fails.

If only a later extent is affected, the tree can still be opened (its
first extent, and hence its root node, is fine); mark it corrupt and
let the caller decide. hfsplus_fill_super() forces the volume
read-only in that case, and hfsplus_reconfigure() checks the same
per-tree flag on remount instead of re-deriving it, refusing to go
back to read-write. attr_tree may be NULL (volumes without an
attributes fork), so both checks guard for that.

This also gives the previous patch's hfsplus_file_extend() fix a
mount-time backstop: a fuzzed or damaged extents overflow fork like
the one in the syzbot report is caught here before any write ever
reaches it.

Reported-by: syzbot+f8ce6c197125ab9d72ce@syzkaller.appspotmail.com
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---
 fs/hfsplus/btree.c      | 12 ++++++++++++
 fs/hfsplus/extents.c    | 37 +++++++++++++++++++++++++++++++++++++
 fs/hfsplus/hfsplus_fs.h |  4 ++++
 fs/hfsplus/super.c      |  9 +++++++++
 4 files changed, 62 insertions(+)

diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index 2ea8cd5658e1..0a05ade53070 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -293,6 +293,18 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 		goto free_inode;
 	}
 
+	switch (hfsplus_check_fork(sb, HFSPLUS_I(tree->inode)->first_extents)) {
+	case -EIO:
+		pr_err("%s (cnid 0x%x) fork's first extent is corrupt\n",
+			hfs_btree_name(id), id);
+		goto free_inode;
+	case 1:
+		pr_warn("%s (cnid 0x%x) fork has corrupt extents, forcing read-only.\n",
+			hfs_btree_name(id), id);
+		tree->corrupt = true;
+		break;
+	}
+
 	mapping = tree->inode->i_mapping;
 	page = read_mapping_page(mapping, 0, NULL);
 	if (IS_ERR(page))
diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index 236f2d9a7a2d..a9303ce5bf8f 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -95,6 +95,43 @@ static bool hfsplus_ext_fork_full(struct hfsplus_extent *ext)
 	return true;
 }
 
+/*
+ * Check a fork's eight extents for the corruption a fuzzed or damaged
+ * volume header can contain: garbage in a slot that should be unused,
+ * an extent that runs past the end of the volume, or a used extent
+ * following an unused one.
+ *
+ * Returns 0 if the fork is fully consistent, 1 if only extents after
+ * the first are affected (the b-tree can still be located, so it's
+ * safe to mount read-only), or -EIO if the first extent itself is
+ * unusable.
+ */
+int hfsplus_check_fork(struct super_block *sb, struct hfsplus_extent *ext)
+{
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+	bool seen_hole = false;
+	int i;
+
+	for (i = 0; i < 8; i++, ext++) {
+		u32 start = be32_to_cpu(ext->start_block);
+		u32 count = be32_to_cpu(ext->block_count);
+		bool bad;
+
+		if (!count) {
+			bad = start != 0;
+			seen_hole = true;
+		} else {
+			bad = seen_hole || start + count < start ||
+			      start + count > sbi->total_blocks;
+		}
+
+		if (bad)
+			return i ? 1 : -EIO;
+	}
+
+	return 0;
+}
+
 static int __hfsplus_ext_write_extent(struct inode *inode,
 		struct hfs_find_data *fd)
 {
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 1e5b58e6a13f..8d47219e67d3 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -56,6 +56,9 @@ struct hfs_btree {
 	unsigned int max_key_len;
 	unsigned int depth;
 
+	/* fork extents past the first were found corrupt at open time */
+	bool corrupt;
+
 	struct mutex tree_lock;
 
 	unsigned int pages_per_bnode;
@@ -440,6 +443,7 @@ int hfsplus_free_fork(struct super_block *sb, u32 cnid,
 		      struct hfsplus_fork_raw *fork, int type);
 int hfsplus_file_extend(struct inode *inode, bool zeroout);
 void hfsplus_file_truncate(struct inode *inode);
+int hfsplus_check_fork(struct super_block *sb, struct hfsplus_extent *ext);
 
 /* inode.c */
 extern const struct address_space_operations hfsplus_aops;
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index ff7d6b3336a6..b65edb8ee589 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -400,6 +400,11 @@ static int hfsplus_reconfigure(struct fs_context *fc)
 			pr_warn("filesystem is marked journaled, leaving read-only.\n");
 			sb->s_flags |= SB_RDONLY;
 			fc->sb_flags |= SB_RDONLY;
+		} else if (sbi->ext_tree->corrupt || sbi->cat_tree->corrupt ||
+				(sbi->attr_tree && sbi->attr_tree->corrupt)) {
+			pr_warn("a b-tree fork was corrupt at mount time, leaving read-only.\n");
+			sb->s_flags |= SB_RDONLY;
+			fc->sb_flags |= SB_RDONLY;
 		}
 	}
 	return 0;
@@ -564,6 +569,10 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 	}
 	sb->s_xattr = hfsplus_xattr_handlers;
 
+	if (sbi->ext_tree->corrupt || sbi->cat_tree->corrupt ||
+	    (sbi->attr_tree && sbi->attr_tree->corrupt))
+		sb->s_flags |= SB_RDONLY;
+
 	inode = hfsplus_iget(sb, HFSPLUS_ALLOC_CNID);
 	if (IS_ERR(inode)) {
 		pr_err("failed to load allocation file\n");
-- 
2.43.0