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

[Davy Felipe]

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@xxxxxxxxx>
> >
> > 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;