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

[Nguyen Ngoc Thang]

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);
 		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;
+	}
+
 	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