[PATCH v2] hfs: report errors when extending files with setattr

[PATCH v2] hfs: report errors when extending files with setattr

[Hengyu Liang]

hfs_inode_setattr() sets i_size and calls hfs_file_truncate() to change
the allocation. If expansion fails, hfs_file_truncate() restores i_size
to the allocated size, but its void return type leaves setattr reporting
success.

On a freshly formatted 64 MiB HFS volume, ftruncate() to 128 MiB returns
zero while the file remains empty. When the requested size fits within
the volume but exceeds the free space, it also returns zero, leaving
the file only partially extended.

Call generic_cont_expand_simple() when growing a file and propagate its
error. Keep truncate_setsize() and hfs_file_truncate() in the shrinking
path. This makes failed extensions report EFBIG or ENOSPC.

Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
---
Changes in v2:
- Keep truncate_setsize() and hfs_file_truncate() in the shrinking path,
  as suggested by Viacheslav Dubeyko.
- Replace the generic/736 and fsx runs with a comparison of the whole
  xfstests set selected for HFS, as requested.

v1: https://lore.kernel.org/r/20261017160612.3740256-1-hengyul@cs.unc.edu

Compared unpatched and patched v7.3-rc7 with identical KASAN configs.
All 805 tests selected for HFS by "check -n -g all" were attempted,
using freshly formatted 2 GiB test and scratch volumes for each case:
                passed  failed  not run
  unpatched       109      27      669
  v2              109      27      669

The different results were generic/113, /394, /464 and /795. /113 and
/464 passed and failed in replays on the unpatched kernel, and the /113
failures are the same existing workqueue warning from hfs_file_fsync().
/795 failed on all three kernels in replays. On the 2 GiB volume, /394
passes without the fix only because ftruncate() returns success after a
partial extension; v1 and v2 report ENOSPC. It passes on all three
kernels with a 4 GiB volume.

Also ran 39 functional and 18 recovery cases per kernel on 32/64/128 MiB
volumes. Content and read-only remount checks passed on unpatched, v1
and v2 kernels. All three had the same 12 post-ENOSPC fsck failures;
recovery fsck checks were clean.

Functional/recovery cases and generic/394 were repeated on unpatched
and v2 kernels at the hfs 7.4 merge (dd7b15aeac80), with the same results.

 fs/hfs/inode.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 2aef3c36a150..9bdaa60fc392 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -684,8 +684,14 @@ int hfs_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 		if (error)
 			return error;
 
-		truncate_setsize(inode, attr->ia_size);
-		hfs_file_truncate(inode);
+		if (attr->ia_size > inode->i_size) {
+			error = generic_cont_expand_simple(inode, attr->ia_size);
+			if (error)
+				return error;
+		} else {
+			truncate_setsize(inode, attr->ia_size);
+			hfs_file_truncate(inode);
+		}
 		inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
 	}
 
-- 
2.53.0

Re: [PATCH v2] hfs: report errors when extending files with setattr

[Viacheslav Dubeyko]

On Wed, 2026-10-21 at 10:56 -0400, Hengyu Liang wrote:
> hfs_inode_setattr() sets i_size and calls hfs_file_truncate() to
> change
> the allocation. If expansion fails, hfs_file_truncate() restores
> i_size
> to the allocated size, but its void return type leaves setattr
> reporting
> success.
> 
> On a freshly formatted 64 MiB HFS volume, ftruncate() to 128 MiB
> returns
> zero while the file remains empty. When the requested size fits
> within
> the volume but exceeds the free space, it also returns zero, leaving
> the file only partially extended.
> 
> Call generic_cont_expand_simple() when growing a file and propagate
> its
> error. Keep truncate_setsize() and hfs_file_truncate() in the
> shrinking
> path. This makes failed extensions report EFBIG or ENOSPC.

I think now this code [1] becomes dead. And we need to rework it:

void hfs_file_truncate(struct inode *inode)
{
        ...
        hfs_dbg(...);

        if (WARN_ON_ONCE(inode->i_size > HFS_I(inode)->phys_size))
                return;
        if (inode->i_size == HFS_I(inode)->phys_size)
                return;

        size = inode->i_size + HFS_SB(sb)->alloc_blksz - 1;
        ...     /* existing shrink logic unchanged */
}

It will be great if you could share another patch with the fix.

