[PATCH] hfs: propagate extent B-tree insertion errors

[PATCH] hfs: propagate extent B-tree insertion errors

[Davy Felipe]

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 the error returned by hfs_brec_insert() and only clear the
extent flags after a successful insertion.

Fault injection of -EIO into the extent B-tree insertion path confirmed
that __hfs_ext_write_extent() returned success despite the insertion
failure. With the error propagated, the failure is returned to the
caller instead.

Signed-off-by: Davy Felipe <davyfelipe34@gmail.com>
---
 fs/hfs/extent.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
index f066a99a863b..77ba0f63fcb9 100644
--- a/fs/hfs/extent.c
+++ b/fs/hfs/extent.c
@@ -121,7 +121,10 @@ 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)
-- 
2.55.0

Re: [PATCH] hfs: propagate extent B-tree insertion errors

[Viacheslav Dubeyko]

On Sun, 2026-09-20 at 13:02 -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 the error returned by hfs_brec_insert() and only clear the
> extent flags after a successful insertion.
> 
> Fault injection of -EIO into the extent B-tree insertion path
> confirmed
> that __hfs_ext_write_extent() returned success despite the insertion
> failure. With the error propagated, the failure is returned to the
> caller instead.

How HFS driver behaves when we return error from hfs_brec_insert()? Are
we capable to properly process it? I assume that everything should work
well. But have you tested failure use-case? 

> 
> Signed-off-by: Davy Felipe <davyfelipe34@gmail.com>
> ---
>  fs/hfs/extent.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
> index f066a99a863b..77ba0f63fcb9 100644
> --- a/fs/hfs/extent.c
> +++ b/fs/hfs/extent.c
> @@ -121,7 +121,10 @@ 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)

We ignore potential errors from hfs_bnode_write(). Should we start
processing it too?

	} else {
		if (res)
			return res;
		hfs_bnode_write(fd->bnode, HFS_I(inode)-
>cached_extents, fd->entryoffset, fd->entrylength);
		HFS_I(inode)->flags &= ~HFS_FLG_EXT_DIRTY;
	}

Thanks,
Slava.

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

[Davy Felipe]

__hfs_ext_write_extent() does not report all failures while updating
the extents B-tree.

When inserting a new extent record, the return value of
hfs_brec_insert() is ignored and HFS_FLG_EXT_DIRTY and HFS_FLG_EXT_NEW
are cleared even if the insertion fails.

When updating an existing extent record, hfs_bnode_write() returns
void, so its caller cannot detect a rejected write. Validate the extent
record size and node range before calling hfs_bnode_write().

Propagate errors returned by hfs_brec_insert() and return -EIO for an
invalid existing extent record. Only clear the extent dirty flags after
a successful operation.

Fault injection confirmed both failure paths. Insertion errors are
propagated to the caller, and invalid existing-record writes are
rejected before hfs_bnode_write() without clearing the dirty state.

Signed-off-by: Davy Felipe <davyfelipe34@gmail.com>

Changes in v2:
- Validate the existing extent record size and node range before
  calling hfs_bnode_write(), following review feedback.
- Return -EIO without clearing HFS_FLG_EXT_DIRTY when validation
  fails.
- Fault-injection tested the existing-record failure path. Before the
  change, hfs_bnode_write() rejected an invalid offset internally but
  __hfs_ext_write_extent() continued and cleared the dirty flag. With
  v2, the invalid write is rejected before hfs_bnode_write().

---
 fs/hfs/extent.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
index f066a99a863b..13426503fbb3 100644
--- a/fs/hfs/extent.c
+++ b/fs/hfs/extent.c
@@ -121,12 +121,21 @@ 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) ||
+		    fd->entryoffset < 0 ||
+		    (u64)fd->entryoffset + fd->entrylength >
+			    fd->tree->node_size)
+			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;
-- 
2.55.0

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

[Viacheslav Dubeyko]

