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

[Davy Felipe]

Hi Slava,
Thanks for the review.
Your suggested changes look reasonable to me. Please feel free to make
these adjustments to the patch if you prefer.
I am fine with renaming the helper and using size_t for expected_len.
Regarding the error code, I am also fine with using -ERANGE if you think
it better represents this condition.
Thanks,
Davy Felipe

Em 24/09/2026 16:23, Viacheslav Dubeyko escreveu:
> On Wed, 2026-09-23 at 21:16 -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 record size and use a reusable B-tree node
>> range helper to reject invalid write parameters before calling
>> hfs_bnode_write(). This prevents an invalid update from being treated
>> as successful and avoids clearing HFS_FLG_EXT_DIRTY in that case.
>>
>> 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>
>>
>> Sorry, I missed your suggestion about factoring the validation into a
>> reusable helper in v3. This revision addresses it.
>>
>> Changes in v4:
>> - Factor B-tree node range validation into a reusable helper, as
>>   suggested by Viacheslav Dubeyko.
>> - Keep the extent-record size check local to the extent write path.
>> - Preserve the error propagation and validation behavior from v3.
>>
>> ---
>>  fs/hfs/btree.h  |  7 +++++++
>>  fs/hfs/extent.c | 12 ++++++++++--
>>  2 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/hfs/btree.h b/fs/hfs/btree.h
>> index b4c3f2a31471..576412e6901e 100644
>> --- a/fs/hfs/btree.h
>> +++ b/fs/hfs/btree.h
>> @@ -84,6 +84,13 @@ struct hfs_find_data {
>>  	int entryoffset, entrylength;
>>  };
>>  
>> +static inline bool hfs_bnode_is_valid_range(struct hfs_bnode *node,
>> +					    int off, int len)
> 
> You can use struct hfs_find_data *fd. It can make argument list
> shorter. We need to find the node before read/write. So, I think we
> should have hfs_find_data available.
> 
>> +{
>> +	return off >= 0 && len > 0 &&
>> +	       (u64)off + len <= node->tree->node_size;
> 
> Maybe, it makes sense to check that node->tree pointers are valid?
> 
>> +}
>> +
>>  
>>  /* 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..ece782205b47 100644
>> --- a/fs/hfs/extent.c
>> +++ b/fs/hfs/extent.c
>> @@ -121,12 +121,20 @@ 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 (fd->entrylength != sizeof(hfs_extent_rec) ||
>> +		    !hfs_bnode_is_valid_range(fd->bnode, fd-
>>> entryoffset,
>> +					      fd->entrylength))
> 
> I think we can share with hfs_bnode_is_valid_range() expected size
> sizeof(hfs_extent_rec) and hfs_bnode_is_valid_range() will be able to
> check the fd->entrylength. What do you think?
> 
> Thanks,
> Slava.
> 
>> +			return -EIO;
>> +		hfs_bnode_write(fd->bnode, HFS_I(inode)-
>>> cached_extents,
>> +				fd->entryoffset, fd->entrylength);
>>  		HFS_I(inode)->flags &= ~HFS_FLG_EXT_DIRTY;
>>  	}
>>  	return 0;