[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 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 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 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