| Message ID | 20190807094119.10834-1-pvorel@suse.cz |
|---|---|
| State | Rejected |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | syscalls/copy_file_range02: Fix #12 when TMPDIR is on tmpfs or ext[234] | expand |
----- Original Message ----- > Recent fix 96e6e8f4a68d ("vfs: add missing checks to copy_file_range") > in kernel v5.3-rc1 changed errno for ext[234] and tmpfs to EINVAL. > > + fix typo from 0242a9a29 > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > Hi all, > > Amir, Darrick, am I right that it's not a problem that errno changed > recently, is it? There have been more errno changes due several fixes in > copy_file_range() in the past. I've seen this approach several times in > other subsystems (e.g. network). Hope userspace not often check for > particular error. > > Cyril, Jan, Li, still not sure what the policy about errno is (see > Cyril's statements in recent discussion about it in Jinhui's patch [1] > [2]). With these frequent changes we should IMHO check for all possible > variants (EXDEV, EFBIG, EINVAL). Petr, Looking at test code, it does seem like testcase you listed violates multiple checks at same time. Are you getting EINVAL on tmpfs/ext234 even when source file length > MIN_OFF ? EINVAL: Requested range extends beyond the end of the source file; or the flags argument is not 0. Regards, Jan > > Or should we investigate all fixes and keep errors which highlight > important fix was not backported (to both stable and LTS/enterprise > distros kernels?). That'd be weird but approach practical :). > > Anyway, we should define and write down LTP policy / rules about it. > > Kind regards, > Petr > > [1] https://patchwork.ozlabs.org/patch/1108229/#2182801 > [2] https://patchwork.ozlabs.org/patch/1108229/#2182837 > > .../copy_file_range/copy_file_range02.c | 20 +++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > index 9004c4a40..b113e44b5 100644 > --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > @@ -49,6 +49,7 @@ static int fd_blkdev; > static int fd_chrdev; > static int fd_fifo; > static int fd_copy; > +static int fd_copy2; > > static int chattr_i_nsup; > static int swap_nsup; > @@ -73,8 +74,8 @@ static struct tcase { > {&fd_blkdev, 0, EINVAL, 0, CONTSIZE, "block device"}, > {&fd_chrdev, 0, EINVAL, 0, CONTSIZE, "char device"}, > {&fd_fifo, 0, EINVAL, 0, CONTSIZE, "fifo"}, > - {&fd_copy, 0, EOVERFLOW, MAX_OFF, ULLONG_MAX, "max length lenght"}, > - {&fd_copy, 0, EFBIG, MAX_OFF, MIN_OFF, "max file size"}, > + {&fd_copy, 0, EOVERFLOW, MAX_OFF, ULLONG_MAX, "max length"}, > + {&fd_copy2, 0, EFBIG, MAX_OFF, MIN_OFF, "max file size"}, > }; > > static int run_command(char *command, char *option, char *file) > @@ -100,6 +101,7 @@ static void verify_copy_file_range(unsigned int n) > struct tcase *tc = &tcases[n]; > tst_res(TINFO, "Test #%d: %s", n, tc->tname); > > + > if (tc->copy_to_fd == &fd_immutable && chattr_i_nsup) { > tst_res(TCONF, "filesystem doesn't support chattr +i, skip it"); > return; > @@ -112,6 +114,20 @@ static void verify_copy_file_range(unsigned int n) > tst_res(TCONF, "filesystem doesn't have free loopdev, skip it"); > return; > } > + > + if (tc->copy_to_fd == &fd_copy2) { > + long fs_type = tst_fs_type("."); > + switch (fs_type) { > + case TST_TMPFS_MAGIC: > + case TST_EXT234_MAGIC: > + default: > + tc->exp_err = EINVAL; > + tst_res(TINFO, "%s filesystem, changing expecting errno to %d", > + tst_fs_type_name(fs_type), tc->exp_err); > + break; > + } > + } > + > TEST(sys_copy_file_range(fd_src, 0, *tc->copy_to_fd, > &tc->dst, tc->len, tc->flags)); > > -- > 2.22.0 > >
On Wed, Aug 07, 2019 at 11:41:19AM +0200, Petr Vorel wrote: > Recent fix 96e6e8f4a68d ("vfs: add missing checks to copy_file_range") > in kernel v5.3-rc1 changed errno for ext[234] and tmpfs to EINVAL. > > + fix typo from 0242a9a29 > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > Hi all, > > Amir, Darrick, am I right that it's not a problem that errno changed > recently, is it? There have been more errno changes due several fixes in > copy_file_range() in the past. I've seen this approach several times in > other subsystems (e.g. network). Hope userspace not often check for > particular error. Same here. I think EXDEV is the only errno that can be worked around in userspace (with a classic read-write loop). > Cyril, Jan, Li, still not sure what the policy about errno is (see > Cyril's statements in recent discussion about it in Jinhui's patch [1] > [2]). With these frequent changes we should IMHO check for all possible > variants (EXDEV, EFBIG, EINVAL). I for one hope we're done messing with the (bad input) -> errno mappings. > Or should we investigate all fixes and keep errors which highlight > important fix was not backported (to both stable and LTS/enterprise > distros kernels?). That'd be weird but approach practical :). copy_file_range was kinda broken before 5.3 if you were using it on a disk filesystem, so I'd hope the LTS folks would backport them (or turn c_f_r off). --D > Anyway, we should define and write down LTP policy / rules about it. > > Kind regards, > Petr > > [1] https://patchwork.ozlabs.org/patch/1108229/#2182801 > [2] https://patchwork.ozlabs.org/patch/1108229/#2182837 > > .../copy_file_range/copy_file_range02.c | 20 +++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > index 9004c4a40..b113e44b5 100644 > --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > @@ -49,6 +49,7 @@ static int fd_blkdev; > static int fd_chrdev; > static int fd_fifo; > static int fd_copy; > +static int fd_copy2; > > static int chattr_i_nsup; > static int swap_nsup; > @@ -73,8 +74,8 @@ static struct tcase { > {&fd_blkdev, 0, EINVAL, 0, CONTSIZE, "block device"}, > {&fd_chrdev, 0, EINVAL, 0, CONTSIZE, "char device"}, > {&fd_fifo, 0, EINVAL, 0, CONTSIZE, "fifo"}, > - {&fd_copy, 0, EOVERFLOW, MAX_OFF, ULLONG_MAX, "max length lenght"}, > - {&fd_copy, 0, EFBIG, MAX_OFF, MIN_OFF, "max file size"}, > + {&fd_copy, 0, EOVERFLOW, MAX_OFF, ULLONG_MAX, "max length"}, > + {&fd_copy2, 0, EFBIG, MAX_OFF, MIN_OFF, "max file size"}, > }; > > static int run_command(char *command, char *option, char *file) > @@ -100,6 +101,7 @@ static void verify_copy_file_range(unsigned int n) > struct tcase *tc = &tcases[n]; > tst_res(TINFO, "Test #%d: %s", n, tc->tname); > > + > if (tc->copy_to_fd == &fd_immutable && chattr_i_nsup) { > tst_res(TCONF, "filesystem doesn't support chattr +i, skip it"); > return; > @@ -112,6 +114,20 @@ static void verify_copy_file_range(unsigned int n) > tst_res(TCONF, "filesystem doesn't have free loopdev, skip it"); > return; > } > + > + if (tc->copy_to_fd == &fd_copy2) { > + long fs_type = tst_fs_type("."); > + switch (fs_type) { > + case TST_TMPFS_MAGIC: > + case TST_EXT234_MAGIC: > + default: > + tc->exp_err = EINVAL; > + tst_res(TINFO, "%s filesystem, changing expecting errno to %d", > + tst_fs_type_name(fs_type), tc->exp_err); > + break; > + } > + } > + > TEST(sys_copy_file_range(fd_src, 0, *tc->copy_to_fd, > &tc->dst, tc->len, tc->flags)); > > -- > 2.22.0 >
Hi Petr, Thanks for highlight this. > Cyril, Jan, Li, still not sure what the policy about errno is (see > Cyril's statements in recent discussion about it in Jinhui's patch [1] > [2]). With these frequent changes we should IMHO check for all possible > variants (EXDEV, EFBIG, EINVAL). > > Or should we investigate all fixes and keep errors which highlight > important fix was not backported (to both stable and LTS/enterprise > distros kernels?). That'd be weird but approach practical :). That sounds not very realistic. We can't satisfy all distributions for errno checking, because we don't know when/wheather it backports the patch. > > Anyway, we should define and write down LTP policy / rules about it. I think we might have these ways of choosing: 1. Only align with the latest kernel behavior e.g limit the latest kernel version and verify that valid errno The disadvantage of this is only tested on a small kernel range. 2. Guarded by kernel version check as Cyril suggests in [1] e.g kernel >= 4.10; Check errno == EBADF kernel < 4.10; Check errno == EISDIR .... The disadvantage is that the test result is affected by LTS with a backport-patch. 3. Regards all acceptable errnos as valid on any kernel version e.g whatever errno get any of them EXDEV, EFBIG, EINVAL, regard valid This sounds obviously awful. Or, anything else?
Hi Li, > Thanks for highlight this. > > Cyril, Jan, Li, still not sure what the policy about errno is (see > > Cyril's statements in recent discussion about it in Jinhui's patch [1] > > [2]). With these frequent changes we should IMHO check for all possible > > variants (EXDEV, EFBIG, EINVAL). > > Or should we investigate all fixes and keep errors which highlight > > important fix was not backported (to both stable and LTS/enterprise > > distros kernels?). That'd be weird but approach practical :). > That sounds not very realistic. We can't satisfy all distributions for > errno checking, because we don't know when/wheather it backports the > patch. I meant it like taking individual approach: insist on errno when different errno means something important hasn't been fixed. > > Anyway, we should define and write down LTP policy / rules about it. > I think we might have these ways of choosing: > 1. Only align with the latest kernel behavior > e.g limit the latest kernel version and verify that valid errno > The disadvantage of this is only tested on a small kernel range. Sticking only for latest kernel is a big advantage. > 2. Guarded by kernel version check as Cyril suggests in [1] > e.g kernel >= 4.10; Check errno == EBADF > kernel < 4.10; Check errno == EISDIR We use this one 278bbbb35 syscalls/fgetxattr02.c: Fix errno EPERM on older kernels 21c27c11c syscalls/bind03: Add version compare according to behaviour difference. > .... > The disadvantage is that the test result is affected by LTS with a > backport-patch. Yep, that's problematic. > 3. Regards all acceptable errnos as valid on any kernel version > e.g whatever errno get any of them EXDEV, EFBIG, EINVAL, regard valid > This sounds obviously awful. Yep, but the problem is that we considered changed errno as a bug, but for kernel developers it's not always a bug. See fix "RFC: changed error code when binding unix socket twice" [1], which was never merged (it fixed a bug, so changing errno was not a big issue). I'm sure there more cases like this one. > Or, anything else? How about try errno for latest kernel for any kernel (in case it has been backported) and if not do the check based on kernel version? Probably combined with TWARN. Kind regards, Petr [1] https://marc.info/?l=linux-kernel&m=149880810113888&w=2
On Fri, Aug 9, 2019 at 5:43 PM Petr Vorel <pvorel@suse.cz> wrote: > > > Or, anything else? > > How about try errno for latest kernel for any kernel (in case it has been > backported) and if not do the check based on kernel version? > Probably combined with TWARN. To combine the methods(1 and 2) together sounds good. But not sure if that could be a formal policy/rule for LTP. We need some more thinking/comments.
diff --git a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c index 9004c4a40..b113e44b5 100644 --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c @@ -49,6 +49,7 @@ static int fd_blkdev; static int fd_chrdev; static int fd_fifo; static int fd_copy; +static int fd_copy2; static int chattr_i_nsup; static int swap_nsup; @@ -73,8 +74,8 @@ static struct tcase { {&fd_blkdev, 0, EINVAL, 0, CONTSIZE, "block device"}, {&fd_chrdev, 0, EINVAL, 0, CONTSIZE, "char device"}, {&fd_fifo, 0, EINVAL, 0, CONTSIZE, "fifo"}, - {&fd_copy, 0, EOVERFLOW, MAX_OFF, ULLONG_MAX, "max length lenght"}, - {&fd_copy, 0, EFBIG, MAX_OFF, MIN_OFF, "max file size"}, + {&fd_copy, 0, EOVERFLOW, MAX_OFF, ULLONG_MAX, "max length"}, + {&fd_copy2, 0, EFBIG, MAX_OFF, MIN_OFF, "max file size"}, }; static int run_command(char *command, char *option, char *file) @@ -100,6 +101,7 @@ static void verify_copy_file_range(unsigned int n) struct tcase *tc = &tcases[n]; tst_res(TINFO, "Test #%d: %s", n, tc->tname); + if (tc->copy_to_fd == &fd_immutable && chattr_i_nsup) { tst_res(TCONF, "filesystem doesn't support chattr +i, skip it"); return; @@ -112,6 +114,20 @@ static void verify_copy_file_range(unsigned int n) tst_res(TCONF, "filesystem doesn't have free loopdev, skip it"); return; } + + if (tc->copy_to_fd == &fd_copy2) { + long fs_type = tst_fs_type("."); + switch (fs_type) { + case TST_TMPFS_MAGIC: + case TST_EXT234_MAGIC: + default: + tc->exp_err = EINVAL; + tst_res(TINFO, "%s filesystem, changing expecting errno to %d", + tst_fs_type_name(fs_type), tc->exp_err); + break; + } + } + TEST(sys_copy_file_range(fd_src, 0, *tc->copy_to_fd, &tc->dst, tc->len, tc->flags));
Recent fix 96e6e8f4a68d ("vfs: add missing checks to copy_file_range") in kernel v5.3-rc1 changed errno for ext[234] and tmpfs to EINVAL. + fix typo from 0242a9a29 Signed-off-by: Petr Vorel <pvorel@suse.cz> --- Hi all, Amir, Darrick, am I right that it's not a problem that errno changed recently, is it? There have been more errno changes due several fixes in copy_file_range() in the past. I've seen this approach several times in other subsystems (e.g. network). Hope userspace not often check for particular error. Cyril, Jan, Li, still not sure what the policy about errno is (see Cyril's statements in recent discussion about it in Jinhui's patch [1] [2]). With these frequent changes we should IMHO check for all possible variants (EXDEV, EFBIG, EINVAL). Or should we investigate all fixes and keep errors which highlight important fix was not backported (to both stable and LTS/enterprise distros kernels?). That'd be weird but approach practical :). Anyway, we should define and write down LTP policy / rules about it. Kind regards, Petr [1] https://patchwork.ozlabs.org/patch/1108229/#2182801 [2] https://patchwork.ozlabs.org/patch/1108229/#2182837 .../copy_file_range/copy_file_range02.c | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)