FSTests Archive on lore.kernel.org
 help / color / Atom feed
* [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	[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 --]
[-- Attachment #0: 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, back to index

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

FSTests Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/fstests/0 fstests/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 fstests fstests/ https://lore.kernel.org/fstests \
		fstests@vger.kernel.org linux-fstests@archiver.kernel.org
	public-inbox-index fstests

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.fstests


AGPL code for this site: git clone https://public-inbox.org/ public-inbox