Re: [PATCH v6] hfsplus: validate b-tree fork extents at mount and inode read

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Fri, 2026-09-18 at 15:51 +0700, Nguyen Ngoc Thang wrote:
> Validate fork extents during inode reading and mount time to catch
> on-disk corruptions early, returning appropriate errors and marking
> the tree as corrupted.
> 
> v6:
>  - Move fork validation logic into inode read fork function.
>  - Refactor extent validation helpers, use volume_blocks, count == 0,
>    and introduce HFSPLUS_EXTENT_LAST_IDX named constant.
>  - Return error from hfsplus_inode_read_fork() to allow
> hfsplus_iget()
>    to catch on-disk corruption and propagate error correctly.

This patch looks like the small portion of the whole fix. As far as I
can see, this patch has lost necessary portion of fixes that we had
before.

> 
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
> ---
>  fs/hfsplus/extents.c | 41 ++++++++++++++++++++++++++++
>  fs/hfsplus/inode.c   | 65 ++++++++++++++++++++++++++----------------
> --
>  2 files changed, 79 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
> index eb7c11524d18..eaf3bed7ede8 100644
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -16,6 +16,47 @@
>  #include "hfsplus_fs.h"
>  #include "hfsplus_raw.h"
>  
> +/* Index of the last extent in the fork */
> +#define HFSPLUS_EXTENT_LAST_IDX 7

I think we need to place this declaration into hfsplus_fs.h.

> +
> +static inline bool is_extents_btree(struct inode *inode)
> +{
> +    return inode->i_ino == HFSPLUS_EXT_CNID;
> +}
> +
> +static bool hfsplus_extent_valid(struct hfsplus_extent *ext, u32
> volume_blocks)
> +{
> +    u32 start = be32_to_cpu(ext->start_block);
> +    u32 count = be32_to_cpu(ext->block_count);
> +
> +    if (count == 0)
> +	return start == 0;
> +
> +    return start + count <= volume_blocks;
> +}
> +
> +/*
> + * Returns 0 if fork extents are consistent, -EUCLEAN if extents
> + * past the first are corrupt, or -EIO if the first extent is
> corrupt.
> + */
> +int hfsplus_check_fork(struct super_block *sb, struct hfsplus_extent
> *ext, u32 volume_blocks)
> +{
> +    bool non_zero_seen = false;
> +    int i;
> +
> +    for (i = 0; i <= HFSPLUS_EXTENT_LAST_IDX; i++, ext++) {
> +	u32 count = be32_to_cpu(ext->block_count);
> +
> +	if (!hfsplus_extent_valid(ext, volume_blocks) ||
> (non_zero_seen && count == 0))
> +	    return i ? -EUCLEAN : -EIO;
> +
> +	if (count > 0)
> +	    non_zero_seen = true;
> +    }
> +
> +    return 0;
> +}

This logic doesn't look like the fork check.

struct hfsplus_fork_raw {
	__be64 total_size;
	__be32 clump_size;
	__be32 total_blocks;
	hfsplus_extent_rec extents;
} __packed;

We need to be sure that total_size, total_blocks are consistent with
the extents state. Also, we can check the clump_size that it is
reasonable one.

> +
>  /* Compare two extents keys, returns 0 on same, pos/neg for
> difference */
>  int hfsplus_ext_cmp_key(const hfsplus_btree_key *k1,
>  			const hfsplus_btree_key *k2)
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index 2ce6de574fa6..aa201f4e80d5 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -559,34 +559,45 @@ void hfsplus_delete_inode(struct inode *inode)
>  	hfsplus_mark_mdb_dirty(sb);
>  }
>  
> -void hfsplus_inode_read_fork(struct inode *inode, struct
> hfsplus_fork_raw *fork)
> +int hfsplus_inode_read_fork(struct inode *inode, struct
> hfsplus_fork_raw *fork)

This patch hasn't any logic of checking the error code of
hfsplus_inode_read_fork().

>  {
> -	struct super_block *sb = inode->i_sb;
> -	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> -	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
> -	u32 count;
> -	int i;
> -
> -	memcpy(&hip->first_extents, &fork->extents,
> sizeof(hfsplus_extent_rec));
> -	for (count = 0, i = 0; i < 8; i++)
> -		count += be32_to_cpu(fork->extents[i].block_count);
> -	hip->first_blocks = count;
> -	memset(hip->cached_extents, 0, sizeof(hfsplus_extent_rec));
> -	hip->cached_start = 0;
> -	hip->cached_blocks = 0;
> -
> -	hip->alloc_blocks = be32_to_cpu(fork->total_blocks);
> -	hip->phys_size = inode->i_size = be64_to_cpu(fork-
> >total_size);
> -	hip->fs_blocks =
> -		(inode->i_size + sb->s_blocksize - 1) >> sb-
> >s_blocksize_bits;
> -	inode_set_bytes(inode, hip->fs_blocks << sb-
> >s_blocksize_bits);
> -	hip->clump_blocks =
> -		be32_to_cpu(fork->clump_size) >> sbi-
> >alloc_blksz_shift;
> -	if (!hip->clump_blocks) {
> -		hip->clump_blocks = HFSPLUS_IS_RSRC(inode) ?
> -			sbi->rsrc_clump_blocks :
> -			sbi->data_clump_blocks;
> -	}
> +    struct super_block *sb = inode->i_sb;
> +    struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> +    struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
> +    u32 count;
> +    int i, ret;
> +
> +    /* Validate fork extents to catch on-disk corruption early */
> +    ret = hfsplus_check_fork(sb, fork->extents, sbi->total_blocks);

We need to check the whole fork but not only extents.

> +    if (ret) {
> +	pr_err("hfsplus: fork check failed for inode %lu
> (err=%d)\n", inode->i_ino, ret);
> +	set_bit(HFSPLUS_I_CORRUPT_TREE, &hip->flags);
> +	sb->s_flags |= SB_RDONLY;

NO, we cannot set SB_RDONLY in this method. It can be done in
hfsplus_fill_super() or hfsplus_reconfigure().

> +	return ret; /* Return error directly to the caller */
> +    }
> +
> +    memcpy(&hip->first_extents, &fork->extents,
> sizeof(hfsplus_extent_rec));
> +    for (count = 0, i = 0; i <= HFSPLUS_EXTENT_LAST_IDX; i++)
> +	count += be32_to_cpu(fork->extents[i].block_count);

Looks like a mess and improper formatting of the code. What's happen
here?

Thanks,
Slava.

> +    hip->first_blocks = count;
> +    memset(hip->cached_extents, 0, sizeof(hfsplus_extent_rec));
> +    hip->cached_start = 0;
> +    hip->cached_blocks = 0;
> +
> +    hip->alloc_blocks = be32_to_cpu(fork->total_blocks);
> +    hip->phys_size = inode->i_size = be64_to_cpu(fork->total_size);
> +    hip->fs_blocks =
> +	(inode->i_size + sb->s_blocksize - 1) >> sb-
> >s_blocksize_bits;
> +    inode_set_bytes(inode, hip->fs_blocks << sb->s_blocksize_bits);
> +    hip->clump_blocks =
> +	be32_to_cpu(fork->clump_size) >> sbi->alloc_blksz_shift;
> +    if (!hip->clump_blocks) {
> +	hip->clump_blocks = HFSPLUS_IS_RSRC(inode) ?
> +	    sbi->rsrc_clump_blocks :
> +	    sbi->data_clump_blocks;
> +    }
> +
> +    return 0;
>  }
>  
>  void hfsplus_inode_write_fork(struct inode *inode,