[LTP] [PATCH v1] read_all :catch alignment faults while reading sys entries seen in commit :bc21785b7336619fb6a67f1fff5afdaf229acc

[LTP] [PATCH v1] read_all :catch alignment faults while reading sys entries seen in commit :bc21785b7336619fb6a67f1fff5afdaf229acc

[Subramanya Swamy]

Signed-off-by: Subramanya Swamy <subramanya.swamy.linux@gmail.com>
---
 testcases/kernel/fs/read_all/read_all.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
index 266678ea7..95e3ca275 100644
--- a/testcases/kernel/fs/read_all/read_all.c
+++ b/testcases/kernel/fs/read_all/read_all.c
@@ -249,6 +249,20 @@ static void read_test(const int worker, const char *const path)
 	}
 
 	worker_heartbeat(worker);
+	/*
+	 * This could catch any alignment faults while reading sys entries
+	 * seen in commit :bc21785b7336619fb6a67f1fff5afdaf229acc so reading 1024 bytes
+	 * in chunks of 8 bytes 128 times
+	 */
+	char check_buf[7];
+	unsigned int i;
+
+	for (i = 0; i < 128; i++) {
+		count = read(fd, check_buf, sizeof(check_buf));
+		if (count == 0 || count < 0)
+			break;
+	}
+
 	count = read(fd, buf, sizeof(buf) - 1);
 	elapsed = worker_elapsed(worker);
 
@@ -713,5 +727,5 @@ static struct tst_test test = {
 	.cleanup = cleanup,
 	.test_all = run,
 	.forks_child = 1,
-	.max_runtime = 100,
+	.max_runtime = 200,
 };
-- 
2.39.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] read_all :catch alignment faults while reading sys entries seen in commit :bc21785b7336619fb6a67f1fff5afdaf229acc

[Cyril Hrubis]

Hi!
> +	/*
> +	 * This could catch any alignment faults while reading sys entries
> +	 * seen in commit :bc21785b7336619fb6a67f1fff5afdaf229acc so reading 1024 bytes
                                 ^
				 This does not seem to match any kernel upstream commit.
> +	 * in chunks of 8 bytes 128 times
> +	 */
> +	char check_buf[7];
                       ^
		       This isn't 8 bytes at all as it's written in
		       description.
> +	unsigned int i;
> +
> +	for (i = 0; i < 128; i++) {
> +		count = read(fd, check_buf, sizeof(check_buf));
> +		if (count == 0 || count < 0)
> +			break;
> +	}

So the intention is to read the buffer in smaller chunks? I guess that
it's hard to tell without having seen the kernel bugfix.

>  	count = read(fd, buf, sizeof(buf) - 1);

I wonder should we seek back in the fd, or do pread() with zero offset here?

>  	elapsed = worker_elapsed(worker);
>  
> @@ -713,5 +727,5 @@ static struct tst_test test = {
>  	.cleanup = cleanup,
>  	.test_all = run,
>  	.forks_child = 1,
> -	.max_runtime = 100,
> +	.max_runtime = 200,
>  };
> -- 
> 2.39.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] read_all :catch alignment faults while reading sys entries seen in commit :bc21785b7336619fb6a67f1fff5afdaf229acc

[Subramanya]

Hi,

On 03/01/24 18:19, Cyril Hrubis wrote:
> Hi!
>> +	/*
>> +	 * This could catch any alignment faults while reading sys entries
>> +	 * seen in commit :bc21785b7336619fb6a67f1fff5afdaf229acc so reading 1024 bytes
>                                   ^
> 				 This does not seem to match any kernel upstream commit.
>> +	 * in chunks of 8 bytes 128 times
>> +	 */
>> +	char check_buf[7];
>                         ^
> 		       This isn't 8 bytes at all as it's written in
> 		       description.
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < 128; i++) {
>> +		count = read(fd, check_buf, sizeof(check_buf));
>> +		if (count == 0 || count < 0)
>> +			break;
>> +	}
> So the intention is to read the buffer in smaller chunks? I guess that
> it's hard to tell without having seen the kernel bugfix.

My bad i provided the wrong commit id in the commit message for the 
kernel bugfix, 
https://github.com/torvalds/linux/commit/1bbc21785b7336619fb6a67f1fff5afdaf229acc 


>>   	count = read(fd, buf, sizeof(buf) - 1);
> I wonder should we seek back in the fd, or do pread() with zero offset here?

yes you're right since we are using open instead of fopen pread() with 
offset zero should be used else the buffer would not print data read_all 
in case the test is run with

-v option

>
>>   	elapsed = worker_elapsed(worker);
>>   
>> @@ -713,5 +727,5 @@ static struct tst_test test = {
>>   	.cleanup = cleanup,
>>   	.test_all = run,
>>   	.forks_child = 1,
>> -	.max_runtime = 100,
>> +	.max_runtime = 200,
>>   };
>> -- 
>> 2.39.3
>>
>>
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp

Best Regards,

Subramanya


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] read_all :catch alignment faults while reading sys entries seen in commit :bc21785b7336619fb6a67f1fff5afdaf229acc

[Cyril Hrubis]

Hi!
> > So the intention is to read the buffer in smaller chunks? I guess that
> > it's hard to tell without having seen the kernel bugfix.
> 
> My bad i provided the wrong commit id in the commit message for the 
> kernel bugfix, 
> https://github.com/torvalds/linux/commit/1bbc21785b7336619fb6a67f1fff5afdaf229acc 

So it looks like the problem happens when we attempt to read the memory
with unaligned offset, so I suppose that single:

pread(fd, buf, 16, 1);