On Tue, 2026-09-22 at 20:40 -0300, Davy Felipe wrote:
> __hfs_ext_write_extent() does not report all failures while updating
> the extents B-tree.
> 
> When inserting a new extent record, the return value of
> hfs_brec_insert() is ignored and HFS_FLG_EXT_DIRTY and
> HFS_FLG_EXT_NEW
> are cleared even if the insertion fails.
> 
> When updating an existing extent record, hfs_bnode_write() returns
> void, so its caller cannot detect a rejected write. Validate the
> extent
> record size and node range before calling hfs_bnode_write().
> 
> Propagate errors returned by hfs_brec_insert() and return -EIO for an
> invalid existing extent record. Only clear the extent dirty flags
> after
> a successful operation.
> 
> Fault injection confirmed both failure paths. Insertion errors are
> propagated to the caller, and invalid existing-record writes are
> rejected before hfs_bnode_write() without clearing the dirty state.
> 
> Signed-off-by: Davy Felipe <davyfelipe34@gmail.com>
> 
> Changes in v2:
> - Validate the existing extent record size and node range before
>   calling hfs_bnode_write(), following review feedback.
> - Return -EIO without clearing HFS_FLG_EXT_DIRTY when validation
>   fails.
> - Fault-injection tested the existing-record failure path. Before the
>   change, hfs_bnode_write() rejected an invalid offset internally but
>   __hfs_ext_write_extent() continued and cleared the dirty flag. With
>   v2, the invalid write is rejected before hfs_bnode_write().
> 
> ---
>  fs/hfs/extent.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
> index f066a99a863b..13426503fbb3 100644
> --- a/fs/hfs/extent.c
> +++ b/fs/hfs/extent.c
> @@ -121,12 +121,21 @@ 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) ||
> +		    fd->entryoffset < 0 ||
> +		    (u64)fd->entryoffset + fd->entrylength >
> +			    fd->tree->node_size)

I think it will be better to introduce a small check function that can
be reused then. And code will be cleaner here. What do you think?

> +			return -EIO;
> +		hfs_bnode_write(fd->bnode, HFS_I(inode)-
> >cached_extents,
> +				fd->entryoffset, fd->entrylength);

I see that you are trying not to go into huge modification. But,
frankly speaking, I believe we need the refactoring of
hfs_bnode_write() calling. This function should return error code and
we need to process this error code in other methods. Maybe, future
refactoring work for you? ;)

Thanks,
Slava.

>  		HFS_I(inode)->flags &= ~HFS_FLG_EXT_DIRTY;
>  	}
>  	return 0;

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

[Davy Felipe]

[-- Attachment #1: Type: text/plain, Size: 3926 bytes --]

Hi Slava,

Thanks for the feedback.

Yes, I agree. A small reusable check helper would make the code cleaner
and avoid duplicating the range validation.

Regarding hfs_bnode_write(), I also agree that its current void
interface makes proper error handling difficult. Converting it to
return an error code and auditing its callers looks like the right
direction for a follow-up refactoring.

For this patch, I will keep the change small, introduce the reusable
check helper, and send a v3.

I would be happy to work on the hfs_bnode_write() refactoring as a
follow-up as well.

Thanks,
Davy Felipe

On Wed, 23 Sep 2026, Viacheslav Dubeyko wrote:

> On Tue, 2026-09-22 at 20:40 -0300, Davy Felipe wrote:
>> __hfs_ext_write_extent() does not report all failures while updating
>> the extents B-tree.
>>
>> When inserting a new extent record, the return value of
>> hfs_brec_insert() is ignored and HFS_FLG_EXT_DIRTY and
>> HFS_FLG_EXT_NEW
>> are cleared even if the insertion fails.
>>
>> When updating an existing extent record, hfs_bnode_write() returns
>> void, so its caller cannot detect a rejected write. Validate the
>> extent
>> record size and node range before calling hfs_bnode_write().
>>
>> Propagate errors returned by hfs_brec_insert() and return -EIO for an
>> invalid existing extent record. Only clear the extent dirty flags
>> after
>> a successful operation.
>>
>> Fault injection confirmed both failure paths. Insertion errors are
>> propagated to the caller, and invalid existing-record writes are
>> rejected before hfs_bnode_write() without clearing the dirty state.
>>
>> Signed-off-by: Davy Felipe <davyfelipe34@gmail.com>
>>
>> Changes in v2:
>> - Validate the existing extent record size and node range before
>>   calling hfs_bnode_write(), following review feedback.
>> - Return -EIO without clearing HFS_FLG_EXT_DIRTY when validation
>>   fails.
>> - Fault-injection tested the existing-record failure path. Before the
>>   change, hfs_bnode_write() rejected an invalid offset internally but
>>   __hfs_ext_write_extent() continued and cleared the dirty flag. With
>>   v2, the invalid write is rejected before hfs_bnode_write().
>>
>> ---
>>  fs/hfs/extent.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
>> index f066a99a863b..13426503fbb3 100644
>> --- a/fs/hfs/extent.c
>> +++ b/fs/hfs/extent.c
>> @@ -121,12 +121,21 @@ 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) ||
>> +		    fd->entryoffset < 0 ||
>> +		    (u64)fd->entryoffset + fd->entrylength >
>> +			    fd->tree->node_size)
>
> I think it will be better to introduce a small check function that can
> be reused then. And code will be cleaner here. What do you think?
>
>> +			return -EIO;
>> +		hfs_bnode_write(fd->bnode, HFS_I(inode)-
>>> cached_extents,
>> +				fd->entryoffset, fd->entrylength);
>
> I see that you are trying not to go into huge modification. But,
> frankly speaking, I believe we need the refactoring of
> hfs_bnode_write() calling. This function should return error code and
> we need to process this error code in other methods. Maybe, future
> refactoring work for you? ;)
>
> Thanks,
> Slava.
>
>>  		HFS_I(inode)->flags &= ~HFS_FLG_EXT_DIRTY;
>>  	}
>>  	return 0;
>

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

