[PATCH 3/3] vfs: return -EOVERFLOW in generic_remap_checks() when overflow check fails

[PATCH 3/3] vfs: return -EOVERFLOW in generic_remap_checks() when overflow check fails

[Julian Sun]

Keep it consistent with the handling of the same check within
generic_copy_file_checks().
Also, returning -EOVERFLOW in this case is more appropriate.

Signed-off-by: Julian Sun <sunjunchao2870@gmail.com>
---
 fs/remap_range.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/remap_range.c b/fs/remap_range.c
index 6fdeb3c8cb70..a26521ded5c8 100644
--- a/fs/remap_range.c
+++ b/fs/remap_range.c
@@ -42,7 +42,7 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in,
 
 	/* The start of both ranges must be aligned to an fs block. */
 	if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_out, bs))
-		return -EINVAL;
+		return -EOVERFLOW;
 
 	/* Ensure offsets don't wrap. */
 	if (check_add_overflow(pos_in, count, &tmp) ||
-- 
2.39.2

Re: [PATCH 3/3] vfs: return -EOVERFLOW in generic_remap_checks() when overflow check fails

[Christoph Hellwig]

On Fri, Sep 20, 2024 at 08:30:22PM +0800, Julian Sun wrote:
> Keep it consistent with the handling of the same check within
> generic_copy_file_checks().
> Also, returning -EOVERFLOW in this case is more appropriate.

Maybe:

Keep the errno value consistent with the equivalent check in
generic_copy_file_checks() that returns -EOVERFLOW, which feels like the
more appropriate value to return compared to the overly generic -EINVAL.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/3] vfs: return -EOVERFLOW in generic_remap_checks() when overflow check fails

[Darrick J. Wong]

On Fri, Sep 20, 2024 at 07:19:28AM -0700, Christoph Hellwig wrote:
> On Fri, Sep 20, 2024 at 08:30:22PM +0800, Julian Sun wrote:
> > Keep it consistent with the handling of the same check within
> > generic_copy_file_checks().
> > Also, returning -EOVERFLOW in this case is more appropriate.
> 
> Maybe:
> 
> Keep the errno value consistent with the equivalent check in
> generic_copy_file_checks() that returns -EOVERFLOW, which feels like the
> more appropriate value to return compared to the overly generic -EINVAL.

The manpage for clone/dedupe/exchange don't say anything about
EOVERFLOW, but they do have this to say about EINVAL:

EINVAL
The  filesystem  does  not  support  reflinking the ranges of the given
files.

Does this errno code change cause any regressions in fstests?

--D

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Re: [PATCH 3/3] vfs: return -EOVERFLOW in generic_remap_checks() when overflow check fails

[Christoph Hellwig]

On Fri, Sep 20, 2024 at 07:37:27AM -0700, Darrick J. Wong wrote:
> On Fri, Sep 20, 2024 at 07:19:28AM -0700, Christoph Hellwig wrote:
> > On Fri, Sep 20, 2024 at 08:30:22PM +0800, Julian Sun wrote:
> > > Keep it consistent with the handling of the same check within
> > > generic_copy_file_checks().
> > > Also, returning -EOVERFLOW in this case is more appropriate.
> > 
> > Maybe:
> > 
> > Keep the errno value consistent with the equivalent check in
> > generic_copy_file_checks() that returns -EOVERFLOW, which feels like the
> > more appropriate value to return compared to the overly generic -EINVAL.
> 
> The manpage for clone/dedupe/exchange don't say anything about
> EOVERFLOW, but they do have this to say about EINVAL:
> 
> EINVAL
> The  filesystem  does  not  support  reflinking the ranges of the given
> files.

Which isn't exactly the integer overflow case described here :)

> Does this errno code change cause any regressions in fstests?

Given our rather sparse test coverage of it I doubt it, but it
would be great to have that confirmed by the submitter.

While we're talking about that - a simple exerciser for the overflow
condition for xfstests would be very useful to have.

Re: [PATCH 3/3] vfs: return -EOVERFLOW in generic_remap_checks() when overflow check fails

[Darrick J. Wong]