Should trigger the problem, or do I miss something?

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] read_all :catch alignment faults while reading sys entries seen in commit :bc21785b7336619fb6a67f1fff5afdaf229acc

[Subramanya Swamy]

Hi,

On 03/01/24 20:43, Cyril Hrubis wrote:
> Hi!
>>> So the intention is to read the buffer in smaller chunks? I guess that
>>> it's hard to tell without having seen the kernel bugfix.
>> My bad i provided the wrong commit id in the commit message for the
>> kernel bugfix,
>> https://github.com/torvalds/linux/commit/1bbc21785b7336619fb6a67f1fff5afdaf229acc  
> So it looks like the problem happens when we attempt to read the memory
> with unaligned offset, so I suppose that single:
>
> pread(fd, buf, 16, 1);
>
> Should trigger the problem, or do I miss something?

without the patch 
https://github.com/torvalds/linux/commit/1bbc21785b7336619fb6a67f1fff5afdaf229acc

the issue is not reproducible with pread(fd,buf,16,1) , tried with odd 
length of 15 to be read from file with offset 1 and it's reproducible

The reproducer for this issue is this dd command from patch 
https://lore.kernel.org/linux-arm-kernel/20220304193107.5ljii5h4kmkwpl3f@redhat.com/ 


dd if=/sys/firmware/acpi/tables/data/BERT of=/dev/null bs=7

& this works to reproduce the issue

pread(fd, buf, 7, 1);


Best Regards

Subramanya





-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3] read_all :catch alignment faults while reading sys entries seen in commit :1bbc21785b7336619fb6a67f1fff5afdaf229acc

[Subramanya Swamy]

Signed-off-by: Subramanya Swamy <subramanya.swamy.linux@gmail.com>
---
 testcases/kernel/fs/read_all/read_all.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
index ddc48edd8..e87f47979 100644
--- a/testcases/kernel/fs/read_all/read_all.c
+++ b/testcases/kernel/fs/read_all/read_all.c
@@ -251,17 +251,11 @@ static void read_test(const int worker, const char *const path)
 	worker_heartbeat(worker);
 	/*
 	 * This could catch any alignment faults while reading sys entries
-	 * seen in commit :1bbc21785b7336619fb6a67f1fff5afdaf229acc so reading 1024 bytes
-	 * in chunks of 8 bytes 128 times
+	 * seen in commit :1bbc21785b7336619fb6a67f1fff5afdaf229acc
 	 */
 	char check_buf[7];
-	unsigned int i;
 
-	for (i = 0; i < 128; i++) {
-		count = read(fd, check_buf, sizeof(check_buf));
-		if (count == 0 || count < 0)
-			break;
-	}
+	count = pread(fd, check_buf, sizeof(check_buf), 1);
 
 	count = pread(fd, buf, sizeof(buf) - 1, 0);
 	elapsed = worker_elapsed(worker);
-- 
2.39.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3] read_all :catch alignment faults while reading sys entries seen in commit :1bbc21785b7336619fb6a67f1fff5afdaf229acc

[Cyril Hrubis]

Hi!
> ---
>  testcases/kernel/fs/read_all/read_all.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
> index ddc48edd8..e87f47979 100644
> --- a/testcases/kernel/fs/read_all/read_all.c
> +++ b/testcases/kernel/fs/read_all/read_all.c
> @@ -251,17 +251,11 @@ static void read_test(const int worker, const char *const path)
>  	worker_heartbeat(worker);
>  	/*
>  	 * This could catch any alignment faults while reading sys entries
> -	 * seen in commit :1bbc21785b7336619fb6a67f1fff5afdaf229acc so reading 1024 bytes
> -	 * in chunks of 8 bytes 128 times
> +	 * seen in commit :1bbc21785b7336619fb6a67f1fff5afdaf229acc

The commit id should go into tags.

>  	 */
>  	char check_buf[7];
> -	unsigned int i;
>  
> -	for (i = 0; i < 128; i++) {
> -		count = read(fd, check_buf, sizeof(check_buf));
> -		if (count == 0 || count < 0)
> -			break;
> -	}
> +	count = pread(fd, check_buf, sizeof(check_buf), 1);
>  
>  	count = pread(fd, buf, sizeof(buf) - 1, 0);
>  	elapsed = worker_elapsed(worker);

This is a patch on a top of a patch that does not apply...


What about this change? That should enough to trigger the problem:

diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
index 266678ea7..86fc6fb61 100644
--- a/testcases/kernel/fs/read_all/read_all.c
+++ b/testcases/kernel/fs/read_all/read_all.c
@@ -228,6 +228,7 @@ static int worker_ttl(const int worker)
 static void read_test(const int worker, const char *const path)
 {
        char buf[BUFFER_SIZE];
+       char odd_buff[7];
        int fd;
        ssize_t count;
        const pid_t pid = workers[worker].pid;
@@ -250,6 +251,8 @@ static void read_test(const int worker, const char *const path)
 
        worker_heartbeat(worker);
        count = read(fd, buf, sizeof(buf) - 1);
+       /* read at odd offset triggers bug fixed by 1bbc21785b73 */
+       pread(fd, odd_buf, sizeof(odd_buf), 1);
        elapsed = worker_elapsed(worker);
 
        if (count > 0 && verbose) {
@@ -714,4 +717,8 @@ static struct tst_test test = {
        .test_all = run,
        .forks_child = 1,
        .max_runtime = 100,
+       .tags = (const struct tst_tag[]) {
+               {"linux-git", "1bbc21785b73"},
+               {}
+       }
 };


-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp