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

[Viacheslav Dubeyko]

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

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

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

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

Thanks,
Slava. 

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