[Davy Felipe]

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. The helper may reject an invalid offset or shorten an overlong
write without returning status to its caller. Validate the extent
record length and write range before calling hfs_bnode_write() so an
invalid update is reported as -EIO instead of being treated as
successful.

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@gmail.com>

Changes in v3:
- Preserve the original insertion-error propagation.
- Validate the existing extent record before hfs_bnode_write().
- Return -EIO for invalid extent write parameters instead of clearing
  HFS_FLG_EXT_DIRTY after a rejected write.
- Clarify that hfs_bnode_write() returns void and therefore has no
  error value for this caller to propagate.
- Keep test-only instrumentation outside the submitted patch.

---
 fs/hfs/extent.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
index f066a99a863b..13426503fbb3 100644
--- a/fs/hfs/extent.c
+++ b/fs/hfs/extent.c
@@ -121,12 +121,21 @@ 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) ||
+		    fd->entryoffset < 0 ||
+		    (u64)fd->entryoffset + fd->entrylength >
+			    fd->tree->node_size)
+			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;
-- 
2.55.0

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

[Davy Felipe]

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@gmail.com>

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)
+{
+	return off >= 0 && len > 0 &&
+	       (u64)off + len <= 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..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))
+			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;
-- 
2.55.0

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

[Viacheslav Dubeyko]

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@gmail.com>
> 
> 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;

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

[Davy Felipe]

Thanks, that makes sense. Sorry, I missed that point in the previous
revision.

Using struct hfs_find_data *fd will make the helper interface shorter
and also allow the validation to stay in one place.

I’ll update the helper to validate the bnode/tree pointers, the entry
offset and length, and compare fd->entrylength against the expected size
passed by the caller. Then the extent path can simply call it with
sizeof(hfs_extent_rec).

I’ll send an updated revision.

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@gmail.com>
>>
>> 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;

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

[Davy Felipe]

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@gmail.com>
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;
 };
 
+static inline bool hfs_bnode_is_valid_range(struct hfs_find_data *fd, int expected_len)
+{
+	struct hfs_bnode *node;
+
+	if (!fd)
+		return false;
+
+	node = fd->bnode;
+	if (!node || !node->tree)
+		return false;
+
+	if (expected_len <= 0 || fd->entryoffset < 0 ||
+	    fd->entrylength != expected_len)
+		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;
+		hfs_bnode_write(fd->bnode, HFS_I(inode)->cached_extents,
+				fd->entryoffset, fd->entrylength);
 		HFS_I(inode)->flags &= ~HFS_FLG_EXT_DIRTY;
 	}
 	return 0;
-- 
2.55.0