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

[Viacheslav Dubeyko]

On 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>
> Co-Authored-By: Claude Sonnet 5 <noreply@xxxxxxxxxxxxx>
> ---
>  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)