> 
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
> ---
> Changes in v2:
> - Keep truncate_setsize() and hfs_file_truncate() in the shrinking
> path,
>   as suggested by Viacheslav Dubeyko.
> - Replace the generic/736 and fsx runs with a comparison of the whole
>   xfstests set selected for HFS, as requested.
> 
> v1:
> https://lore.kernel.org/r/20261017160612.3740256-1-hengyul@cs.unc.edu
> 
> Compared unpatched and patched v7.3-rc7 with identical KASAN configs.
> All 805 tests selected for HFS by "check -n -g all" were attempted,
> using freshly formatted 2 GiB test and scratch volumes for each case:
>                 passed  failed  not run
>   unpatched       109      27      669
>   v2              109      27      669
> 
> The different results were generic/113, /394, /464 and /795. /113 and
> /464 passed and failed in replays on the unpatched kernel, and the
> /113
> failures are the same existing workqueue warning from
> hfs_file_fsync().
> /795 failed on all three kernels in replays. On the 2 GiB volume,
> /394
> passes without the fix only because ftruncate() returns success after
> a
> partial extension; v1 and v2 report ENOSPC. It passes on all three
> kernels with a 4 GiB volume.
> 
> Also ran 39 functional and 18 recovery cases per kernel on 32/64/128
> MiB
> volumes. Content and read-only remount checks passed on unpatched, v1
> and v2 kernels. All three had the same 12 post-ENOSPC fsck failures;
> recovery fsck checks were clean.
> 
> Functional/recovery cases and generic/394 were repeated on unpatched
> and v2 kernels at the hfs 7.4 merge (dd7b15aeac80), with the same
> results.
> 
>  fs/hfs/inode.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 2aef3c36a150..9bdaa60fc392 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -684,8 +684,14 @@ int hfs_inode_setattr(struct mnt_idmap *idmap,
> struct dentry *dentry,
>  		if (error)
>  			return error;
>  
> -		truncate_setsize(inode, attr->ia_size);
> -		hfs_file_truncate(inode);
> +		if (attr->ia_size > inode->i_size) {
> +			error = generic_cont_expand_simple(inode,
> attr->ia_size);
> +			if (error)
> +				return error;
> +		} else {
> +			truncate_setsize(inode, attr->ia_size);
> +			hfs_file_truncate(inode);
> +		}
>  		inode_set_mtime_to_ts(inode,
> inode_set_ctime_current(inode));
>  	}
>  

Looks good.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.

[1]
https://elixir.bootlin.com/linux/v7.3-rc5/source/fs/hfs/extent.c#L488

Re: [PATCH v2] hfs: report errors when extending files with setattr

[Hengyu Liang]

Hi Slava,