On Fri, Sep 20, 2024 at 07:58:01AM -0700, Christoph Hellwig wrote:
> On Fri, Sep 20, 2024 at 07:37:27AM -0700, Darrick J. Wong wrote:
> > On Fri, Sep 20, 2024 at 07:19:28AM -0700, Christoph Hellwig wrote:
> > > On Fri, Sep 20, 2024 at 08:30:22PM +0800, Julian Sun wrote:
> > > > Keep it consistent with the handling of the same check within
> > > > generic_copy_file_checks().
> > > > Also, returning -EOVERFLOW in this case is more appropriate.
> > > 
> > > Maybe:
> > > 
> > > Keep the errno value consistent with the equivalent check in
> > > generic_copy_file_checks() that returns -EOVERFLOW, which feels like the
> > > more appropriate value to return compared to the overly generic -EINVAL.
> > 
> > The manpage for clone/dedupe/exchange don't say anything about
> > EOVERFLOW, but they do have this to say about EINVAL:
> > 
> > EINVAL
> > The  filesystem  does  not  support  reflinking the ranges of the given
> > files.
> 
> Which isn't exactly the integer overflow case described here :)

Hm?  This patch is touching the error code you get for failing alignment
checks, not the one you get for failing check_add_overflow.  EOVERFLOW
seems like an odd return code for unaligned arguments.  Though you're
right that EINVAL is verrry vague.

> > Does this errno code change cause any regressions in fstests?
> 
> Given our rather sparse test coverage of it I doubt it, but it
> would be great to have that confirmed by the submitter.

Yes. :)

> While we're talking about that - a simple exerciser for the overflow
> condition for xfstests would be very useful to have.

Yes, there's <cough> supposed to be one that does that.

$ git grep -ci CLONE.*invalid.argument
common/filter.btrfs:1
tests/btrfs/035.out:1
tests/btrfs/052.out:12
tests/btrfs/096.out:1
tests/btrfs/112.out:16
tests/btrfs/113.out:1
tests/btrfs/229:1
tests/generic/157.out:6
tests/generic/303.out:4
tests/generic/518.out:1


--D

Re: [PATCH 3/3] vfs: return -EOVERFLOW in generic_remap_checks() when overflow check fails

[Christoph Hellwig]

On Fri, Sep 20, 2024 at 08:02:13AM -0700, Darrick J. Wong wrote:
> > Which isn't exactly the integer overflow case described here :)
> 
> Hm?  This patch is touching the error code you get for failing alignment
> checks, not the one you get for failing check_add_overflow.  EOVERFLOW
> seems like an odd return code for unaligned arguments.  Though you're
> right that EINVAL is verrry vague.

I misread the patch (or rather mostly read the description).  Yes,
-EOVERFLOW is rather odd here.  And generic_copy_file_checks doesn't
even have alignment checks, so the message is wrong as well.  I'll
wait for Jun what the intention was here - maybe the diff got
misapplied and this was supposed to be applied to an  overflow
check that returns -EINVAL?

Re: [PATCH 3/3] vfs: return -EOVERFLOW in generic_remap_checks() when overflow check fails

[Julian Sun]

Christoph Hellwig <hch@infradead.org> 于2024年9月20日周五 23:07写道：
>
> On Fri, Sep 20, 2024 at 08:02:13AM -0700, Darrick J. Wong wrote:
> > > Which isn't exactly the integer overflow case described here :)
> >
> > Hm?  This patch is touching the error code you get for failing alignment
> > checks, not the one you get for failing check_add_overflow.  EOVERFLOW
> > seems like an odd return code for unaligned arguments.  Though you're
> > right that EINVAL is verrry vague.
>
> I misread the patch (or rather mostly read the description).  Yes,
> -EOVERFLOW is rather odd here.  And generic_copy_file_checks doesn't
> even have alignment checks, so the message is wrong as well.  I'll
> wait for Jun what the intention was here - maybe the diff got
> misapplied and this was supposed to be applied to an  overflow
> check that returns -EINVAL?

Yeah... The patch was originally intended for overflow check and
sourced from [1], differs from its description. After applying it to
the latest kernel version, there were no warnings or errors, but I
suspect there may be an issue with the git apply process. I'll fix it
in the patch v2, thanks.

[1]: https://lore.kernel.org/linux-fsdevel/20240906033202.1252195-1-sunjunchao2870@gmail.com/
>

Thanks,
-- 
Julian Sun <sunjunchao2870@gmail.com>