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) ||
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH 3/3] vfs: return -EOVERFLOW in generic_remap_checks() when overflow check fails Link: https://lore.kernel.org/stable/20240920123022.215863-1-sunjunchao2870%40gmai...
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
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
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.
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
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?
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-sunjunchao287...
Thanks,
linux-stable-mirror@lists.linaro.org
-
Christoph Hellwig -
Darrick J. Wong -
Julian Sun -
kernel test robot