Re: [PATCH v5] hfs: handle extent B-tree write errors

[Viacheslav Dubeyko]

On Thu, 2026-09-24 at 20:10 -0300, Davy Felipe wrote:
> hfs_brec_insert() may fail while inserting a new extent record, but
> __hfs_ext_write_extent() currently ignores its return value and
> clears
> HFS_FLG_EXT_DIRTY and HFS_FLG_EXT_NEW as if the insertion had
> succeeded.
> 
> Propagate errors returned by hfs_brec_insert() and only clear the
> extent flags after a successful insertion.
> 
> When updating an existing extent record, hfs_bnode_write() returns
> void. Validate the extent write parameters before calling it so an
> invalid update is reported as -EIO instead of being treated as
> successful.
> 
> Use a reusable B-tree node range helper that takes the find data and
> the expected record size. The helper validates the bnode and tree
> pointers, entry offset and entry length, and ensures that the write
> range fits within the node.
> 
> Negative-path testing in QEMU confirmed that an insertion error is
> propagated to the caller. Testing the existing-record path also
> confirmed that invalid write parameters are rejected before
> HFS_FLG_EXT_DIRTY is cleared.
> 
> Signed-off-by: Davy Felipe <davyfelipe34@xxxxxxxxx>
___

> Thanks for the feedback. I updated the helper to take struct
> hfs_find_data and the expected entry size so the bnode/tree,
> entry offset and entry length validation stay in one place.
> 
> Changes in v5:
> - Pass struct hfs_find_data to hfs_bnode_is_valid_range().
> - Validate the bnode and tree pointers in the helper.
> - Pass the expected entry size and validate fd->entrylength there.
> - Simplify the extent write path to a single validation helper call.
> 
> ---
>  fs/hfs/btree.h  | 19 +++++++++++++++++++
>  fs/hfs/extent.c | 10 ++++++++--
>  2 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfs/btree.h b/fs/hfs/btree.h
> index b4c3f2a31471..ab7a7c65a126 100644
> --- a/fs/hfs/btree.h
> +++ b/fs/hfs/btree.h
> @@ -84,6 +84,25 @@ struct hfs_find_data {
>  	int entryoffset, entrylength;
>  };

Mostly, I like the patch. But I have some cosmetic remarks for
discussion because I don't want to change the patch without your
consent. 

>  
> +static inline bool hfs_bnode_is_valid_range(struct hfs_find_data

Current name sounds slightly not natural. :) We can consider
is_hfs_bnode_range_valid(). What do you think? Maybe you have something
better in mind?

> *fd, int expected_len)

Do we really need to use integer for expected_len? Why not size_t? Any
particular reason?

> +{
> +	struct hfs_bnode *node;
> +
> +	if (!fd)
> +		return false;
> +
> +	node = fd->bnode;
> +	if (!node || !node->tree)
> +		return false;
> +
> +	if (expected_len <= 0 || fd->entryoffset < 0 ||

If we have size_t, then we don't need to check that expected_len < 0.

> +	    fd->entrylength != expected_len)

Do we need to check that expected_len == 0? Should fd->entrylength !=
expected_len be enough?

> +		return false;
> +
> +	return (u64)fd->entryoffset + fd->entrylength <=
> +	       node->tree->node_size;
> +}
> +
>  
>  /* btree.c */
>  extern struct hfs_btree *hfs_btree_open(struct super_block *sb, u32
> id,
> diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
> index f066a99a863b..4d65943d1117 100644
> --- a/fs/hfs/extent.c
> +++ b/fs/hfs/extent.c
> @@ -121,12 +121,18 @@ static int __hfs_ext_write_extent(struct inode
> *inode, struct hfs_find_data *fd)
>  		res = hfs_bmap_reserve(fd->tree, fd->tree->depth +
> 1);
>  		if (res)
>  			return res;
> -		hfs_brec_insert(fd, HFS_I(inode)->cached_extents,
> sizeof(hfs_extent_rec));
> +		res = hfs_brec_insert(fd, HFS_I(inode)-
> >cached_extents,
> +				      sizeof(hfs_extent_rec));
> +		if (res)
> +			return res;
>  		HFS_I(inode)->flags &=
> ~(HFS_FLG_EXT_DIRTY|HFS_FLG_EXT_NEW);
>  	} else {
>  		if (res)
>  			return res;
> -		hfs_bnode_write(fd->bnode, HFS_I(inode)-
> >cached_extents, fd->entryoffset, fd->entrylength);
> +		if (!hfs_bnode_is_valid_range(fd,
> sizeof(hfs_extent_rec)))
> +			return -EIO;

I am started to guess. Is -EIO correct error code here? It is still
operation in memory and we are checking the correctness of struct
hfs_find_data content. Maybe, we need to consider another error code
here? What's about -ERANGE? What is your preference?

Thanks,
Slava.

> +		hfs_bnode_write(fd->bnode, HFS_I(inode)-
> >cached_extents,
> +				fd->entryoffset, fd->entrylength);
>  		HFS_I(inode)->flags &= ~HFS_FLG_EXT_DIRTY;
>  	}
>  	return 0;