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

[Viacheslav Dubeyko <slava@dubeyko.com>]

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