* [PATCH] fsx: add more check for copy_file_range
@ 2019-09-25 7:16 Su Yanjun
2019-09-25 13:08 ` Eryu Guan
0 siblings, 1 reply; 4+ messages in thread
From: Su Yanjun @ 2019-09-25 7:16 UTC (permalink / raw)
To: guaneryu; +Cc: fstests, suyj.fnst
On some linux distros(RHEL7, centos 7) copy_file_range uses
general implementation (splice interface). splice interace
uses pipe_to_file. pipe_to_file only work for different page.
The userspace cant's be aware of such error because copy_file_range
returns ok too.
So for such case when copy_file_range return we read back data
then check it.
Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
---
ltp/fsx.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/ltp/fsx.c b/ltp/fsx.c
index 06d08e4..0439430 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -1602,6 +1602,7 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
size_t olen;
ssize_t nr;
int tries = 0;
+ int ret = 0;
if (length == 0) {
if (!quiet && testcalls > simulatedopcount)
@@ -1665,6 +1666,37 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
memset(good_buf + file_size, '\0', dest - file_size);
if (dest + length > file_size)
file_size = dest + length;
+ /*
+ * Although copy_file_range returns ok
+ * for some linux distros that use general implementation
+ * (splice interface) such as RHEL 7, centos 7 that use
+ * pipe_to_file will cause test fail.
+ *
+ * Here we add a little more check here for copy_file_range
+ * We read copied data then check it. If check fail here
+ * then report it.
+ */
+ ret = lseek(fd, (off_t)dest, SEEK_SET);
+ if (ret == (off_t)-1) {
+ prterr("doread: lseek");
+ report_failure(140);
+ }
+ ret = fsxread(fd, temp_buf, length, dest);
+ if (ret != length) {
+ if (ret == -1)
+ prterr("doread: read");
+ else
+ prt("short read: 0x%x bytes instead of 0x%x\n",
+ ret, length);
+ report_failure(141);
+ }
+ if (memcmp(good_buf+dest, temp_buf, length) != 0) {
+ prt("copy range: 0x%x to 0x%x at 0x%x\n", offset,
+ offset + length, dest);
+ prterr("do_copy_range:");
+ report_failure(161);
+
+ }
}
#else
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] fsx: add more check for copy_file_range
2019-09-25 7:16 [PATCH] fsx: add more check for copy_file_range Su Yanjun
@ 2019-09-25 13:08 ` Eryu Guan
2019-09-27 1:38 ` Su Yanjun
0 siblings, 1 reply; 4+ messages in thread
From: Eryu Guan @ 2019-09-25 13:08 UTC (permalink / raw)
To: Su Yanjun; +Cc: fstests
On Wed, Sep 25, 2019 at 03:16:55PM +0800, Su Yanjun wrote:
> On some linux distros(RHEL7, centos 7) copy_file_range uses
> general implementation (splice interface). splice interace
> uses pipe_to_file. pipe_to_file only work for different page.
> The userspace cant's be aware of such error because copy_file_range
> returns ok too.
This looks like a kernel bug to me.
> So for such case when copy_file_range return we read back data
> then check it.
Yeah, I think we should make sure the data copied is correct.
>
> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
> ---
> ltp/fsx.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 06d08e4..0439430 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -1602,6 +1602,7 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
> size_t olen;
> ssize_t nr;
> int tries = 0;
> + int ret = 0;
>
> if (length == 0) {
> if (!quiet && testcalls > simulatedopcount)
> @@ -1665,6 +1666,37 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
> memset(good_buf + file_size, '\0', dest - file_size);
> if (dest + length > file_size)
> file_size = dest + length;
> + /*
> + * Although copy_file_range returns ok
> + * for some linux distros that use general implementation
> + * (splice interface) such as RHEL 7, centos 7 that use
> + * pipe_to_file will cause test fail.
> + *
> + * Here we add a little more check here for copy_file_range
> + * We read copied data then check it. If check fail here
> + * then report it.
> + */
Above comments seem like not necessary, the content check is inspired by
Rthis HEL7 issue, but not the main purpose.
> + ret = lseek(fd, (off_t)dest, SEEK_SET);
> + if (ret == (off_t)-1) {
> + prterr("doread: lseek");
^^^^^^^ not doread
> + report_failure(140);
Use a different failure number than that in doread, i.e. don't copy the
code from doread.
> + }
> + ret = fsxread(fd, temp_buf, length, dest);
> + if (ret != length) {
> + if (ret == -1)
> + prterr("doread: read");
> + else
> + prt("short read: 0x%x bytes instead of 0x%x\n",
> + ret, length);
Same here, not doread, and weired indention above.
> + report_failure(141);
> + }
> + if (memcmp(good_buf+dest, temp_buf, length) != 0) {
> + prt("copy range: 0x%x to 0x%x at 0x%x\n", offset,
> + offset + length, dest);
> + prterr("do_copy_range:");
> + report_failure(161);
I think we could take use of check_buffers().
And similar check could be added to do_clone_range as well, I think.
Thanks,
Eryu
> +
> + }
> }
>
> #else
> --
> 2.7.4
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fsx: add more check for copy_file_range
2019-09-25 13:08 ` Eryu Guan
@ 2019-09-27 1:38 ` Su Yanjun
2019-09-27 1:38 ` Su Yanjun
0 siblings, 1 reply; 4+ messages in thread
From: Su Yanjun @ 2019-09-27 1:38 UTC (permalink / raw)
To: Eryu Guan; +Cc: fstests
在 2019/9/25 21:08, Eryu Guan 写道:
> On Wed, Sep 25, 2019 at 03:16:55PM +0800, Su Yanjun wrote:
>> On some linux distros(RHEL7, centos 7) copy_file_range uses
>> general implementation (splice interface). splice interace
>> uses pipe_to_file. pipe_to_file only work for different page.
>> The userspace cant's be aware of such error because copy_file_range
>> returns ok too.
> This looks like a kernel bug to me.
Yes, it's RHEL kernel 3.10's bug. The new kernel is ok.
>> So for such case when copy_file_range return we read back data
>> then check it.
> Yeah, I think we should make sure the data copied is correct.
>
>> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
>> ---
>> ltp/fsx.c | 32 ++++++++++++++++++++++++++++++++
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/ltp/fsx.c b/ltp/fsx.c
>> index 06d08e4..0439430 100644
>> --- a/ltp/fsx.c
>> +++ b/ltp/fsx.c
>> @@ -1602,6 +1602,7 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
>> size_t olen;
>> ssize_t nr;
>> int tries = 0;
>> + int ret = 0;
>>
>> if (length == 0) {
>> if (!quiet && testcalls > simulatedopcount)
>> @@ -1665,6 +1666,37 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
>> memset(good_buf + file_size, '\0', dest - file_size);
>> if (dest + length > file_size)
>> file_size = dest + length;
>> + /*
>> + * Although copy_file_range returns ok
>> + * for some linux distros that use general implementation
>> + * (splice interface) such as RHEL 7, centos 7 that use
>> + * pipe_to_file will cause test fail.
>> + *
>> + * Here we add a little more check here for copy_file_range
>> + * We read copied data then check it. If check fail here
>> + * then report it.
>> + */
> Above comments seem like not necessary, the content check is inspired by
> Rthis HEL7 issue, but not the main purpose.
>
>> + ret = lseek(fd, (off_t)dest, SEEK_SET);
>> + if (ret == (off_t)-1) {
>> + prterr("doread: lseek");
> ^^^^^^^ not doread
>
>> + report_failure(140);
> Use a different failure number than that in doread, i.e. don't copy the
> code from doread.
>
>> + }
>> + ret = fsxread(fd, temp_buf, length, dest);
>> + if (ret != length) {
>> + if (ret == -1)
>> + prterr("doread: read");
>> + else
>> + prt("short read: 0x%x bytes instead of 0x%x\n",
>> + ret, length);
> Same here, not doread, and weired indention above.
>
>> + report_failure(141);
>> + }
>> + if (memcmp(good_buf+dest, temp_buf, length) != 0) {
>> + prt("copy range: 0x%x to 0x%x at 0x%x\n", offset,
>> + offset + length, dest);
>> + prterr("do_copy_range:");
>> + report_failure(161);
> I think we could take use of check_buffers().
>
> And similar check could be added to do_clone_range as well, I think.
Ok, i'll send patch v2 later.
Thanks
>
> Thanks,
> Eryu
>
>> +
>> + }
>> }
>>
>> #else
>> --
>> 2.7.4
>>
>>
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fsx: add more check for copy_file_range
2019-09-27 1:38 ` Su Yanjun
@ 2019-09-27 1:38 ` Su Yanjun
0 siblings, 0 replies; 4+ messages in thread
From: Su Yanjun @ 2019-09-27 1:38 UTC (permalink / raw)
To: Eryu Guan; +Cc: fstests
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8"; format="flowed", Size: 2905 bytes --]
ÔÚ 2019/9/25 21:08, Eryu Guan дµÀ:
> On Wed, Sep 25, 2019 at 03:16:55PM +0800, Su Yanjun wrote:
>> On some linux distros(RHEL7, centos 7) copy_file_range uses
>> general implementation (splice interface). splice interace
>> uses pipe_to_file. pipe_to_file only work for different page.
>> The userspace cant's be aware of such error because copy_file_range
>> returns ok too.
> This looks like a kernel bug to me.
Yes, it's RHEL kernel 3.10's bug. The new kernel is ok.
>> So for such case when copy_file_range return we read back data
>> then check it.
> Yeah, I think we should make sure the data copied is correct.
>
>> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
>> ---
>> ltp/fsx.c | 32 ++++++++++++++++++++++++++++++++
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/ltp/fsx.c b/ltp/fsx.c
>> index 06d08e4..0439430 100644
>> --- a/ltp/fsx.c
>> +++ b/ltp/fsx.c
>> @@ -1602,6 +1602,7 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
>> size_t olen;
>> ssize_t nr;
>> int tries = 0;
>> + int ret = 0;
>>
>> if (length == 0) {
>> if (!quiet && testcalls > simulatedopcount)
>> @@ -1665,6 +1666,37 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
>> memset(good_buf + file_size, '\0', dest - file_size);
>> if (dest + length > file_size)
>> file_size = dest + length;
>> + /*
>> + * Although copy_file_range returns ok
>> + * for some linux distros that use general implementation
>> + * (splice interface) such as RHEL 7, centos 7 that use
>> + * pipe_to_file will cause test fail.
>> + *
>> + * Here we add a little more check here for copy_file_range
>> + * We read copied data then check it. If check fail here
>> + * then report it.
>> + */
> Above comments seem like not necessary, the content check is inspired by
> Rthis HEL7 issue, but not the main purpose.
>
>> + ret = lseek(fd, (off_t)dest, SEEK_SET);
>> + if (ret == (off_t)-1) {
>> + prterr("doread: lseek");
> ^^^^^^^ not doread
>
>> + report_failure(140);
> Use a different failure number than that in doread, i.e. don't copy the
> code from doread.
>
>> + }
>> + ret = fsxread(fd, temp_buf, length, dest);
>> + if (ret != length) {
>> + if (ret == -1)
>> + prterr("doread: read");
>> + else
>> + prt("short read: 0x%x bytes instead of 0x%x\n",
>> + ret, length);
> Same here, not doread, and weired indention above.
>
>> + report_failure(141);
>> + }
>> + if (memcmp(good_buf+dest, temp_buf, length) != 0) {
>> + prt("copy range: 0x%x to 0x%x at 0x%x\n", offset,
>> + offset + length, dest);
>> + prterr("do_copy_range:");
>> + report_failure(161);
> I think we could take use of check_buffers().
>
> And similar check could be added to do_clone_range as well, I think.
Ok, i'll send patch v2 later.
Thanks
>
> Thanks,
> Eryu
>
>> +
>> + }
>> }
>>
>> #else
>> --
>> 2.7.4
>>
>>
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-09-27 1:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-25 7:16 [PATCH] fsx: add more check for copy_file_range Su Yanjun
2019-09-25 13:08 ` Eryu Guan
2019-09-27 1:38 ` Su Yanjun
2019-09-27 1:38 ` Su Yanjun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).