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

[Viacheslav Dubeyko]

On Tue, 2026-09-08 at 18:54 +0700, ThangNN99 wrote:
> Hi Slava,
> 
> > Could you share the dump/content of the Extents Overflow file's
> > fork?
> 
> Decoded the volume header from the syzbot image. The Extents Overflow
> fork (offset 192 in the header):
> 
>   logicalSize=32768 clumpSize=32768 totalBlocks=32
>   extents[0]=(start=3,   count=32)
>   extents[1]=(start=0,   count=0)
>   extents[2]=(start=0,   count=0)
>   extents[3]=(start=0,   count=134217728)   <- garbage
>   extents[4]=(start=0,   count=0)
>   extents[5]=(start=0,   count=0)
>   extents[6]=(start=0,   count=11796736)    <- garbage
>   extents[7]=(start=0,   count=0)
> 
> Correction to my last mail: it's not totalBlocks exceeding the fork's
> extents, it's the reverse and messier. hfsplus_inode_read_fork() sums
> all 8 extents' block_count into hip->first_blocks with no validation
> (inode.c:569). Slots 3 and 6 have start_block=0 (i.e. "unused") but
> garbage non-zero block_count, so first_blocks comes out to 146014496
> against a real totalBlocks (hip->alloc_blocks) of 32. Either
> direction
> of that mismatch takes hfsplus_file_extend() down the same
> hfsplus_ext_read_extent() path, since the code only tests
> alloc_blocks == first_blocks.

I think we need to have more precise fork check. Because, corruption
could be more severe. The start block of extents could be out of
volume. The total number of block could be not consistent with
calculated one and be bigger than volume itself. Also, we need to check
the fork for all types of btrees (Catalog, Extents, Extended
attributes).

> 
> > Could we detect the corruption of the fork during the mount phase?
> > If we can then we need to mount in Read-Only mode the corrupted
> > volume.
> 
> Yes. Proposed v2, forcing read-only instead of touching extents.c:
> 
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -293,6 +293,14 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id)
>  		goto free_inode;
>  	}
> 
> +	/* Per TN1150, the extents file can't have overflow extents
> of its own. */
> +	if (id == HFSPLUS_EXT_CNID &&
> +	    HFSPLUS_I(tree->inode)->first_blocks !=
> +	    HFSPLUS_I(tree->inode)->alloc_blocks) {
> +		pr_warn("extents overflow file has overflow extents
> of its own, forcing read-only.\n");
> +		sb->s_flags |= SB_RDONLY;
> +	}
> +

The hfs_btree_open() could be called not only during mount or re-mount.
So, I think that it needs to return the error code from this method.
Finally, caller could make decision if it is a proper place to set sb-
>s_flags |= SB_RDONLY. And, again, we need to check not only Extents
Overflow file.

>  	mapping = tree->inode->i_mapping;
>  	page = read_mapping_page(mapping, 0, NULL);
>  	if (IS_ERR(page))
> 
> One catch: the reproducer mounts MS_RDONLY, then remounts rw via a
> bare MS_REMOUNT|MS_MOVE. hfsplus_reconfigure() only re-checks
> VOL_UNMNT/SOFTLOCK/JOURNALED before allowing that, and this image
> sets
> VOL_UNMNT, so a read-only-only fix in hfs_btree_open() gets undone by
> that remount. Same check needs to go in hfsplus_reconfigure() too:
> 
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -400,6 +400,12 @@ static int hfsplus_reconfigure(struct fs_context
> *fc)
>  			pr_warn("filesystem is marked journaled,
> leaving read-only.\n");
>  			sb->s_flags |= SB_RDONLY;
>  			fc->sb_flags |= SB_RDONLY;
> +		} else if (HFSPLUS_I(sbi->ext_tree->inode)-
> >first_blocks !=
> +				HFSPLUS_I(sbi->ext_tree->inode)-
> >alloc_blocks) {
> +			/* Per TN1150, the extents file can't have
> overflow extents of its own. */
> +			pr_warn("extents overflow file has overflow
> extents of its own, leaving read-only.\n");
> +			sb->s_flags |= SB_RDONLY;
> +			fc->sb_flags |= SB_RDONLY;

Frankly speaking, I would like to introduce some method(s) that can
check the btree consistency.

Do we really need to repeat the check on remount? If we detected that
btree is corrupted during mount, then we should set the state of the
btree as inconsistent and simply check such flag(s). Do we have
something like this now? If it is not, then we can simply introduce
one.

>  		}
>  	}
>  	return 0;
> 
> Both hunks build cleanly here. Want me to send this as v2 replacing
> the extents.c hunk, or keep the extents.c guard too as a second line
> of defense (it's independent of mount-time state and free)?

The checking inconsistency is one direction. Another direction is that
we exhausted the volume or volume is so fragmented that we cannot
extend the Extents Overflow file anymore. I think we need to check
before extending the Extents Overflow file 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. Because, we cannot
add any new data on the volume. The main question here how to add or
modify the logic in safe way.

Thanks,
Slava.