hfsplus: limit sb_maxbytes to partition size

[PATCH] hfsplus: limit sb_maxbytes to partition size

Posted by Hyunchul Lee 1 month, 1 week ago

s_maxbytes currently is set to MAX_LFS_FILESIZE,
which allows writes beyond the partition size. As a result,
large-offset writes on small partitions can fail late
with ENOSPC.

Set s_maxbytes to the partition size.

With this change, the large-offset writes are rejected
early by `generic_write_check_limit()` with EFBIG.

This patch also fixes generic/268.

Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
---
 fs/hfsplus/super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 7229a8ae89f9..18350abc659b 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -500,7 +500,8 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	/* Set up operations so we can load metadata */
 	sb->s_op = &hfsplus_sops;
-	sb->s_maxbytes = MAX_LFS_FILESIZE;
+	sb->s_maxbytes = (loff_t)min(MAX_LFS_FILESIZE,
+				     (u64)sbi->total_blocks << sbi->alloc_blksz_shift);
 
 	if (!(vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_UNMNT))) {
 		pr_warn("Filesystem was not cleanly unmounted, running fsck.hfsplus is recommended.  mounting read-only.\n");
-- 
2.43.0

Re: [PATCH] hfsplus: limit sb_maxbytes to partition size

Posted by Christoph Hellwig 1 month, 1 week ago

On Tue, Mar 03, 2026 at 05:28:07PM +0900, Hyunchul Lee wrote:
> s_maxbytes currently is set to MAX_LFS_FILESIZE,
> which allows writes beyond the partition size.

The "partition size" does not matter here.  s_maxbytes is the maximum
size supported by the format and has nothing to do with the actual space
allocated to the file system (which in Linux terminology would be the
block device and not the partition anyway).

>
> As a result,
> large-offset writes on small partitions can fail late
> with ENOSPC.

That sounds like some other check is missing in hfsplus, but it
should be about the available free space, not the device size.

Re: [PATCH] hfsplus: limit sb_maxbytes to partition size

Posted by Hyunchul Lee 1 month, 1 week ago

On Wed, Mar 04, 2026 at 05:08:15AM -0800, Christoph Hellwig wrote:
> On Tue, Mar 03, 2026 at 05:28:07PM +0900, Hyunchul Lee wrote:
> > s_maxbytes currently is set to MAX_LFS_FILESIZE,
> > which allows writes beyond the partition size.
> 
> The "partition size" does not matter here.  s_maxbytes is the maximum
> size supported by the format and has nothing to do with the actual space
> allocated to the file system (which in Linux terminology would be the
> block device and not the partition anyway).
> 
> >
> > As a result,
> > large-offset writes on small partitions can fail late
> > with ENOSPC.
> 
> That sounds like some other check is missing in hfsplus, but it
> should be about the available free space, not the device size.
> 

When running xfs_io -c "pwrite 8t 512", hfsplus fills the block device
with zeros before returning ENOSPC. I was trying to fix this,
but as you mentioned, I will look for another solution.

Thank for your review and comments.

-- 
Thanks,
Hyunchul

RE: [PATCH] hfsplus: limit sb_maxbytes to partition size

Posted by Viacheslav Dubeyko 1 month, 1 week ago

On Wed, 2026-03-04 at 05:08 -0800, Christoph Hellwig wrote:
> On Tue, Mar 03, 2026 at 05:28:07PM +0900, Hyunchul Lee wrote:
> > s_maxbytes currently is set to MAX_LFS_FILESIZE,
> > which allows writes beyond the partition size.
> 
> The "partition size" does not matter here.  s_maxbytes is the maximum
> size supported by the format and has nothing to do with the actual space
> allocated to the file system (which in Linux terminology would be the
> block device and not the partition anyway).
> 
> > 
> > As a result,
> > large-offset writes on small partitions can fail late
> > with ENOSPC.
> 
> That sounds like some other check is missing in hfsplus, but it
> should be about the available free space, not the device size.
> 

I agree with Christoph.

But, frankly speaking, I don't quite follow which particular issue is under fix
here. I can see that generic/268 failure has been mentioned. However, I can see
this:

sudo ./check generic/268 
FSTYP         -- hfsplus
PLATFORM      -- Linux/x86_64 hfsplus-testing-0001 6.19.0-rc1+ #95 SMP
PREEMPT_DYNAMIC Thu Feb 19 15:29:55 PST 2026
MKFS_OPTIONS  -- /dev/loop51
MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch

generic/268       [not run] Reflink not supported by scratch filesystem type:
hfsplus
Ran: generic/268
Not run: generic/268
Passed all 1 tests

Which particular issue is under fix?

Thanks,
Slava.

Re: [PATCH] hfsplus: limit sb_maxbytes to partition size

Posted by Hyunchul Lee 1 month, 1 week ago

On Wed, Mar 04, 2026 at 08:04:30PM +0000, Viacheslav Dubeyko wrote:
> On Wed, 2026-03-04 at 05:08 -0800, Christoph Hellwig wrote:
> > On Tue, Mar 03, 2026 at 05:28:07PM +0900, Hyunchul Lee wrote:
> > > s_maxbytes currently is set to MAX_LFS_FILESIZE,
> > > which allows writes beyond the partition size.
> > 
> > The "partition size" does not matter here.  s_maxbytes is the maximum
> > size supported by the format and has nothing to do with the actual space
> > allocated to the file system (which in Linux terminology would be the
> > block device and not the partition anyway).
> > 
> > > 
> > > As a result,
> > > large-offset writes on small partitions can fail late
> > > with ENOSPC.
> > 
> > That sounds like some other check is missing in hfsplus, but it
> > should be about the available free space, not the device size.
> > 
> 
> I agree with Christoph.
> 
> But, frankly speaking, I don't quite follow which particular issue is under fix
> here. I can see that generic/268 failure has been mentioned. However, I can see
> this:
> 
> sudo ./check generic/268 
> FSTYP         -- hfsplus
> PLATFORM      -- Linux/x86_64 hfsplus-testing-0001 6.19.0-rc1+ #95 SMP
> PREEMPT_DYNAMIC Thu Feb 19 15:29:55 PST 2026
> MKFS_OPTIONS  -- /dev/loop51
> MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch
> 
> generic/268       [not run] Reflink not supported by scratch filesystem type:
> hfsplus
> Ran: generic/268
> Not run: generic/268
> Passed all 1 tests
> 
> Which particular issue is under fix?

Sorry it's generic/285, not generic/268.
in generic/285, there is a test that creates a hole exceeding the block
size and appends small data to the file. hfsplus fails because it fills
the block device and returns ENOSPC. However if it returns EFBIG
instead, the test is skipped.

For writes like xfs_io -c "pwrite 8t 512", should fops->write_iter
returns ENOSPC, or would it be better to return EFBIG?

> 
> Thanks,
> Slava.

-- 
Thanks,
Hyunchul

Re: [PATCH] hfsplus: limit sb_maxbytes to partition size

Posted by hch@infradead.org 1 month, 1 week ago

On Thu, Mar 05, 2026 at 09:29:33AM +0900, Hyunchul Lee wrote:
> Sorry it's generic/285, not generic/268.
> in generic/285, there is a test that creates a hole exceeding the block
> size and appends small data to the file. hfsplus fails because it fills
> the block device and returns ENOSPC. However if it returns EFBIG
> instead, the test is skipped.

generic/285 needs to call _require_sparse_files.

Re: [PATCH] hfsplus: limit sb_maxbytes to partition size

Posted by Hyunchul Lee 1 month, 1 week ago

On Thu, Mar 05, 2026 at 06:27:34AM -0800, hch@infradead.org wrote:
> On Thu, Mar 05, 2026 at 09:29:33AM +0900, Hyunchul Lee wrote:
> > Sorry it's generic/285, not generic/268.
> > in generic/285, there is a test that creates a hole exceeding the block
> > size and appends small data to the file. hfsplus fails because it fills
> > the block device and returns ENOSPC. However if it returns EFBIG
> > instead, the test is skipped.
> 
> generic/285 needs to call _require_sparse_files.
> 

The generic/258(src/seek_sanity_test.c) is considering filesystems
that don't support sparse files[1].

int test_basic_support()
  ...
  pos = lseek(fd, 0, SEEK_HOLE);
  ...
  if (pos == filsze) {
    default_behavior = 1;
    fprintf(stderr, "File system supports the default behavior.\n");
  ...

The issue is that there are some tests which write to offsets larger
than the block device. How about skipping for such test cases when
dealing with filesystems that don't support sparse files?

[1]: https://github.com/kdave/xfstests/blob/master/src/seek_sanity_test.c#L1244

-- 
Thanks,
Hyunchul

RE: [PATCH] hfsplus: limit sb_maxbytes to partition size

Posted by Viacheslav Dubeyko 1 month, 1 week ago

On Thu, 2026-03-05 at 09:29 +0900, Hyunchul Lee wrote:
> On Wed, Mar 04, 2026 at 08:04:30PM +0000, Viacheslav Dubeyko wrote:
> > On Wed, 2026-03-04 at 05:08 -0800, Christoph Hellwig wrote:
> > > On Tue, Mar 03, 2026 at 05:28:07PM +0900, Hyunchul Lee wrote:
> > > > s_maxbytes currently is set to MAX_LFS_FILESIZE,
> > > > which allows writes beyond the partition size.
> > > 
> > > The "partition size" does not matter here.  s_maxbytes is the maximum
> > > size supported by the format and has nothing to do with the actual space
> > > allocated to the file system (which in Linux terminology would be the
> > > block device and not the partition anyway).
> > > 
> > > > 
> > > > As a result,
> > > > large-offset writes on small partitions can fail late
> > > > with ENOSPC.
> > > 
> > > That sounds like some other check is missing in hfsplus, but it
> > > should be about the available free space, not the device size.
> > > 
> > 
> > I agree with Christoph.
> > 
> > But, frankly speaking, I don't quite follow which particular issue is under fix
> > here. I can see that generic/268 failure has been mentioned. However, I can see
> > this:
> > 
> > sudo ./check generic/268 
> > FSTYP         -- hfsplus
> > PLATFORM      -- Linux/x86_64 hfsplus-testing-0001 6.19.0-rc1+ #95 SMP
> > PREEMPT_DYNAMIC Thu Feb 19 15:29:55 PST 2026
> > MKFS_OPTIONS  -- /dev/loop51
> > MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch
> > 
> > generic/268       [not run] Reflink not supported by scratch filesystem type:
> > hfsplus
> > Ran: generic/268
> > Not run: generic/268
> > Passed all 1 tests
> > 
> > Which particular issue is under fix?
> 
> Sorry it's generic/285, not generic/268.
> in generic/285, there is a test that creates a hole exceeding the block
> size and appends small data to the file. hfsplus fails because it fills
> the block device and returns ENOSPC. However if it returns EFBIG
> instead, the test is skipped.
> 
> For writes like xfs_io -c "pwrite 8t 512", should fops->write_iter
> returns ENOSPC, or would it be better to return EFBIG?
> > 

Current hfsplus_file_extend() implementation doesn't support holes. I assume you
mean this code [1]:

	len = hip->clump_blocks;
	start = hfsplus_block_allocate(sb, sbi->total_blocks, goal, &len);
	if (start >= sbi->total_blocks) {
		start = hfsplus_block_allocate(sb, goal, 0, &len);
		if (start >= goal) {
			res = -ENOSPC;
			goto out;
		}
	}

Am I correct?

Do you mean that calling logic expects -EFBIG? Potentially, if we tries to
extend the file, then -EFBIG could be more appropriate. But it needs to check
the whole call trace.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.19/source/fs/hfsplus/extents.c#L463

Re: [PATCH] hfsplus: limit sb_maxbytes to partition size

Posted by Hyunchul Lee 1 month, 1 week ago

2026년 3월 5일 (목) AM 9:47, Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>님이 작성:
>
> On Thu, 2026-03-05 at 09:29 +0900, Hyunchul Lee wrote:
> > On Wed, Mar 04, 2026 at 08:04:30PM +0000, Viacheslav Dubeyko wrote:
> > > On Wed, 2026-03-04 at 05:08 -0800, Christoph Hellwig wrote:
> > > > On Tue, Mar 03, 2026 at 05:28:07PM +0900, Hyunchul Lee wrote:
> > > > > s_maxbytes currently is set to MAX_LFS_FILESIZE,
> > > > > which allows writes beyond the partition size.
> > > >
> > > > The "partition size" does not matter here.  s_maxbytes is the maximum
> > > > size supported by the format and has nothing to do with the actual space
> > > > allocated to the file system (which in Linux terminology would be the
> > > > block device and not the partition anyway).
> > > >
> > > > >
> > > > > As a result,
> > > > > large-offset writes on small partitions can fail late
> > > > > with ENOSPC.
> > > >
> > > > That sounds like some other check is missing in hfsplus, but it
> > > > should be about the available free space, not the device size.
> > > >
> > >
> > > I agree with Christoph.
> > >
> > > But, frankly speaking, I don't quite follow which particular issue is under fix
> > > here. I can see that generic/268 failure has been mentioned. However, I can see
> > > this:
> > >
> > > sudo ./check generic/268
> > > FSTYP         -- hfsplus
> > > PLATFORM      -- Linux/x86_64 hfsplus-testing-0001 6.19.0-rc1+ #95 SMP
> > > PREEMPT_DYNAMIC Thu Feb 19 15:29:55 PST 2026
> > > MKFS_OPTIONS  -- /dev/loop51
> > > MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch
> > >
> > > generic/268       [not run] Reflink not supported by scratch filesystem type:
> > > hfsplus
> > > Ran: generic/268
> > > Not run: generic/268
> > > Passed all 1 tests
> > >
> > > Which particular issue is under fix?
> >
> > Sorry it's generic/285, not generic/268.
> > in generic/285, there is a test that creates a hole exceeding the block
> > size and appends small data to the file. hfsplus fails because it fills
> > the block device and returns ENOSPC. However if it returns EFBIG
> > instead, the test is skipped.
> >
> > For writes like xfs_io -c "pwrite 8t 512", should fops->write_iter
> > returns ENOSPC, or would it be better to return EFBIG?
> > >
>
> Current hfsplus_file_extend() implementation doesn't support holes. I assume you
> mean this code [1]:
>
>         len = hip->clump_blocks;
>         start = hfsplus_block_allocate(sb, sbi->total_blocks, goal, &len);
>         if (start >= sbi->total_blocks) {
>                 start = hfsplus_block_allocate(sb, goal, 0, &len);
>                 if (start >= goal) {
>                         res = -ENOSPC;
>                         goto out;
>                 }
>         }
>
> Am I correct?
>
Yes,

hfsplus_write_begin()
  cont_write_begin()
    cont_expand_zero()

1) xfs_io -c "pwrite 8t 512"
2) hfsplus_begin_write() is called with offset 2^43 and length 512
3) cont_expand_zero() allocates and zeroes out one block repeatedly
for the range
0 to 2^43 - 1. To achieve this, hfsplus_write_begin() is called repeatedly.
4) hfsplus_write_begin() allocates one block through hfsplus_get_block() =>
hfsplus_file_extend()

> Do you mean that calling logic expects -EFBIG? Potentially, if we tries to
> extend the file, then -EFBIG could be more appropriate. But it needs to check
> the whole call trace.

generic/285 creates a hole by pwrite at offset 2^43 + @ and handle the
error as follow:
https://github.com/kdave/xfstests/blob/master/src/seek_sanity_test.c#L271

if (errno == EFBIG) {
  fprintf(stdout, "Test skipped as fs doesn't support so large files.\n");
  ret = 0

>
> Thanks,
> Slava.
>
> [1] https://elixir.bootlin.com/linux/v6.19/source/fs/hfsplus/extents.c#L463



-- 
Thanks,
Hyunchul