[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Jinhui huang]

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(-)

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

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Li Wang]

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
>


-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190531/9c287c61/attachment.html>

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Xiao Yang]

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));
>  	}
>  }
>  

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Li Wang]

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));
> >       }
> >  }
> >
>
>
>
>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190531/e62514d2/attachment.html>

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Li Wang]

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.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190531/9c601a43/attachment.html>

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Amir Goldstein]

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

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Yang Xu]

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

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Amir Goldstein]

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.

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Yang Xu]

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.
>
>
>

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Amir Goldstein]

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.

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Li Wang]

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
>


-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190531/61205c12/attachment-0001.html>

[LTP] [PATCH] syscalls/copy_file_range02.c: Compatible with new and old kernels

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz