| Message ID | 1559292243-2882-1-git-send-email-huangjh.jy@cn.fujitsu.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | syscalls/copy_file_range02.c: Compatible with new and old kernels | expand |
On Fri, May 31, 2019 at 4:44 PM Jinhui huang <huangjh.jy@cn.fujitsu.com> wrote: > On new kernels, copy_file_range() returned EISDIR when copyed contents > to directory, but on old kernels, it returned EBADF, we should accept > EBADF to be compatible with new and old kernels. > > The patch as follows: > commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") > Patch makes sense. Reviewed-by: Li Wang <liwang@redhat.com> > > Signed-off-by: Jinhui huang <huangjh.jy@cn.fujitsu.com> > --- > .../syscalls/copy_file_range/copy_file_range02.c | 33 > +++++++++++++++------- > 1 file changed, 23 insertions(+), 10 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 07c0207..9e6356c 100644 > --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > @@ -54,19 +54,32 @@ static void verify_copy_file_range(unsigned int n) > TEST(sys_copy_file_range(fd_src, 0, *tc->copy_to_fd, > 0, CONTSIZE, tc->flags)); > > - if (TST_RET == -1) { > - if (tc->exp_err == TST_ERR) { > + if (TST_RET != -1) { > + tst_res(TFAIL, > + "copy_file_range returned wrong value: %ld", > TST_RET); > + return; > + } > + > + if (tc->exp_err == TST_ERR) { > + tst_res(TPASS | TTERRNO, > + "copy_file_range failed as expected"); > + } else { > + /* copy_file_range() returned EISDIR when copyed contents > to > + * directory on new kernels, but on old kernels, it > returned > + * EBADF. > + * > + * the patch as follws: > + * commit 11cbfb10775a ("vfs: deny copy_file_range() for > non regular files") > + */ > + if (tc->exp_err == EISDIR && TST_ERR == EBADF) { > tst_res(TPASS | TTERRNO, > - "copy_file_range failed as > expected"); > - } else { > - tst_res(TFAIL | TTERRNO, > - "copy_file_range failed unexpectedly; > expected %s, but got", > - tst_strerrno(tc->exp_err)); > + "copy_file_range failed as expected"); > return; > } > - } else { > - tst_res(TFAIL, > - "copy_file_range returned wrong value: %ld", > TST_RET); > + > + tst_res(TFAIL | TTERRNO, > + "copy_file_range failed unexpectedly; expected %s, > but got", > + tst_strerrno(tc->exp_err)); > } > } > > -- > 1.8.3.1 > > > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp >
On 2019/05/31 16:44, Jinhui huang wrote: > On new kernels, copy_file_range() returned EISDIR when copyed contents > to directory, but on old kernels, it returned EBADF, we should accept > EBADF to be compatible with new and old kernels. > > The patch as follows: > commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") Hi, From description of commit, I wonder if we can add more tests for some non regular files(e.g. block, pipe)? I just want to increase coverage and fix all similar issues as you did. :-) Best Regards, Xiao Yang > Signed-off-by: Jinhui huang <huangjh.jy@cn.fujitsu.com> > --- > .../syscalls/copy_file_range/copy_file_range02.c | 33 +++++++++++++++------- > 1 file changed, 23 insertions(+), 10 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 07c0207..9e6356c 100644 > --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > @@ -54,19 +54,32 @@ static void verify_copy_file_range(unsigned int n) > TEST(sys_copy_file_range(fd_src, 0, *tc->copy_to_fd, > 0, CONTSIZE, tc->flags)); > > - if (TST_RET == -1) { > - if (tc->exp_err == TST_ERR) { > + if (TST_RET != -1) { > + tst_res(TFAIL, > + "copy_file_range returned wrong value: %ld", TST_RET); > + return; > + } > + > + if (tc->exp_err == TST_ERR) { > + tst_res(TPASS | TTERRNO, > + "copy_file_range failed as expected"); > + } else { > + /* copy_file_range() returned EISDIR when copyed contents to > + * directory on new kernels, but on old kernels, it returned > + * EBADF. > + * > + * the patch as follws: > + * commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") > + */ > + if (tc->exp_err == EISDIR && TST_ERR == EBADF) { > tst_res(TPASS | TTERRNO, > - "copy_file_range failed as expected"); > - } else { > - tst_res(TFAIL | TTERRNO, > - "copy_file_range failed unexpectedly; expected %s, but got", > - tst_strerrno(tc->exp_err)); > + "copy_file_range failed as expected"); > return; > } > - } else { > - tst_res(TFAIL, > - "copy_file_range returned wrong value: %ld", TST_RET); > + > + tst_res(TFAIL | TTERRNO, > + "copy_file_range failed unexpectedly; expected %s, but got", > + tst_strerrno(tc->exp_err)); > } > } >
Hi! > The patch as follows: > commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") > > Signed-off-by: Jinhui huang <huangjh.jy@cn.fujitsu.com> > --- > .../syscalls/copy_file_range/copy_file_range02.c | 33 +++++++++++++++------- > 1 file changed, 23 insertions(+), 10 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 07c0207..9e6356c 100644 > --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > @@ -54,19 +54,32 @@ static void verify_copy_file_range(unsigned int n) > TEST(sys_copy_file_range(fd_src, 0, *tc->copy_to_fd, > 0, CONTSIZE, tc->flags)); > > - if (TST_RET == -1) { > - if (tc->exp_err == TST_ERR) { > + if (TST_RET != -1) { > + tst_res(TFAIL, > + "copy_file_range returned wrong value: %ld", TST_RET); > + return; > + } > + > + if (tc->exp_err == TST_ERR) { > + tst_res(TPASS | TTERRNO, > + "copy_file_range failed as expected"); > + } else { > + /* copy_file_range() returned EISDIR when copyed contents to > + * directory on new kernels, but on old kernels, it returned > + * EBADF. > + * > + * the patch as follws: > + * commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") > + */ > + if (tc->exp_err == EISDIR && TST_ERR == EBADF) { > tst_res(TPASS | TTERRNO, > - "copy_file_range failed as expected"); If nothing else this should be guarded by kernel version check because if new kernel starts to return EBADFD again, that would be regression. So we should check the kernel version in test setup and set a flag that would be checked here as well.
On Fri, May 31, 2019 at 7:01 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > The patch as follows: > > commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") > > > > Signed-off-by: Jinhui huang <huangjh.jy@cn.fujitsu.com> > > --- > > .../syscalls/copy_file_range/copy_file_range02.c | 33 > +++++++++++++++------- > > 1 file changed, 23 insertions(+), 10 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 07c0207..9e6356c 100644 > > --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > > +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > > @@ -54,19 +54,32 @@ static void verify_copy_file_range(unsigned int n) > > TEST(sys_copy_file_range(fd_src, 0, *tc->copy_to_fd, > > 0, CONTSIZE, tc->flags)); > > > > - if (TST_RET == -1) { > > - if (tc->exp_err == TST_ERR) { > > + if (TST_RET != -1) { > > + tst_res(TFAIL, > > + "copy_file_range returned wrong value: %ld", > TST_RET); > > + return; > > + } > > + > > + if (tc->exp_err == TST_ERR) { > > + tst_res(TPASS | TTERRNO, > > + "copy_file_range failed as expected"); > > + } else { > > + /* copy_file_range() returned EISDIR when copyed contents > to > > + * directory on new kernels, but on old kernels, it > returned > > + * EBADF. > > + * > > + * the patch as follws: > > + * commit 11cbfb10775a ("vfs: deny copy_file_range() for > non regular files") > > + */ > > + if (tc->exp_err == EISDIR && TST_ERR == EBADF) { > > tst_res(TPASS | TTERRNO, > > - "copy_file_range failed as > expected"); > > If nothing else this should be guarded by kernel version check because > if new kernel starts to return EBADFD again, that would be regression. > > So we should check the kernel version in test setup and set a flag that > would be checked here as well. > This is a good suggestion. Another point I can come up is, if an LTS Linux distribution backports that commit 11cbfb10775a to their old kernel as regression fix, then this flag will make no sense. So, to strict we maybe need to regards the EISDIR as the only one legal errno(copying content to dir) when kernel >= 4.10(includes commit 11cbfb10775a). > > -- > Cyril Hrubis > chrubis@suse.cz > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp >
Hi! > This is a good suggestion. Another point I can come up is, if an LTS Linux > distribution backports that commit 11cbfb10775a to their old kernel as > regression fix, then this flag will make no sense. Well that's life, there is no way how to detect if patch was backported or not, so either we make the test pass on both EISDIR and EBADFD on older kernels or we leave it as it is. Neither of the solutions is perfect. > So, to strict we maybe need to regards the EISDIR as the only one legal > errno(copying content to dir) when kernel >= 4.10(includes commit > 11cbfb10775a). Yes, exactly.
On Fri, May 31, 2019 at 6:15 PM Xiao Yang <yangx.jy@cn.fujitsu.com> wrote: > On 2019/05/31 16:44, Jinhui huang wrote: > > On new kernels, copy_file_range() returned EISDIR when copyed contents > > to directory, but on old kernels, it returned EBADF, we should accept > > EBADF to be compatible with new and old kernels. > > > > The patch as follows: > > commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") > Hi, > > From description of commit, I wonder if we can add more tests for some > non regular files(e.g. block, pipe)? > I have no objection on this. But, is there really make sense to test some more non regular files which not being mentioned by Linux Manual Page? The copy_file_range02 test errors are all extract from manual page, I commented that in Christian's first patch version. And I don't think it's necessary to test undefined behavior in syscall using, because how do we know what error return is the expected? I just want to increase coverage and fix all similar issues as you did. :-) > > Best Regards, > Xiao Yang > > Signed-off-by: Jinhui huang <huangjh.jy@cn.fujitsu.com> > > --- > > .../syscalls/copy_file_range/copy_file_range02.c | 33 > +++++++++++++++------- > > 1 file changed, 23 insertions(+), 10 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 07c0207..9e6356c 100644 > > --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > > +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c > > @@ -54,19 +54,32 @@ static void verify_copy_file_range(unsigned int n) > > TEST(sys_copy_file_range(fd_src, 0, *tc->copy_to_fd, > > 0, CONTSIZE, tc->flags)); > > > > - if (TST_RET == -1) { > > - if (tc->exp_err == TST_ERR) { > > + if (TST_RET != -1) { > > + tst_res(TFAIL, > > + "copy_file_range returned wrong value: %ld", > TST_RET); > > + return; > > + } > > + > > + if (tc->exp_err == TST_ERR) { > > + tst_res(TPASS | TTERRNO, > > + "copy_file_range failed as expected"); > > + } else { > > + /* copy_file_range() returned EISDIR when copyed contents > to > > + * directory on new kernels, but on old kernels, it > returned > > + * EBADF. > > + * > > + * the patch as follws: > > + * commit 11cbfb10775a ("vfs: deny copy_file_range() for > non regular files") > > + */ > > + if (tc->exp_err == EISDIR && TST_ERR == EBADF) { > > tst_res(TPASS | TTERRNO, > > - "copy_file_range failed as > expected"); > > - } else { > > - tst_res(TFAIL | TTERRNO, > > - "copy_file_range failed unexpectedly; > expected %s, but got", > > - tst_strerrno(tc->exp_err)); > > + "copy_file_range failed as expected"); > > return; > > } > > - } else { > > - tst_res(TFAIL, > > - "copy_file_range returned wrong value: %ld", > TST_RET); > > + > > + tst_res(TFAIL | TTERRNO, > > + "copy_file_range failed unexpectedly; expected %s, > but got", > > + tst_strerrno(tc->exp_err)); > > } > > } > > > > > >
Hi! > I have no objection on this. But, is there really make sense to test some > more non regular files which not being mentioned by Linux Manual Page? > > The copy_file_range02 test errors are all extract from manual page, I > commented that in Christian's first patch version. And I don't think it's > necessary to test undefined behavior in syscall using, because how do we > know what error return is the expected? That's not undefined that's undocummented at best. The kernel code for vfs_copy_file_range does: if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode)) return -EISDIR; if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode)) return -EINVAL; Which means that directories are treated as special here and all other file descriptors that are not regular files are supposed to fail with EINVAL. So as far as I can tell it makes sense to pass a pipe fd for example and check for EINVAL. And we should do that both for in_fd and out_fd as well.
On Fri, May 31, 2019 at 8:26 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > I have no objection on this. But, is there really make sense to test some > > more non regular files which not being mentioned by Linux Manual Page? > > > > The copy_file_range02 test errors are all extract from manual page, I > > commented that in Christian's first patch version. And I don't think it's > > necessary to test undefined behavior in syscall using, because how do we > > know what error return is the expected? > > That's not undefined that's undocummented at best. The kernel code for > vfs_copy_file_range does: > > if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode)) > return -EISDIR; > if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode)) > return -EINVAL; > > Which means that directories are treated as special here and all other file > descriptors that are not regular files are supposed to fail with EINVAL. > Yeah, Indeed true. > So as far as I can tell it makes sense to pass a pipe fd for example and > check > for EINVAL. And we should do that both for in_fd and out_fd as well. > Sounds reasonable. Thanks for the explanation.
On Fri, May 31, 2019 at 3:03 PM Li Wang <liwang@redhat.com> wrote: > > > > On Fri, May 31, 2019 at 6:15 PM Xiao Yang <yangx.jy@cn.fujitsu.com> wrote: >> >> On 2019/05/31 16:44, Jinhui huang wrote: >> > On new kernels, copy_file_range() returned EISDIR when copyed contents >> > to directory, but on old kernels, it returned EBADF, we should accept >> > EBADF to be compatible with new and old kernels. >> > >> > The patch as follows: >> > commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") >> Hi, >> >> From description of commit, I wonder if we can add more tests for some >> non regular files(e.g. block, pipe)? > > > I have no objection on this. But, is there really make sense to test some more non regular files which not being mentioned by Linux Manual Page? > FYI, more changes to copy_file_range checks are in the works: https://lore.kernel.org/linux-fsdevel/20190526061100.21761-1-amir73il@gmail.com/T/#me34d4363449118bd3b2ec8421d282a77e9a7d2d1 > The copy_file_range02 test errors are all extract from manual page, I commented that in Christian's first patch version. And I don't think it's necessary to test undefined behavior in syscall using, because how do we know what error return is the expected? > >> I just want to increase coverage and fix all similar issues as you did. :-) >> >> Best Regards, >> Xiao Yang >> > Signed-off-by: Jinhui huang <huangjh.jy@cn.fujitsu.com> >> > --- >> > .../syscalls/copy_file_range/copy_file_range02.c | 33 +++++++++++++++------- >> > 1 file changed, 23 insertions(+), 10 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 07c0207..9e6356c 100644 >> > --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c >> > +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c >> > @@ -54,19 +54,32 @@ static void verify_copy_file_range(unsigned int n) >> > TEST(sys_copy_file_range(fd_src, 0, *tc->copy_to_fd, >> > 0, CONTSIZE, tc->flags)); >> > >> > - if (TST_RET == -1) { >> > - if (tc->exp_err == TST_ERR) { >> > + if (TST_RET != -1) { >> > + tst_res(TFAIL, >> > + "copy_file_range returned wrong value: %ld", TST_RET); >> > + return; >> > + } >> > + >> > + if (tc->exp_err == TST_ERR) { >> > + tst_res(TPASS | TTERRNO, >> > + "copy_file_range failed as expected"); >> > + } else { >> > + /* copy_file_range() returned EISDIR when copyed contents to >> > + * directory on new kernels, but on old kernels, it returned >> > + * EBADF. >> > + * >> > + * the patch as follws: >> > + * commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") >> > + */ >> > + if (tc->exp_err == EISDIR && TST_ERR == EBADF) { >> > tst_res(TPASS | TTERRNO, >> > - "copy_file_range failed as expected"); >> > - } else { >> > - tst_res(TFAIL | TTERRNO, >> > - "copy_file_range failed unexpectedly; expected %s, but got", >> > - tst_strerrno(tc->exp_err)); >> > + "copy_file_range failed as expected"); >> > return; >> > } >> > - } else { >> > - tst_res(TFAIL, >> > - "copy_file_range returned wrong value: %ld", TST_RET); >> > + >> > + tst_res(TFAIL | TTERRNO, >> > + "copy_file_range failed unexpectedly; expected %s, but got", >> > + tst_strerrno(tc->exp_err)); >> > } >> > } >> > >> >> >> > > > -- > Regards, > Li Wang > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
on 2019/05/31 21:02, Amir Goldstein wrote: > On Fri, May 31, 2019 at 3:03 PM Li Wang<liwang@redhat.com> wrote: >> >> >> On Fri, May 31, 2019 at 6:15 PM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: >>> On 2019/05/31 16:44, Jinhui huang wrote: >>>> On new kernels, copy_file_range() returned EISDIR when copyed contents >>>> to directory, but on old kernels, it returned EBADF, we should accept >>>> EBADF to be compatible with new and old kernels. >>>> >>>> The patch as follows: >>>> commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") >>> Hi, >>> >>> From description of commit, I wonder if we can add more tests for some >>> non regular files(e.g. block, pipe)? >> >> I have no objection on this. But, is there really make sense to test some more non regular files which not being mentioned by Linux Manual Page? >> > FYI, more changes to copy_file_range checks are in the works: > https://lore.kernel.org/linux-fsdevel/20190526061100.21761-1-amir73il@gmail.com/T/#me34d4363449118bd3b2ec8421d282a77e9a7d2d1 Hi Amir Meet again. We can increase ltp copy_file_range02 coverage include( swapfile ->ETXTBUSY, immutable file->EPERM) as same as xfstests generic/553[4]. Also the two other checks(overlaping and offset wrap). I am glad to do it. PS: Why we don't have test for overlaping and offset wrap check on xfstests? Or, I miss it? Thanks Yang Xu >> The copy_file_range02 test errors are all extract from manual page, I commented that in Christian's first patch version. And I don't think it's necessary to test undefined behavior in syscall using, because how do we know what error return is the expected? >> >>> I just want to increase coverage and fix all similar issues as you did. :-) >>> >>> Best Regards, >>> Xiao Yang >>>> Signed-off-by: Jinhui huang<huangjh.jy@cn.fujitsu.com> >>>> --- >>>> .../syscalls/copy_file_range/copy_file_range02.c | 33 +++++++++++++++------- >>>> 1 file changed, 23 insertions(+), 10 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 07c0207..9e6356c 100644 >>>> --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c >>>> +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c >>>> @@ -54,19 +54,32 @@ static void verify_copy_file_range(unsigned int n) >>>> TEST(sys_copy_file_range(fd_src, 0, *tc->copy_to_fd, >>>> 0, CONTSIZE, tc->flags)); >>>> >>>> - if (TST_RET == -1) { >>>> - if (tc->exp_err == TST_ERR) { >>>> + if (TST_RET != -1) { >>>> + tst_res(TFAIL, >>>> + "copy_file_range returned wrong value: %ld", TST_RET); >>>> + return; >>>> + } >>>> + >>>> + if (tc->exp_err == TST_ERR) { >>>> + tst_res(TPASS | TTERRNO, >>>> + "copy_file_range failed as expected"); >>>> + } else { >>>> + /* copy_file_range() returned EISDIR when copyed contents to >>>> + * directory on new kernels, but on old kernels, it returned >>>> + * EBADF. >>>> + * >>>> + * the patch as follws: >>>> + * commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") >>>> + */ >>>> + if (tc->exp_err == EISDIR&& TST_ERR == EBADF) { >>>> tst_res(TPASS | TTERRNO, >>>> - "copy_file_range failed as expected"); >>>> - } else { >>>> - tst_res(TFAIL | TTERRNO, >>>> - "copy_file_range failed unexpectedly; expected %s, but got", >>>> - tst_strerrno(tc->exp_err)); >>>> + "copy_file_range failed as expected"); >>>> return; >>>> } >>>> - } else { >>>> - tst_res(TFAIL, >>>> - "copy_file_range returned wrong value: %ld", TST_RET); >>>> + >>>> + tst_res(TFAIL | TTERRNO, >>>> + "copy_file_range failed unexpectedly; expected %s, but got", >>>> + tst_strerrno(tc->exp_err)); >>>> } >>>> } >>>> >>> >>> >> >> -- >> Regards, >> Li Wang >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp
On Thu, Jun 27, 2019 at 11:03 AM Yang Xu <xuyang2018.jy@cn.fujitsu.com> wrote: > > on 2019/05/31 21:02, Amir Goldstein wrote: > > > On Fri, May 31, 2019 at 3:03 PM Li Wang<liwang@redhat.com> wrote: > >> > >> > >> On Fri, May 31, 2019 at 6:15 PM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: > >>> On 2019/05/31 16:44, Jinhui huang wrote: > >>>> On new kernels, copy_file_range() returned EISDIR when copyed contents > >>>> to directory, but on old kernels, it returned EBADF, we should accept > >>>> EBADF to be compatible with new and old kernels. > >>>> > >>>> The patch as follows: > >>>> commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") > >>> Hi, > >>> > >>> From description of commit, I wonder if we can add more tests for some > >>> non regular files(e.g. block, pipe)? > >> > >> I have no objection on this. But, is there really make sense to test some more non regular files which not being mentioned by Linux Manual Page? > >> > > FYI, more changes to copy_file_range checks are in the works: > > https://lore.kernel.org/linux-fsdevel/20190526061100.21761-1-amir73il@gmail.com/T/#me34d4363449118bd3b2ec8421d282a77e9a7d2d1 > > Hi Amir > > Meet again. We can increase ltp copy_file_range02 coverage include( swapfile ->ETXTBUSY, immutable file->EPERM) as same as xfstests generic/553[4]. > Also the two other checks(overlaping and offset wrap). I am glad to do it. That would be great! > > PS: Why we don't have test for overlaping and offset wrap check on xfstests? Or, I miss it? The bounds check test was posted to xfstests: https://marc.info/?l=linux-xfs&m=155947929219690&w=2 But because the test requires a new feature from xfs_io: https://marc.info/?l=linux-xfs&m=156152984109774&w=2 I recommended that xfstests maintainer will hold off merging the test, before the required change is at least in xfsprogs for-next branch. Thanks, Amir.
on 2019/06/27 16:39, Amir Goldstein wrote: > On Thu, Jun 27, 2019 at 11:03 AM Yang Xu<xuyang2018.jy@cn.fujitsu.com> wrote: >> on 2019/05/31 21:02, Amir Goldstein wrote: >> >>> On Fri, May 31, 2019 at 3:03 PM Li Wang<liwang@redhat.com> wrote: >>>> >>>> On Fri, May 31, 2019 at 6:15 PM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: >>>>> On 2019/05/31 16:44, Jinhui huang wrote: >>>>>> On new kernels, copy_file_range() returned EISDIR when copyed contents >>>>>> to directory, but on old kernels, it returned EBADF, we should accept >>>>>> EBADF to be compatible with new and old kernels. >>>>>> >>>>>> The patch as follows: >>>>>> commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") >>>>> Hi, >>>>> >>>>> From description of commit, I wonder if we can add more tests for some >>>>> non regular files(e.g. block, pipe)? >>>> I have no objection on this. But, is there really make sense to test some more non regular files which not being mentioned by Linux Manual Page? >>>> >>> FYI, more changes to copy_file_range checks are in the works: >>> https://lore.kernel.org/linux-fsdevel/20190526061100.21761-1-amir73il@gmail.com/T/#me34d4363449118bd3b2ec8421d282a77e9a7d2d1 >> Hi Amir >> >> Meet again. We can increase ltp copy_file_range02 coverage include( swapfile ->ETXTBUSY, immutable file->EPERM) as same as xfstests generic/553[4]. >> Also the two other checks(overlaping and offset wrap). I am glad to do it. > That would be great! Hi Amir I have tried it. swapfile and immutable file has no problem, but when I try to reproduce EINVAL(same file overlaping) without xfs_io, I got EFAULT error. It look likes we must depend on the new xfs_io feature patch. Right? ps: If it must xfs_io command, I think we can not check this situation because we should only check by copy_file_range syscall. what do you think about it? another question: I have seen copy_file_range manpage, it says fd_out data can be overwriting, but I got EFAULT when I use the following code. open(src_path, O_RDWR|O_CREAT, 0644) = fd_copy open(copy_path, O_RDWR|O_CREAT, 0644) = fd_src SAFE_WRITE(1, fd_src, CONTENT, CONTSIZE); SAFE_WRITE(1, fd_copy, CONTENT, CONTSIZE); copy_file_range(fd_src,0, fd_copy, CONTSIZE/4, CONTSIZE/2, 0) = -1 EFAULT fs/read_write.c copy_file_range function copy_from_user reports this error. If off_out or off_in isn't equal to 0, the error occurs. --------------------- ret= -EFAULT; .... if (off_out) { copy_from_user(&pos_out, off_out, sizeof(loff_t)); goto out; } ---------------------- Is it a bug or I miss something? Thanks Yang Xu > >> PS: Why we don't have test for overlaping and offset wrap check on xfstests? Or, I miss it? > The bounds check test was posted to xfstests: > https://marc.info/?l=linux-xfs&m=155947929219690&w=2 > > But because the test requires a new feature from xfs_io: > https://marc.info/?l=linux-xfs&m=156152984109774&w=2 > > I recommended that xfstests maintainer will hold off merging the test, > before the > required change is at least in xfsprogs for-next branch. > > Thanks, > Amir. > > >
On Thu, Jul 4, 2019 at 1:35 PM Yang Xu <xuyang2018.jy@cn.fujitsu.com> wrote: > > on 2019/06/27 16:39, Amir Goldstein wrote: > > On Thu, Jun 27, 2019 at 11:03 AM Yang Xu<xuyang2018.jy@cn.fujitsu.com> wrote: > >> on 2019/05/31 21:02, Amir Goldstein wrote: > >> > >>> On Fri, May 31, 2019 at 3:03 PM Li Wang<liwang@redhat.com> wrote: > >>>> > >>>> On Fri, May 31, 2019 at 6:15 PM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: > >>>>> On 2019/05/31 16:44, Jinhui huang wrote: > >>>>>> On new kernels, copy_file_range() returned EISDIR when copyed contents > >>>>>> to directory, but on old kernels, it returned EBADF, we should accept > >>>>>> EBADF to be compatible with new and old kernels. > >>>>>> > >>>>>> The patch as follows: > >>>>>> commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") > >>>>> Hi, > >>>>> > >>>>> From description of commit, I wonder if we can add more tests for some > >>>>> non regular files(e.g. block, pipe)? > >>>> I have no objection on this. But, is there really make sense to test some more non regular files which not being mentioned by Linux Manual Page? > >>>> > >>> FYI, more changes to copy_file_range checks are in the works: > >>> https://lore.kernel.org/linux-fsdevel/20190526061100.21761-1-amir73il@gmail.com/T/#me34d4363449118bd3b2ec8421d282a77e9a7d2d1 > >> Hi Amir > >> > >> Meet again. We can increase ltp copy_file_range02 coverage include( swapfile ->ETXTBUSY, immutable file->EPERM) as same as xfstests generic/553[4]. > >> Also the two other checks(overlaping and offset wrap). I am glad to do it. > > That would be great! > > Hi Amir > > I have tried it. swapfile and immutable file has no problem, but when I try to reproduce EINVAL(same file overlaping) without xfs_io, I got EFAULT error. > It look likes we must depend on the new xfs_io feature patch. Right? > > ps: If it must xfs_io command, I think we can not check this situation because we should only check by copy_file_range syscall. > what do you think about it? > I don't understand how xfs_io is related. LTP should use only copy_file_range() syscall. Do you have patches I can look at? > another question: > I have seen copy_file_range manpage, it says fd_out data can be overwriting, but I got EFAULT when I use the following code. > > open(src_path, O_RDWR|O_CREAT, 0644) = fd_copy > open(copy_path, O_RDWR|O_CREAT, 0644) = fd_src > SAFE_WRITE(1, fd_src, CONTENT, CONTSIZE); > SAFE_WRITE(1, fd_copy, CONTENT, CONTSIZE); > copy_file_range(fd_src,0, fd_copy, CONTSIZE/4, CONTSIZE/2, 0) = -1 EFAULT > > fs/read_write.c copy_file_range function copy_from_user reports this error. If off_out or off_in isn't equal to 0, the error occurs. > > --------------------- > ret= -EFAULT; > .... > if (off_out) { > copy_from_user(&pos_out, off_out, sizeof(loff_t)); > > goto out; > } > ---------------------- > > Is it a bug or I miss something? > You know off_out/off_out are either NULL or pointer to loff_t offset variable? If you have a sample code you may post it for review. Thanks, Amir.
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 07c0207..9e6356c 100644 --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c @@ -54,19 +54,32 @@ static void verify_copy_file_range(unsigned int n) TEST(sys_copy_file_range(fd_src, 0, *tc->copy_to_fd, 0, CONTSIZE, tc->flags)); - if (TST_RET == -1) { - if (tc->exp_err == TST_ERR) { + if (TST_RET != -1) { + tst_res(TFAIL, + "copy_file_range returned wrong value: %ld", TST_RET); + return; + } + + if (tc->exp_err == TST_ERR) { + tst_res(TPASS | TTERRNO, + "copy_file_range failed as expected"); + } else { + /* copy_file_range() returned EISDIR when copyed contents to + * directory on new kernels, but on old kernels, it returned + * EBADF. + * + * the patch as follws: + * commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") + */ + if (tc->exp_err == EISDIR && TST_ERR == EBADF) { tst_res(TPASS | TTERRNO, - "copy_file_range failed as expected"); - } else { - tst_res(TFAIL | TTERRNO, - "copy_file_range failed unexpectedly; expected %s, but got", - tst_strerrno(tc->exp_err)); + "copy_file_range failed as expected"); return; } - } else { - tst_res(TFAIL, - "copy_file_range returned wrong value: %ld", TST_RET); + + tst_res(TFAIL | TTERRNO, + "copy_file_range failed unexpectedly; expected %s, but got", + tst_strerrno(tc->exp_err)); } }
On new kernels, copy_file_range() returned EISDIR when copyed contents to directory, but on old kernels, it returned EBADF, we should accept EBADF to be compatible with new and old kernels. The patch as follows: commit 11cbfb10775a ("vfs: deny copy_file_range() for non regular files") Signed-off-by: Jinhui huang <huangjh.jy@cn.fujitsu.com> --- .../syscalls/copy_file_range/copy_file_range02.c | 33 +++++++++++++++------- 1 file changed, 23 insertions(+), 10 deletions(-)