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

[Nguyen Ngoc Thang]

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

> 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.

Both hunks above build cleanly here.

Thanks,
Thang