| Message ID | 20240920123022.215863-1-sunjunchao2870@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/3] xfs: Do not unshare ranges beyond EOF | expand |
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-sunjunchao2870@gmail.com/ > Thanks,
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) ||
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(-)