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

[Viacheslav Dubeyko <slava@dubeyko.com>]

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