fs/hfsplus/extents.c | 9 +++++++++ 1 file changed, 9 insertions(+)
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.0On 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;
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
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
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
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.
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
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
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
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
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
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
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
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.0On Sat, 2026-09-12 at 20:24 +0700, Nguyen Ngoc Thang wrote:
> 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)
static inline?
> +{
> + int i;
> +
> + for (i = 0; i < 8; ext++, i++)
I am introducing the special constant for the 8 extents of the fork in
HFS+ iomap patchset. How can we handle this? Because I would like to
see the named constant instead of hardcoded value.
> +
> 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.
> + */
The comment is not fully correct. We should not be here for the case of
Extents Overflow file because there is no forks other than in
superblock. It's not about the lock issue. We simply should not be here
at all.
> + if (inode->i_ino == HFSPLUS_EXT_CNID)
Maybe, we need to introduce something like is_extents_btree() method?
What do you think?
> + 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)) {
It looks like complicated condition and it deserves a static inline
function, from my point of view.
> + /*
> + * 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.
> + */
Maybe, instead of this long comment we need to introduce a dedicated
method for processing Extents Overflow file allocation case?
> + start = hfsplus_block_allocate(sb, goal + 1, goal,
> &len);
Maybe, I am missing something here. But goal + 1 sounds like we request
to allocate only one block. Is it correct? If yes, why only one block?
Usually, we need to try to allocate a clumpSize.
> + 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))
Can we be here at all? If start != goal, then we cannot allocate at
all. And we can be here only if we have empty slot it the fork. Am I
right?
Additional comment:
checkpatch.pl --strict flags one alignment style issue:
fs/hfsplus/extents.c:575: pr_err("can't free extent: start %u, count
%u\n",
start, len);
continuation should align with the open paren — cosmetic only
Thanks,
Slava.
> + 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)
Hi Slava,
Thanks a lot for the detailed review, this caught a real bug. Replies
inline, v5 diff at the bottom.
> static inline?
>
> > +{
> > + int i;
> > +
> > + for (i = 0; i < 8; ext++, i++)
>
> I am introducing the special constant for the 8 extents of the fork in
> HFS+ iomap patchset. How can we handle this? Because I would like to
> see the named constant instead of hardcoded value.
Added HFSPLUS_EXTENT_COUNT in hfsplus_fs.h and used it here. I don't
know what name you're using in the iomap patchset -- happy to rename
to match once you let me know, so we don't end up with two constants
for the same thing when that series lands.
I kept hfsplus_ext_fork_full() itself as plain "static", not "static
inline": it's not a single-line wrapper, and the compiler already
inlines small static functions like this at -O2, so an explicit
"inline" in a .c file (as opposed to a header) doesn't buy us
anything here. I did make the new one-line is_extents_btree() helper
below "static inline", since that one really is just a trivial
predicate wrapper.
> The comment is not fully correct. We should not be here for the case of
> Extents Overflow file because there is no forks other than in
> superblock. It's not about the lock issue. We simply should not be here
> at all.
You're right, fixed. The comment now says: "Per the HFS+ format, the
extents overflow file is fully described by its own eight fork
extents and can never have an overflow extent of its own recorded in
the tree; this function should never legitimately be reached for it."
> Maybe, we need to introduce something like is_extents_btree() method?
> What do you think?
Done -- added is_extents_btree() and used it at all three call sites
in this patch (hfsplus_ext_read_extent(), the fork-full check in
hfsplus_file_extend(), and the insert_extent backstop).
> It looks like complicated condition and it deserves a static inline
> function, from my point of view.
Extracted into hfsplus_ext_file_needs_contig_grow().
> Maybe, instead of this long comment we need to introduce a dedicated
> method for processing Extents Overflow file allocation case?
Extracted into hfsplus_ext_file_grow(), replacing the inline comment
with a doc comment on the function itself.
> Maybe, I am missing something here. But goal + 1 sounds like we request
> to allocate only one block. Is it correct? If yes, why only one block?
> Usually, we need to try to allocate a clumpSize.
You're right, and this was an actual bug, not just a readability
issue. I traced hfsplus_block_allocate(): the `size` argument bounds
both where the scan stops *and* the returned run length via
`len = min(size - start, len)`. With `size = goal + 1` and
`start = goal`, that clamps `len` to 1 no matter what clump_blocks
was, so this path only ever allocated a single block. Fixed to use
`goal + *len` (the original clump_blocks) as the bound instead, in
hfsplus_ext_file_grow(). That keeps the "must start exactly at goal"
rejection (still checked via `start != goal` by the caller) while
allowing a full clump to be granted when the space is there.
> Can we be here at all? If start != goal, then we cannot allocate at
> all. And we can be here only if we have empty slot it the fork. Am I
> right?
The other way around: this branch is taken when
hfsplus_ext_fork_full() returns true, i.e. there is *no* free slot
left in the fork. If there is a free slot, we fall through to the
regular allocate-anywhere path and hfsplus_add_extent() just records
it in that slot -- no special-casing needed. I renamed the condition
to hfsplus_ext_file_needs_contig_grow() to make that unambiguous.
> checkpatch.pl --strict flags one alignment style issue [...]
> continuation should align with the open paren — cosmetic only
Fixed.
Thanks again for catching the goal+1 bug in particular -- v5 below.
---
Changes since v4:
- Fix hfsplus_file_extend() requesting only 1 block instead of a
full clump when growing the extents overflow file's fork
(goal + 1 -> goal + len in the block_allocate() call).
- Add HFSPLUS_EXTENT_COUNT instead of hardcoding 8.
- Add is_extents_btree() instead of repeating the i_ino comparison.
- Extract hfsplus_ext_file_needs_contig_grow() and
hfsplus_ext_file_grow() out of hfsplus_file_extend().
- Fix comment on the HFSPLUS_EXT_CNID guard in
hfsplus_ext_read_extent() to state the real reason.
- Fix checkpatch --strict alignment nit on pr_err() continuation.
(all per Slava's review)
diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index eb7c11524d18..f3a4b8fd567f 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -84,6 +84,23 @@ static u32 hfsplus_ext_lastblock(struct hfsplus_extent *ext)
return be32_to_cpu(ext->start_block) + be32_to_cpu(ext->block_count);
}
+/* True if the inode is the extents overflow file's own inode */
+static inline bool is_extents_btree(struct inode *inode)
+{
+ return inode->i_ino == HFSPLUS_EXT_CNID;
+}
+
+/* True if all 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 < HFSPLUS_EXTENT_COUNT; 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 +234,15 @@ static int hfsplus_ext_read_extent(struct inode *inode, u32 block)
block < hip->cached_start + hip->cached_blocks)
return 0;
+ /*
+ * Per the HFS+ format, the extents overflow file is fully
+ * described by its own eight fork extents and can never have an
+ * overflow extent of its own recorded in the tree; this function
+ * should never legitimately be reached for it.
+ */
+ if (is_extents_btree(inode))
+ return -ENOSPC;
+
res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
if (!res) {
res = __hfsplus_ext_cache_extent(&fd, inode, block);
@@ -392,6 +418,34 @@ static int hfsplus_free_extents(struct super_block *sb,
}
}
+/*
+ * True when growing the extents overflow file's own inode needs the
+ * contiguous-only special case below: its fork's eight extents are
+ * all in use, so there is no free slot left to record a new extent
+ * for it.
+ */
+static bool hfsplus_ext_file_needs_contig_grow(struct inode *inode,
+ struct hfsplus_inode_info *hip)
+{
+ return is_extents_btree(inode) &&
+ hip->alloc_blocks == hip->first_blocks &&
+ hfsplus_ext_fork_full(hip->first_extents);
+}
+
+/*
+ * Allocate blocks to grow the extents overflow file itself once its
+ * fork is full (see hfsplus_ext_file_needs_contig_grow()). Per the
+ * HFS+ format this file can never record an overflow extent of its
+ * own, so the only way to grow it further is a contiguous extension
+ * of the last extent already in the fork: search for up to *len free
+ * blocks starting exactly at goal, and return a start block other
+ * than goal if the block at goal itself isn't free.
+ */
+static u32 hfsplus_ext_file_grow(struct super_block *sb, u32 goal, u32 *len)
+{
+ return hfsplus_block_allocate(sb, goal + *len, goal, len);
+}
+
int hfsplus_free_fork(struct super_block *sb, u32 cnid,
struct hfsplus_fork_raw *fork, int type)
{
@@ -465,13 +519,21 @@ 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 (hfsplus_ext_file_needs_contig_grow(inode, hip)) {
+ start = hfsplus_ext_file_grow(sb, 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 +588,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 (is_extents_btree(inode)) {
+ 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)
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 1e5b58e6a13f..7c53832f2784 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -24,6 +24,9 @@
#define HFSPLUS_TYPE_DATA 0x00
#define HFSPLUS_TYPE_RSRC 0xFF
+/* Number of extent slots in a fork (hfsplus_extent_rec, hfs_common.h) */
+#define HFSPLUS_EXTENT_COUNT 8
+
typedef int (*btree_keycmp)(const hfsplus_btree_key *,
const hfsplus_btree_key *);
--
Thanks,
Nguyen Ngoc Thang
On Tue, 2026-09-15 at 21:07 +0700, Nguyen Ngoc Thang wrote:
> Hi Slava,
>
> Thanks a lot for the detailed review, this caught a real bug. Replies
> inline, v5 diff at the bottom.
>
> > static inline?
> >
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < 8; ext++, i++)
> >
> > I am introducing the special constant for the 8 extents of the fork
> > in
> > HFS+ iomap patchset. How can we handle this? Because I would like
> > to
> > see the named constant instead of hardcoded value.
>
> Added HFSPLUS_EXTENT_COUNT in hfsplus_fs.h and used it here. I don't
> know what name you're using in the iomap patchset -- happy to rename
> to match once you let me know, so we don't end up with two constants
> for the same thing when that series lands.
You cannot simply declare the same constant because our patchsets will
conflict. Probably, you need to keep 8 as hardcoded value now. And it
will be good to make the refactoring after my patchset will be in
HFS/HFS+ git tree.
>
> I kept hfsplus_ext_fork_full() itself as plain "static", not "static
> inline": it's not a single-line wrapper, and the compiler already
> inlines small static functions like this at -O2, so an explicit
> "inline" in a .c file (as opposed to a header) doesn't buy us
> anything here. I did make the new one-line is_extents_btree() helper
> below "static inline", since that one really is just a trivial
> predicate wrapper.
>
> > The comment is not fully correct. We should not be here for the
> > case of
> > Extents Overflow file because there is no forks other than in
> > superblock. It's not about the lock issue. We simply should not be
> > here
> > at all.
>
> You're right, fixed. The comment now says: "Per the HFS+ format, the
> extents overflow file is fully described by its own eight fork
> extents and can never have an overflow extent of its own recorded in
> the tree; this function should never legitimately be reached for it."
>
> > Maybe, we need to introduce something like is_extents_btree()
> > method?
> > What do you think?
>
> Done -- added is_extents_btree() and used it at all three call sites
> in this patch (hfsplus_ext_read_extent(), the fork-full check in
> hfsplus_file_extend(), and the insert_extent backstop).
>
> > It looks like complicated condition and it deserves a static inline
> > function, from my point of view.
>
> Extracted into hfsplus_ext_file_needs_contig_grow().
>
> > Maybe, instead of this long comment we need to introduce a
> > dedicated
> > method for processing Extents Overflow file allocation case?
>
> Extracted into hfsplus_ext_file_grow(), replacing the inline comment
> with a doc comment on the function itself.
>
> > Maybe, I am missing something here. But goal + 1 sounds like we
> > request
> > to allocate only one block. Is it correct? If yes, why only one
> > block?
> > Usually, we need to try to allocate a clumpSize.
>
> You're right, and this was an actual bug, not just a readability
> issue. I traced hfsplus_block_allocate(): the `size` argument bounds
> both where the scan stops *and* the returned run length via
> `len = min(size - start, len)`. With `size = goal + 1` and
> `start = goal`, that clamps `len` to 1 no matter what clump_blocks
> was, so this path only ever allocated a single block. Fixed to use
> `goal + *len` (the original clump_blocks) as the bound instead, in
> hfsplus_ext_file_grow(). That keeps the "must start exactly at goal"
> rejection (still checked via `start != goal` by the caller) while
> allowing a full clump to be granted when the space is there.
>
> > Can we be here at all? If start != goal, then we cannot allocate at
> > all. And we can be here only if we have empty slot it the fork. Am
> > I
> > right?
>
> The other way around: this branch is taken when
> hfsplus_ext_fork_full() returns true, i.e. there is *no* free slot
> left in the fork. If there is a free slot, we fall through to the
> regular allocate-anywhere path and hfsplus_add_extent() just records
> it in that slot -- no special-casing needed. I renamed the condition
> to hfsplus_ext_file_needs_contig_grow() to make that unambiguous.
>
> > checkpatch.pl --strict flags one alignment style issue [...]
> > continuation should align with the open paren — cosmetic only
>
> Fixed.
>
> Thanks again for catching the goal+1 bug in particular -- v5 below.
I cannot treat as a v5 of the patch because it's not the patch but
simple discussion. And it makes the review process really complicated.
Please, don't mess the discussion with the formal patches.
>
> ---
> Changes since v4:
> - Fix hfsplus_file_extend() requesting only 1 block instead of a
> full clump when growing the extents overflow file's fork
> (goal + 1 -> goal + len in the block_allocate() call).
> - Add HFSPLUS_EXTENT_COUNT instead of hardcoding 8.
> - Add is_extents_btree() instead of repeating the i_ino comparison.
> - Extract hfsplus_ext_file_needs_contig_grow() and
> hfsplus_ext_file_grow() out of hfsplus_file_extend().
> - Fix comment on the HFSPLUS_EXT_CNID guard in
> hfsplus_ext_read_extent() to state the real reason.
> - Fix checkpatch --strict alignment nit on pr_err() continuation.
> (all per Slava's review)
>
> diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
> index eb7c11524d18..f3a4b8fd567f 100644
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -84,6 +84,23 @@ static u32 hfsplus_ext_lastblock(struct
> hfsplus_extent *ext)
> return be32_to_cpu(ext->start_block) + be32_to_cpu(ext-
> >block_count);
> }
>
> +/* True if the inode is the extents overflow file's own inode */
This comment is useless because the name is informative enough.
> +static inline bool is_extents_btree(struct inode *inode)
> +{
> + return inode->i_ino == HFSPLUS_EXT_CNID;
> +}
> +
> +/* True if all extents of a fork are in use (no free slot left) */
I assume that you are practicing AI assistant a lot. Please, clean upo
useless comments after this stuff. The name of function is informative
enough.
> +static bool hfsplus_ext_fork_full(struct hfsplus_extent *ext)
hfsplus_fork_full()...
If you would like to be sure that fork is not corrupted and it is full,
then you need to analyze the fork structure. Otherwise, it is enough to
check the latest extent in the fork.
> +{
> + int i;
> +
> + for (i = 0; i < HFSPLUS_EXTENT_COUNT; 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 +234,15 @@ static int hfsplus_ext_read_extent(struct inode
> *inode, u32 block)
> block < hip->cached_start + hip->cached_blocks)
> return 0;
>
> + /*
> + * Per the HFS+ format, the extents overflow file is fully
> + * described by its own eight fork extents and can never
> have an
> + * overflow extent of its own recorded in the tree; this
> function
> + * should never legitimately be reached for it.
> + */
> + if (is_extents_btree(inode))
> + return -ENOSPC;
> +
> res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
> if (!res) {
> res = __hfsplus_ext_cache_extent(&fd, inode, block);
> @@ -392,6 +418,34 @@ static int hfsplus_free_extents(struct
> super_block *sb,
> }
> }
>
> +/*
> + * True when growing the extents overflow file's own inode needs the
> + * contiguous-only special case below: its fork's eight extents are
> + * all in use, so there is no free slot left to record a new extent
> + * for it.
> + */
Comment is longer than the function itself. It is not necessary at all.
> +static bool hfsplus_ext_file_needs_contig_grow(struct inode *inode,
> + struct
> hfsplus_inode_info *hip)
is_ext_file_need_grow() ?
> +{
> + return is_extents_btree(inode) &&
> + hip->alloc_blocks == hip->first_blocks &&
> + hfsplus_ext_fork_full(hip->first_extents);
> +}
> +
> +/*
> + * Allocate blocks to grow the extents overflow file itself once its
> + * fork is full (see hfsplus_ext_file_needs_contig_grow()). Per the
> + * HFS+ format this file can never record an overflow extent of its
> + * own, so the only way to grow it further is a contiguous extension
> + * of the last extent already in the fork: search for up to *len
> free
> + * blocks starting exactly at goal, and return a start block other
> + * than goal if the block at goal itself isn't free.
> + */
Ditto.
> +static u32 hfsplus_ext_file_grow(struct super_block *sb, u32 goal,
> u32 *len)
> +{
> + return hfsplus_block_allocate(sb, goal + *len, goal, len);
> +}
This doesn't make sense at all.
> +
> int hfsplus_free_fork(struct super_block *sb, u32 cnid,
> struct hfsplus_fork_raw *fork, int type)
> {
> @@ -465,13 +519,21 @@ 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 (hfsplus_ext_file_needs_contig_grow(inode, hip)) {
> + start = hfsplus_ext_file_grow(sb, 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;
> + }
> + }
This didn't make the situation better. Probably, all this piece of code
should be one function.
>
> }
>
> if (zeroout) {
> @@ -526,6 +588,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 (is_extents_btree(inode)) {
> + if (hfsplus_block_free(sb, start, len))
> + pr_err("can't free extent: start %u, count
> %u\n",
> + start, len);
> + res = -ENOSPC;
> + goto out;
> + }
It's hard to discuss if you are moving discussion out of the code. My
question still the same here. Because I cannot connect your answer with
my question and code.
Thanks,
Slava.
> +
> hfs_dbg("insert new extent\n");
> res = hfsplus_ext_write_extent_locked(inode);
> if (res)
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 1e5b58e6a13f..7c53832f2784 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -24,6 +24,9 @@
> #define HFSPLUS_TYPE_DATA 0x00
> #define HFSPLUS_TYPE_RSRC 0xFF
>
> +/* Number of extent slots in a fork (hfsplus_extent_rec,
> hfs_common.h) */
> +#define HFSPLUS_EXTENT_COUNT 8
> +
> typedef int (*btree_keycmp)(const hfsplus_btree_key *,
> const hfsplus_btree_key *);
>
> --
> Thanks,
> 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.0On Sat, 2026-09-12 at 20:24 +0700, Nguyen Ngoc Thang wrote:
> 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)) {
If we return error code for corrupted fork (that makes more sense),
then we don't need in switch here.
> + 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:
I don't see the point returning 1 from the function. It should be error
code.
> + 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)
Why not struct hfsplus_fork_raw here for check?
> +{
> + struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> + bool seen_hole = false;
> + int i;
> +
> + for (i = 0; i < 8; i++, ext++) {
Ditto. Related to hardcoded value.
> + 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;
> + }
I think that current logic of check looks complicated. And I think not
all possible cases are checked. For example, fork cannot be completely
empty. Could we rework the logic to be more clear? Maybe, we need to
introduce the function for extent check, function for checking the
extents are logically contiguous?
Also, the fork contains more details to check:
struct hfsplus_fork_raw {
__be64 total_size;
__be32 clump_size;
__be32 total_blocks;
hfsplus_extent_rec extents;
} __packed;
Why are we not check the fork itself?
> +
> + if (bad)
> + return i ? 1 : -EIO;
Ditto. Related to 1. I prefer to have error code instead.
> + }
> +
> + 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;
> +
I don't want to say that this direction is wrong. However, we have
flags:
#define HFSPLUS_I_CAT_DIRTY 1 /* has changes in the catalog
tree */
#define HFSPLUS_I_EXT_DIRTY 2 /* has changes in the extent
tree */
#define HFSPLUS_I_ALLOC_DIRTY 3 /* has changes in the
allocation file */
#define HFSPLUS_I_ATTR_DIRTY 4 /* has changes in the
attributes tree */
And we are using inode's flag to track the dirty state of the tree.
Potentially, we can introduce the HFSPLUS_I_CORRUPT_TREE. And I think
one flags for all b-tree will be enough because inode is dedicated for
a particular tree. What do you think?
> 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)) {
Currently, only hfsplus_fill_super() can detect the b-tree corruption.
Why do we have the check here? Do you mean that xattr b-tree can be
created and to be corrupted?
> + 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;
If we fail to check any b-tree, then logic should stop. Why haven't we
checked the error code of hfs_btree_open()?
Thanks,
Slava.
> +
> inode = hfsplus_iget(sb, HFSPLUS_ALLOC_CNID);
> if (IS_ERR(inode)) {
> pr_err("failed to load allocation file\n");
Hi Slava,
Thanks again for the review, replies inline, v5 diff (applies on top
of the v5 1/2 I just sent) at the bottom.
> If we return error code for corrupted fork (that makes more sense),
> then we don't need in switch here.
>
> > + 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:
>
> I don't see the point returning 1 from the function. It should be error
> code.
Agreed, done. hfsplus_check_fork() now returns 0 (consistent),
-EUCLEAN (corrupt past the first extent, tree still locatable, mount
read-only), or -EIO (first extent corrupt, or no used extent at all --
see below). hfs_btree_open() now just checks the return value with
if/else instead of switching on it.
> Why not struct hfsplus_fork_raw here for check?
I looked into this, but the b-tree's inode only keeps the decoded
first_extents/first_blocks fields (see hfsplus_iget()), not the raw
hfsplus_fork_raw (total_size/clump_size/total_blocks as a struct) --
that only exists transiently while reading the volume header. Passing
the raw fork through would mean plumbing it from hfsplus_fill_super()
into hfs_btree_open() as an extra argument, which felt like a bigger
restructuring than this patch should take on. I'd rather scope that as
a follow-up than guess at it here -- let me know if you disagree and
I'll take a pass at it.
> Ditto. Related to hardcoded value.
>
> > + for (i = 0; i < 8; i++, ext++) {
Uses HFSPLUS_EXTENT_COUNT now too (same constant added in patch 1/2).
> I think that current logic of check looks complicated. [...] For
> example, fork cannot be completely empty. Could we rework the logic
> to be more clear? Maybe, we need to introduce the function for extent
> check, function for checking the extents are logically contiguous?
Fixed the empty-fork case: hfsplus_check_fork() now tracks seen_used
and returns -EIO if no extent was ever in use. Note hs_btree_open()
already guarded against this indirectly via its existing
`!first_blocks` check right before calling hfsplus_check_fork(), so
this makes the function correct on its own instead of relying on that
caller-side check.
I held off on splitting per-extent-check and contiguity-check into
separate functions -- the loop is short and the two conditions
(garbage in an unused slot vs. a used extent overflowing/following a
hole) share the same start/count/seen_hole state per iteration, so
splitting it looked like it'd add indirection without really
clarifying anything. Happy to revisit if you still think it's worth
it.
> Ditto. Related to 1. I prefer to have error code instead.
Same fix as above (-EUCLEAN).
> I don't want to say that this direction is wrong. However, we have
> flags: [...] Potentially, we can introduce the HFSPLUS_I_CORRUPT_TREE.
Done -- dropped struct hfs_btree.corrupt, added HFSPLUS_I_CORRUPT_TREE
next to the existing HFSPLUS_I_*_DIRTY flags, tested via a new
HFSPLUS_TREE_IS_CORRUPT(tree) helper macro on the tree's own inode.
> Currently, only hfsplus_fill_super() can detect the b-tree
> corruption. Why do we have the check here? Do you mean that xattr
> b-tree can be created and to be corrupted?
No -- corruption is only ever detected once, in hfs_btree_open() at
initial mount. The hfsplus_reconfigure() check isn't detecting
anything new; it's re-reading the flag hfs_btree_open() already set,
so that a remount to rw can't silently clear SB_RDONLY on a volume
that was already known to be corrupt at mount time. Added a short
comment there to make that explicit.
> If we fail to check any b-tree, then logic should stop. Why haven't
> we checked the error code of hfs_btree_open()?
I checked -- hfsplus_fill_super() already does check every
hfs_btree_open() call (out_close_ext_tree / out_close_cat_tree /
out_close_attr_tree gotos) before it ever looks at
HFSPLUS_TREE_IS_CORRUPT(), so no change was needed there.
One more from the previous mail I noticed while redoing this: there
was also a checkpatch --strict alignment nit on the pr_err()
continuation line in hfs_btree_open() itself (not one you'd flagged,
but same category), fixed that too while I was in there.
Thanks again for the thorough review -- v5 below.
---
Changes since v4:
- hfsplus_check_fork() returns real error codes (0/-EUCLEAN/-EIO)
instead of 0/1/-EIO; hfs_btree_open() uses if/else instead of a
switch.
- A fork with no used extent at all is now treated as corrupt.
- Use HFSPLUS_EXTENT_COUNT instead of hardcoding 8.
- Track per-tree corruption as an HFSPLUS_I_CORRUPT_TREE inode flag
instead of a bool on struct hfs_btree.
- Comment explaining the corrupt-tree check in hfsplus_reconfigure().
- Fixed a checkpatch --strict alignment nit in hfs_btree_open().
(all per Slava's review)
diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index 2ea8cd5658e1..2dbbb8096575 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -274,6 +274,7 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
struct inode *inode;
struct page *page;
unsigned int size;
+ int res;
tree = kzalloc_obj(*tree);
if (!tree)
@@ -293,6 +294,17 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
goto free_inode;
}
+ res = hfsplus_check_fork(sb, HFSPLUS_I(tree->inode)->first_extents);
+ if (res == -EIO) {
+ pr_err("%s (cnid 0x%x) fork's first extent is corrupt\n",
+ hfs_btree_name(id), id);
+ goto free_inode;
+ } else if (res) {
+ pr_warn("%s (cnid 0x%x) fork has corrupt extents, forcing read-only.\n",
+ hfs_btree_name(id), id);
+ set_bit(HFSPLUS_I_CORRUPT_TREE, &HFSPLUS_I(tree->inode)->flags);
+ }
+
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 f3a4b8fd567f..d98261c01b13 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -101,6 +101,39 @@ static bool hfsplus_ext_fork_full(struct hfsplus_extent *ext)
return true;
}
+/*
+ * Validate a fork's extents. Returns 0 if consistent, -EUCLEAN if only
+ * extents past the first are corrupt (safe to mount read-only), or
+ * -EIO if the first extent is corrupt or the fork has no used extent.
+ */
+int hfsplus_check_fork(struct super_block *sb, struct hfsplus_extent *ext)
+{
+ struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+ bool seen_hole = false;
+ bool seen_used = false;
+ int i;
+
+ for (i = 0; i < HFSPLUS_EXTENT_COUNT; 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;
+ seen_used = true;
+ }
+
+ if (bad)
+ return i ? -EUCLEAN : -EIO;
+ }
+
+ return seen_used ? 0 : -EIO;
+}
+
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 7c53832f2784..3290812c0fea 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -230,10 +230,15 @@ struct hfsplus_inode_info {
#define HFSPLUS_I_EXT_DIRTY 2 /* has changes in the extent tree */
#define HFSPLUS_I_ALLOC_DIRTY 3 /* has changes in the allocation file */
#define HFSPLUS_I_ATTR_DIRTY 4 /* has changes in the attributes tree */
+#define HFSPLUS_I_CORRUPT_TREE 5 /* tree's fork had corrupt extents at open time */
#define HFSPLUS_IS_RSRC(inode) \
test_bit(HFSPLUS_I_RSRC, &HFSPLUS_I(inode)->flags)
+/* Test HFSPLUS_I_CORRUPT_TREE on the tree's own inode */
+#define HFSPLUS_TREE_IS_CORRUPT(tree) \
+ test_bit(HFSPLUS_I_CORRUPT_TREE, &HFSPLUS_I((tree)->inode)->flags)
+
static inline struct hfsplus_inode_info *HFSPLUS_I(struct inode *inode)
{
return container_of(inode, struct hfsplus_inode_info, vfs_inode);
@@ -443,6 +448,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..a1669bd45701 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -400,6 +400,14 @@ 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_TREE_IS_CORRUPT(sbi->ext_tree) ||
+ HFSPLUS_TREE_IS_CORRUPT(sbi->cat_tree) ||
+ (sbi->attr_tree &&
+ HFSPLUS_TREE_IS_CORRUPT(sbi->attr_tree))) {
+ /* Re-checks the flag hfs_btree_open() set at mount */
+ 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 +572,11 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
}
sb->s_xattr = hfsplus_xattr_handlers;
+ if (HFSPLUS_TREE_IS_CORRUPT(sbi->ext_tree) ||
+ HFSPLUS_TREE_IS_CORRUPT(sbi->cat_tree) ||
+ (sbi->attr_tree && HFSPLUS_TREE_IS_CORRUPT(sbi->attr_tree)))
+ sb->s_flags |= SB_RDONLY;
+
inode = hfsplus_iget(sb, HFSPLUS_ALLOC_CNID);
if (IS_ERR(inode)) {
pr_err("failed to load allocation file\n");
--
Thanks,
Nguyen Ngoc Thang
Hi Nguyen Ngoc,
Please, don't move your answers from the code where I left my
questions. I really cannot follow to your answers and the whole
discussion is broken. I cannot follow to your answers. I am simply
rejecting the whole email.
Thanks,
Slava.
On Tue, 2026-09-15 at 21:15 +0700, Nguyen Ngoc Thang wrote:
> Hi Slava,
>
> Thanks again for the review, replies inline, v5 diff (applies on top
> of the v5 1/2 I just sent) at the bottom.
>
> > If we return error code for corrupted fork (that makes more sense),
> > then we don't need in switch here.
> >
> > > + 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:
> >
> > I don't see the point returning 1 from the function. It should be
> > error
> > code.
>
> Agreed, done. hfsplus_check_fork() now returns 0 (consistent),
> -EUCLEAN (corrupt past the first extent, tree still locatable, mount
> read-only), or -EIO (first extent corrupt, or no used extent at all -
> -
> see below). hfs_btree_open() now just checks the return value with
> if/else instead of switching on it.
>
> > Why not struct hfsplus_fork_raw here for check?
>
> I looked into this, but the b-tree's inode only keeps the decoded
> first_extents/first_blocks fields (see hfsplus_iget()), not the raw
> hfsplus_fork_raw (total_size/clump_size/total_blocks as a struct) --
> that only exists transiently while reading the volume header. Passing
> the raw fork through would mean plumbing it from hfsplus_fill_super()
> into hfs_btree_open() as an extra argument, which felt like a bigger
> restructuring than this patch should take on. I'd rather scope that
> as
> a follow-up than guess at it here -- let me know if you disagree and
> I'll take a pass at it.
>
> > Ditto. Related to hardcoded value.
> >
> > > + for (i = 0; i < 8; i++, ext++) {
>
> Uses HFSPLUS_EXTENT_COUNT now too (same constant added in patch 1/2).
>
> > I think that current logic of check looks complicated. [...] For
> > example, fork cannot be completely empty. Could we rework the logic
> > to be more clear? Maybe, we need to introduce the function for
> > extent
> > check, function for checking the extents are logically contiguous?
>
> Fixed the empty-fork case: hfsplus_check_fork() now tracks seen_used
> and returns -EIO if no extent was ever in use. Note hs_btree_open()
> already guarded against this indirectly via its existing
> `!first_blocks` check right before calling hfsplus_check_fork(), so
> this makes the function correct on its own instead of relying on that
> caller-side check.
>
> I held off on splitting per-extent-check and contiguity-check into
> separate functions -- the loop is short and the two conditions
> (garbage in an unused slot vs. a used extent overflowing/following a
> hole) share the same start/count/seen_hole state per iteration, so
> splitting it looked like it'd add indirection without really
> clarifying anything. Happy to revisit if you still think it's worth
> it.
>
> > Ditto. Related to 1. I prefer to have error code instead.
>
> Same fix as above (-EUCLEAN).
>
> > I don't want to say that this direction is wrong. However, we have
> > flags: [...] Potentially, we can introduce the
> > HFSPLUS_I_CORRUPT_TREE.
>
> Done -- dropped struct hfs_btree.corrupt, added
> HFSPLUS_I_CORRUPT_TREE
> next to the existing HFSPLUS_I_*_DIRTY flags, tested via a new
> HFSPLUS_TREE_IS_CORRUPT(tree) helper macro on the tree's own inode.
>
> > Currently, only hfsplus_fill_super() can detect the b-tree
> > corruption. Why do we have the check here? Do you mean that xattr
> > b-tree can be created and to be corrupted?
>
> No -- corruption is only ever detected once, in hfs_btree_open() at
> initial mount. The hfsplus_reconfigure() check isn't detecting
> anything new; it's re-reading the flag hfs_btree_open() already set,
> so that a remount to rw can't silently clear SB_RDONLY on a volume
> that was already known to be corrupt at mount time. Added a short
> comment there to make that explicit.
>
> > If we fail to check any b-tree, then logic should stop. Why haven't
> > we checked the error code of hfs_btree_open()?
>
> I checked -- hfsplus_fill_super() already does check every
> hfs_btree_open() call (out_close_ext_tree / out_close_cat_tree /
> out_close_attr_tree gotos) before it ever looks at
> HFSPLUS_TREE_IS_CORRUPT(), so no change was needed there.
>
> One more from the previous mail I noticed while redoing this: there
> was also a checkpatch --strict alignment nit on the pr_err()
> continuation line in hfs_btree_open() itself (not one you'd flagged,
> but same category), fixed that too while I was in there.
>
> Thanks again for the thorough review -- v5 below.
>
> ---
> Changes since v4:
> - hfsplus_check_fork() returns real error codes (0/-EUCLEAN/-EIO)
> instead of 0/1/-EIO; hfs_btree_open() uses if/else instead of a
> switch.
> - A fork with no used extent at all is now treated as corrupt.
> - Use HFSPLUS_EXTENT_COUNT instead of hardcoding 8.
> - Track per-tree corruption as an HFSPLUS_I_CORRUPT_TREE inode flag
> instead of a bool on struct hfs_btree.
> - Comment explaining the corrupt-tree check in
> hfsplus_reconfigure().
> - Fixed a checkpatch --strict alignment nit in hfs_btree_open().
> (all per Slava's review)
>
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index 2ea8cd5658e1..2dbbb8096575 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -274,6 +274,7 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id)
> struct inode *inode;
> struct page *page;
> unsigned int size;
> + int res;
>
> tree = kzalloc_obj(*tree);
> if (!tree)
> @@ -293,6 +294,17 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id)
> goto free_inode;
> }
>
> + res = hfsplus_check_fork(sb, HFSPLUS_I(tree->inode)-
> >first_extents);
> + if (res == -EIO) {
> + pr_err("%s (cnid 0x%x) fork's first extent is
> corrupt\n",
> + hfs_btree_name(id), id);
> + goto free_inode;
> + } else if (res) {
> + pr_warn("%s (cnid 0x%x) fork has corrupt extents,
> forcing read-only.\n",
> + hfs_btree_name(id), id);
> + set_bit(HFSPLUS_I_CORRUPT_TREE, &HFSPLUS_I(tree-
> >inode)->flags);
> + }
> +
> 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 f3a4b8fd567f..d98261c01b13 100644
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -101,6 +101,39 @@ static bool hfsplus_ext_fork_full(struct
> hfsplus_extent *ext)
> return true;
> }
>
> +/*
> + * Validate a fork's extents. Returns 0 if consistent, -EUCLEAN if
> only
> + * extents past the first are corrupt (safe to mount read-only), or
> + * -EIO if the first extent is corrupt or the fork has no used
> extent.
> + */
> +int hfsplus_check_fork(struct super_block *sb, struct hfsplus_extent
> *ext)
> +{
> + struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> + bool seen_hole = false;
> + bool seen_used = false;
> + int i;
> +
> + for (i = 0; i < HFSPLUS_EXTENT_COUNT; 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;
> + seen_used = true;
> + }
> +
> + if (bad)
> + return i ? -EUCLEAN : -EIO;
> + }
> +
> + return seen_used ? 0 : -EIO;
> +}
> +
> 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 7c53832f2784..3290812c0fea 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -230,10 +230,15 @@ struct hfsplus_inode_info {
> #define HFSPLUS_I_EXT_DIRTY 2 /* has changes in the extent
> tree */
> #define HFSPLUS_I_ALLOC_DIRTY 3 /* has changes in the
> allocation file */
> #define HFSPLUS_I_ATTR_DIRTY 4 /* has changes in the
> attributes tree */
> +#define HFSPLUS_I_CORRUPT_TREE 5 /* tree's fork had corrupt
> extents at open time */
>
> #define HFSPLUS_IS_RSRC(inode) \
> test_bit(HFSPLUS_I_RSRC, &HFSPLUS_I(inode)->flags)
>
> +/* Test HFSPLUS_I_CORRUPT_TREE on the tree's own inode */
> +#define HFSPLUS_TREE_IS_CORRUPT(tree) \
> + test_bit(HFSPLUS_I_CORRUPT_TREE, &HFSPLUS_I((tree)->inode)-
> >flags)
> +
> static inline struct hfsplus_inode_info *HFSPLUS_I(struct inode
> *inode)
> {
> return container_of(inode, struct hfsplus_inode_info,
> vfs_inode);
> @@ -443,6 +448,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..a1669bd45701 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -400,6 +400,14 @@ 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_TREE_IS_CORRUPT(sbi->ext_tree) ||
> + HFSPLUS_TREE_IS_CORRUPT(sbi-
> >cat_tree) ||
> + (sbi->attr_tree &&
> + HFSPLUS_TREE_IS_CORRUPT(sbi-
> >attr_tree))) {
> + /* Re-checks the flag hfs_btree_open() set
> at mount */
> + 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 +572,11 @@ static int hfsplus_fill_super(struct super_block
> *sb, struct fs_context *fc)
> }
> sb->s_xattr = hfsplus_xattr_handlers;
>
> + if (HFSPLUS_TREE_IS_CORRUPT(sbi->ext_tree) ||
> + HFSPLUS_TREE_IS_CORRUPT(sbi->cat_tree) ||
> + (sbi->attr_tree && HFSPLUS_TREE_IS_CORRUPT(sbi-
> >attr_tree)))
> + sb->s_flags |= SB_RDONLY;
> +
> inode = hfsplus_iget(sb, HFSPLUS_ALLOC_CNID);
> if (IS_ERR(inode)) {
> pr_err("failed to load allocation file\n");
> --
> Thanks,
> Nguyen Ngoc Thang
On Tue, 2026-09-15 at 16:48 -0700, Viacheslav Dubeyko wrote: > Hi Nguyen Ngoc, > > Please, don't move your answers from the code where I left my > questions. I really cannot follow to your answers and the whole > discussion is broken. I cannot follow to your answers. I am simply > rejecting the whole email. > Could you please send the formal patch v5 and we can re-start the discussion from this point? It's really hard to continue discussion if the questions and answers have been moved from the code context. And it really complicates the review if you mess the discussion and new code together. Please, send new state of the code as a formal patch but not the part of the discussion. This mixture blows my mind. Thanks, Slava.
© 2016 - 2026 Red Hat, Inc.