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

[Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>]

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