| Message ID | 20240103121726.1854-1-subramanya.swamy.linux@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] read_all :catch alignment faults while reading sys entries seen in commit :bc21785b7336619fb6a67f1fff5afdaf229acc | expand |
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
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
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?
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
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, };
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(-)