On Wed, Oct 21, 2026 at 02:22:41PM -0700, Viacheslav Dubeyko wrote:
> I think now this code [1] becomes dead. And we need to rework it:
>
> void hfs_file_truncate(struct inode *inode)
> {
>         ...
>         hfs_dbg(...);
>
>         if (WARN_ON_ONCE(inode->i_size > HFS_I(inode)->phys_size))
>                 return;
>         if (inode->i_size == HFS_I(inode)->phys_size)
>                 return;
[...]
> It will be great if you could share another patch with the fix.

Thanks for the review. I made this change and ran the whole xfstests
set, but the grow path is still reached. The WARN_ON_ONCE() fires under
fsstress in generic/013, 076, 476, 650, 750 and 795, every time from
close() through hfs_file_release().

It comes from direct writes:

1. When an inode is read, phys_size is set to i_size, which does not
   have to be block aligned, e.g. 100.
2. A direct write does not go through cont_write_begin(). When it
   needs a new block, hfs_get_block() only adds s_blocksize to
   phys_size. A direct write of 512 bytes at offset 512 makes phys_size
   612 and i_size 1024.
3. At the last close, hfs_file_truncate() sees i_size > phys_size and
   calls hfs_write_begin(). cont_write_begin() then zeroes
   [phys_size, i_size) in the page cache, which overwrites the data
   that the direct write has just written.

So today this path loses data, and removing it would only replace that
with the warning, while phys_size stays behind i_size.

The issue can be reproduced with:

    truncate -s 32M hfs.img
    mkfs.hfsplus -h hfs.img
    mount -t hfs -o loop hfs.img /mnt
    printf 'A%.0s' $(seq 100) > /mnt/f
    umount /mnt
    mount -t hfs -o loop hfs.img /mnt
    xfs_io -d -c 'pwrite -S 0x42 512 512' /mnt/f
    umount /mnt
    mount -t hfs -o loop hfs.img /mnt
    od -An -tx1 -j 512 -N 512 /mnt/f

On a804c5742bff, with and without the v2 patch, the last 412 bytes
read back as zeros instead of 0x42, and fsck.hfs reports no error.
With the cleanup applied, the data is intact but the warning fires.

HFS+ has the same problem. hfsplus_file_truncate() has the same grow
path, and commit 5f63ac80aef2 ("hfsplus: fix issue of direct writes
beyond end-of-file") only covers writes that start past EOF. A direct
write of 1024 bytes at offset 0 to the same 100-byte file loses 924
bytes on HFS+.

I think the direct write path should be fixed first, so that i_size
can no longer get past phys_size. Then the grow path really is dead
and your cleanup is right. The simplest way I see is what
fat_direct_IO() does: if a direct write would end past phys_size,
return 0 so that it falls back to a buffered write, which zeroes and
accounts through cont_write_begin().

Would that be fine with you? If so, I will send that fix for HFS with
your cleanup on top, and a similar fix for HFS+.

Thanks,
Hengyu

Re: [PATCH v2] hfs: report errors when extending files with setattr

[Viacheslav Dubeyko]

On Thu, 2026-10-22 at 09:51 -0400, Hengyu Liang wrote:
> Hi Slava,
> 
> On Wed, Oct 21, 2026 at 02:22:41PM -0700, Viacheslav Dubeyko wrote:
> > I think now this code [1] becomes dead. And we need to rework it:
> > 
> > void hfs_file_truncate(struct inode *inode)
> > {
> >         ...
> >         hfs_dbg(...);
> > 
> >         if (WARN_ON_ONCE(inode->i_size > HFS_I(inode)->phys_size))
> >                 return;
> >         if (inode->i_size == HFS_I(inode)->phys_size)
> >                 return;
> [...]
> > It will be great if you could share another patch with the fix.
> 
> Thanks for the review. I made this change and ran the whole xfstests
> set, but the grow path is still reached. The WARN_ON_ONCE() fires
> under
> fsstress in generic/013, 076, 476, 650, 750 and 795, every time from
> close() through hfs_file_release().
> 
> It comes from direct writes:
> 
> 1. When an inode is read, phys_size is set to i_size, which does not
>    have to be block aligned, e.g. 100.
> 2. A direct write does not go through cont_write_begin(). When it
>    needs a new block, hfs_get_block() only adds s_blocksize to
>    phys_size. A direct write of 512 bytes at offset 512 makes
> phys_size
>    612 and i_size 1024.
> 3. At the last close, hfs_file_truncate() sees i_size > phys_size and
>    calls hfs_write_begin(). cont_write_begin() then zeroes
>    [phys_size, i_size) in the page cache, which overwrites the data
>    that the direct write has just written.
> 
> So today this path loses data, and removing it would only replace
> that
> with the warning, while phys_size stays behind i_size.
> 
> The issue can be reproduced with:
> 
>     truncate -s 32M hfs.img
>     mkfs.hfsplus -h hfs.img
>     mount -t hfs -o loop hfs.img /mnt
>     printf 'A%.0s' $(seq 100) > /mnt/f
>     umount /mnt
>     mount -t hfs -o loop hfs.img /mnt
>     xfs_io -d -c 'pwrite -S 0x42 512 512' /mnt/f
>     umount /mnt
>     mount -t hfs -o loop hfs.img /mnt
>     od -An -tx1 -j 512 -N 512 /mnt/f
> 
> On a804c5742bff, with and without the v2 patch, the last 412 bytes
> read back as zeros instead of 0x42, and fsck.hfs reports no error.
> With the cleanup applied, the data is intact but the warning fires.
> 
> HFS+ has the same problem. hfsplus_file_truncate() has the same grow
> path, and commit 5f63ac80aef2 ("hfsplus: fix issue of direct writes
> beyond end-of-file") only covers writes that start past EOF. A direct
> write of 1024 bytes at offset 0 to the same 100-byte file loses 924
> bytes on HFS+.
> 
> I think the direct write path should be fixed first, so that i_size
> can no longer get past phys_size. Then the grow path really is dead
> and your cleanup is right. The simplest way I see is what
> fat_direct_IO() does: if a direct write would end past phys_size,
> return 0 so that it falls back to a buffered write, which zeroes and
> accounts through cont_write_begin().
> 
> Would that be fine with you? If so, I will send that fix for HFS with
> your cleanup on top, and a similar fix for HFS+.
> 
> 

Yes, we still have enough issues in HFS and let's fix it step by step.
You are right, we need to fix the direct IO issue before removing this
"dead" code (it's not completely dead yet).

Related to HFS+... I am still working on rebuilding direct IO path for
HFS+ and, potentially, your fix could conflict with my patchset. But,
anyway, you can share your fix and, then, we can manage this conflict
(if we have one finally).

Thanks,
Slava.