This series updates the brk LTP tests to use direct syscalls which makes the compat tests pass on our musl-based distribution.
Add a simple sanity check for purecap that checks for ENOSYS and fails otherwise. Deactivate brk02 in purecap as it cannot be completed.
checkpatch complains about using ENOSYS but brk should not be implemented in purecap, so this is expected.
Branch for review : review/teo/brk-fixes-v3 https://git.morello-project.org/Teo-CD/morello-linux-ltp/-/commits/review/te... --- v2: Better descriptions, switched to direct syscall and not checking for EINTR. v3: Fix tst_syscall return type and use it again for brk.
Teo Couprie Diaz (2): syscalls/brk: use direct syscall syscalls/brk: add a sanity check for purecap
runtest/morello_transitional_extended | 3 +++ testcases/kernel/syscalls/brk/brk01.c | 19 +++++++++++++------ testcases/kernel/syscalls/brk/brk02.c | 14 ++++++-------- 3 files changed, 22 insertions(+), 14 deletions(-)
base-commit: 75fda47a48d1b6f1c6671fa9819ca9fd9169cf2a
Direct usage of brk is discouraged in favor of using malloc. Also, brk was removed from POSIX in POSIX.1-2001. In particular, the musl libc's brk always returns -ENOMEM which causes the LTP tests to fail. Invoking the syscall directly allows them to pass.
Instead, use tst_syscall() and handle the failure cases ourselves, as we don't depend on the libc to do it anymore.
The patch also removes the dependency on sbrk to get the current break as it has the same issues. Instead, call tst_syscall(__NR_brk, 0) which should always fail, returning the current break.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com --- I believe this patch could be upstreamed, maybe at first as an RFC to gather interest, as it would fix the tests for Musl distributions.
testcases/kernel/syscalls/brk/brk01.c | 9 +++------ testcases/kernel/syscalls/brk/brk02.c | 14 ++++++-------- 2 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..919435755 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,6 +9,7 @@ #include <errno.h>
#include "tst_test.h" +#include "lapi/syscalls.h"
void verify_brk(void) { @@ -16,7 +17,7 @@ void verify_brk(void) size_t inc = getpagesize() * 2 - 1; unsigned int i;
- cur_brk = (uintptr_t)sbrk(0); + cur_brk = (uintptr_t)tst_syscall(__NR_brk, 0);
for (i = 0; i < 33; i++) { switch (i % 3) { @@ -31,11 +32,7 @@ void verify_brk(void) break; }
- TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()"); - if (!TST_PASS) - return; - - cur_brk = (uintptr_t)sbrk(0); + cur_brk = tst_syscall(__NR_brk, new_brk);
if (cur_brk != new_brk) { tst_res(TFAIL, diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c index 11e803cb4..dabda30c2 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -14,24 +14,22 @@ #include <unistd.h> #include <sys/mman.h> #include "tst_test.h" +#include "lapi/syscalls.h"
void brk_down_vmas(void) { - void *brk_addr = sbrk(0); - - if (brk_addr == (void *) -1) - tst_brk(TBROK, "sbrk() failed"); + void *brk_addr = (void *)tst_syscall(__NR_brk, 0);
unsigned long page_size = getpagesize(); void *addr = brk_addr + page_size;
- if (brk(addr)) { + if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size"); return; }
addr += page_size; - if (brk(addr)) { + if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size"); return; } @@ -42,12 +40,12 @@ void brk_down_vmas(void) }
addr += page_size; - if (brk(addr)) { + if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect"); return; }
- if (brk(brk_addr)) { + if ((void *)tst_syscall(__NR_brk, brk_addr) != brk_addr) { tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address"); return; }
Hi Teo,
On 15-11-2022 17:01, Teo Couprie Diaz wrote:
Direct usage of brk is discouraged in favor of using malloc. Also, brk was removed from POSIX in POSIX.1-2001. In particular, the musl libc's brk always returns -ENOMEM which causes the LTP tests to fail. Invoking the syscall directly allows them to pass.
Instead, use tst_syscall() and handle the failure cases ourselves, as we don't depend on the libc to do it anymore.
The patch also removes the dependency on sbrk to get the current break as it has the same issues. Instead, call tst_syscall(__NR_brk, 0) which should always fail, returning the current break.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
I believe this patch could be upstreamed, maybe at first as an RFC to gather interest, as it would fix the tests for Musl distributions.
testcases/kernel/syscalls/brk/brk01.c | 9 +++------ testcases/kernel/syscalls/brk/brk02.c | 14 ++++++-------- 2 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..919435755 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,6 +9,7 @@ #include <errno.h> #include "tst_test.h" +#include "lapi/syscalls.h" void verify_brk(void) { @@ -16,7 +17,7 @@ void verify_brk(void) size_t inc = getpagesize() * 2 - 1; unsigned int i;
- cur_brk = (uintptr_t)sbrk(0);
- cur_brk = (uintptr_t)tst_syscall(__NR_brk, 0);
for (i = 0; i < 33; i++) { switch (i % 3) { @@ -31,11 +32,7 @@ void verify_brk(void) break; }
TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()");if (!TST_PASS)return;cur_brk = (uintptr_t)sbrk(0);
cur_brk = tst_syscall(__NR_brk, new_brk);
nit: Should you have a cast to uintptr_t here as well? Or even better, make cur_brk and new_brk to be void* as in brk02.c? Up to you.
Also this patch needs e5d2a05a90e5 ("regen.sh: Use intptr_t for tst_syscall return") to be cherry-picked from upstream, right? Maybe add a note to it in the cover letter? Otherwise than that, I have no further comments on this series. Well done!
Thanks, Tudor
if (cur_brk != new_brk) { tst_res(TFAIL, diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c index 11e803cb4..dabda30c2 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -14,24 +14,22 @@ #include <unistd.h> #include <sys/mman.h> #include "tst_test.h" +#include "lapi/syscalls.h" void brk_down_vmas(void) {
- void *brk_addr = sbrk(0);
- if (brk_addr == (void *) -1)
tst_brk(TBROK, "sbrk() failed");
- void *brk_addr = (void *)tst_syscall(__NR_brk, 0);
unsigned long page_size = getpagesize(); void *addr = brk_addr + page_size;
- if (brk(addr)) {
- if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size"); return; }
addr += page_size;
- if (brk(addr)) {
- if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size"); return; }
@@ -42,12 +40,12 @@ void brk_down_vmas(void) } addr += page_size;
- if (brk(addr)) {
- if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect"); return; }
- if (brk(brk_addr)) {
- if ((void *)tst_syscall(__NR_brk, brk_addr) != brk_addr) { tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address"); return; }
On 16/11/2022 14:07, Tudor Cretu wrote:
Hi Teo,
On 15-11-2022 17:01, Teo Couprie Diaz wrote:
Direct usage of brk is discouraged in favor of using malloc. Also, brk was removed from POSIX in POSIX.1-2001. In particular, the musl libc's brk always returns -ENOMEM which causes the LTP tests to fail. Invoking the syscall directly allows them to pass.
Instead, use tst_syscall() and handle the failure cases ourselves, as we don't depend on the libc to do it anymore.
The patch also removes the dependency on sbrk to get the current break as it has the same issues. Instead, call tst_syscall(__NR_brk, 0) which should always fail, returning the current break.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
I believe this patch could be upstreamed, maybe at first as an RFC to gather interest, as it would fix the tests for Musl distributions.
testcases/kernel/syscalls/brk/brk01.c | 9 +++------ testcases/kernel/syscalls/brk/brk02.c | 14 ++++++-------- 2 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..919435755 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,6 +9,7 @@ #include <errno.h> #include "tst_test.h" +#include "lapi/syscalls.h" void verify_brk(void) { @@ -16,7 +17,7 @@ void verify_brk(void) size_t inc = getpagesize() * 2 - 1; unsigned int i; - cur_brk = (uintptr_t)sbrk(0); + cur_brk = (uintptr_t)tst_syscall(__NR_brk, 0); for (i = 0; i < 33; i++) { switch (i % 3) { @@ -31,11 +32,7 @@ void verify_brk(void) break; } - TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()"); - if (!TST_PASS) - return;
- cur_brk = (uintptr_t)sbrk(0); + cur_brk = tst_syscall(__NR_brk, new_brk);
nit: Should you have a cast to uintptr_t here as well? Or even better, make cur_brk and new_brk to be void* as in brk02.c? Up to you.
You're right, it would be better with the cast. I don't really know which of void* or uintptr_t would be better to make the two tests use the same type to be honest, I don't mind either way.
Also this patch needs e5d2a05a90e5 ("regen.sh: Use intptr_t for tst_syscall return") to be cherry-picked from upstream, right? Maybe add a note to it in the cover letter?
It does, however Beata already cherry-picked it a few days ago which is why I didn't make a not of it :) https://git.morello-project.org/morello/morello-linux-ltp/-/commits/morello/...
Otherwise than that, I have no further comments on this series. Well done!
Thanks, Tudor
Thanks for having a look ! Téo
if (cur_brk != new_brk) { tst_res(TFAIL, diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c index 11e803cb4..dabda30c2 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -14,24 +14,22 @@ #include <unistd.h> #include <sys/mman.h> #include "tst_test.h" +#include "lapi/syscalls.h" void brk_down_vmas(void) { - void *brk_addr = sbrk(0);
- if (brk_addr == (void *) -1) - tst_brk(TBROK, "sbrk() failed"); + void *brk_addr = (void *)tst_syscall(__NR_brk, 0); unsigned long page_size = getpagesize(); void *addr = brk_addr + page_size; - if (brk(addr)) { + if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size"); return; } addr += page_size; - if (brk(addr)) { + if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size"); return; } @@ -42,12 +40,12 @@ void brk_down_vmas(void) } addr += page_size; - if (brk(addr)) { + if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect"); return; } - if (brk(brk_addr)) { + if ((void *)tst_syscall(__NR_brk, brk_addr) != brk_addr) { tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address"); return; }
On 17-11-2022 10:38, Teo Couprie Diaz wrote:
On 16/11/2022 14:07, Tudor Cretu wrote:
Hi Teo,
On 15-11-2022 17:01, Teo Couprie Diaz wrote:
Direct usage of brk is discouraged in favor of using malloc. Also, brk was removed from POSIX in POSIX.1-2001. In particular, the musl libc's brk always returns -ENOMEM which causes the LTP tests to fail. Invoking the syscall directly allows them to pass.
Instead, use tst_syscall() and handle the failure cases ourselves, as we don't depend on the libc to do it anymore.
The patch also removes the dependency on sbrk to get the current break as it has the same issues. Instead, call tst_syscall(__NR_brk, 0) which should always fail, returning the current break.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
I believe this patch could be upstreamed, maybe at first as an RFC to gather interest, as it would fix the tests for Musl distributions.
testcases/kernel/syscalls/brk/brk01.c | 9 +++------ testcases/kernel/syscalls/brk/brk02.c | 14 ++++++-------- 2 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..919435755 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,6 +9,7 @@ #include <errno.h> #include "tst_test.h" +#include "lapi/syscalls.h" void verify_brk(void) { @@ -16,7 +17,7 @@ void verify_brk(void) size_t inc = getpagesize() * 2 - 1; unsigned int i; - cur_brk = (uintptr_t)sbrk(0); + cur_brk = (uintptr_t)tst_syscall(__NR_brk, 0); for (i = 0; i < 33; i++) { switch (i % 3) { @@ -31,11 +32,7 @@ void verify_brk(void) break; } - TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()"); - if (!TST_PASS) - return;
- cur_brk = (uintptr_t)sbrk(0); + cur_brk = tst_syscall(__NR_brk, new_brk);
nit: Should you have a cast to uintptr_t here as well? Or even better, make cur_brk and new_brk to be void* as in brk02.c? Up to you.
You're right, it would be better with the cast. I don't really know which of void* or uintptr_t would be better to make the two tests use the same type to be honest, I don't mind either way.
Eh, arithmetic on a void* is a GNU extension, yes, but it's fairly common throughout the project from what I see, so I wouldn't think too much about it. Up to you if you want to further nitpick it, from my point of view it looks great as-is as well.
Also this patch needs e5d2a05a90e5 ("regen.sh: Use intptr_t for tst_syscall return") to be cherry-picked from upstream, right? Maybe add a note to it in the cover letter?
It does, however Beata already cherry-picked it a few days ago which is why I didn't make a not of it :) https://git.morello-project.org/morello/morello-linux-ltp/-/commits/morello/...
Awesome!
Thanks, Tudor
Otherwise than that, I have no further comments on this series. Well done!
Thanks, Tudor
Thanks for having a look ! Téo
if (cur_brk != new_brk) { tst_res(TFAIL, diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c index 11e803cb4..dabda30c2 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -14,24 +14,22 @@ #include <unistd.h> #include <sys/mman.h> #include "tst_test.h" +#include "lapi/syscalls.h" void brk_down_vmas(void) { - void *brk_addr = sbrk(0);
- if (brk_addr == (void *) -1) - tst_brk(TBROK, "sbrk() failed"); + void *brk_addr = (void *)tst_syscall(__NR_brk, 0); unsigned long page_size = getpagesize(); void *addr = brk_addr + page_size; - if (brk(addr)) { + if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size"); return; } addr += page_size; - if (brk(addr)) { + if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size"); return; } @@ -42,12 +40,12 @@ void brk_down_vmas(void) } addr += page_size; - if (brk(addr)) { + if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect"); return; } - if (brk(brk_addr)) { + if ((void *)tst_syscall(__NR_brk, brk_addr) != brk_addr) { tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address"); return; }
Hi Teo,
On Tue, Nov 15, 2022 at 05:01:06PM +0000, Teo Couprie Diaz wrote:
Direct usage of brk is discouraged in favor of using malloc. Also, brk was removed from POSIX in POSIX.1-2001. In particular, the musl libc's brk always returns -ENOMEM which causes the LTP tests to fail. Invoking the syscall directly allows them to pass.
How about: ... which causes the LTP tests to fail prematurely. Invoking the syscall directly allows them to carry out their validation of brk behaviour. ?
Instead, use tst_syscall() and handle the failure cases ourselves, as we don't depend on the libc to do it anymore.
The patch also removes the dependency on sbrk to get the current break as it has the same issues. Instead, call tst_syscall(__NR_brk, 0) which should always fail, returning the current break.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
I believe this patch could be upstreamed, maybe at first as an RFC to gather interest, as it would fix the tests for Musl distributions.
testcases/kernel/syscalls/brk/brk01.c | 9 +++------ testcases/kernel/syscalls/brk/brk02.c | 14 ++++++-------- 2 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..919435755 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,6 +9,7 @@ #include <errno.h> #include "tst_test.h" +#include "lapi/syscalls.h" void verify_brk(void) { @@ -16,7 +17,7 @@ void verify_brk(void) size_t inc = getpagesize() * 2 - 1; unsigned int i;
- cur_brk = (uintptr_t)sbrk(0);
- cur_brk = (uintptr_t)tst_syscall(__NR_brk, 0);
for (i = 0; i < 33; i++) { switch (i % 3) { @@ -31,11 +32,7 @@ void verify_brk(void) break; }
TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()");if (!TST_PASS)return;cur_brk = (uintptr_t)sbrk(0);
cur_brk = tst_syscall(__NR_brk, new_brk);
For consistency it would be good to add explicit cast here (as already mentioned by Tudor). On the other hand, the explicit cast is no longer necessary as tst_syscall is dealing with intptr_t already, so you gonna get implicit conversion here either way.
--- BR B.
if (cur_brk != new_brk) { tst_res(TFAIL, diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c index 11e803cb4..dabda30c2 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -14,24 +14,22 @@ #include <unistd.h> #include <sys/mman.h> #include "tst_test.h" +#include "lapi/syscalls.h" void brk_down_vmas(void) {
- void *brk_addr = sbrk(0);
- if (brk_addr == (void *) -1)
tst_brk(TBROK, "sbrk() failed");
- void *brk_addr = (void *)tst_syscall(__NR_brk, 0);
unsigned long page_size = getpagesize(); void *addr = brk_addr + page_size;
- if (brk(addr)) {
- if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size"); return; }
addr += page_size;
- if (brk(addr)) {
- if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size"); return; }
@@ -42,12 +40,12 @@ void brk_down_vmas(void) } addr += page_size;
- if (brk(addr)) {
- if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect"); return; }
- if (brk(brk_addr)) {
- if ((void *)tst_syscall(__NR_brk, brk_addr) != brk_addr) { tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address"); return; }
-- 2.25.1
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org
Hi Beata,
On 18/11/2022 13:28, Beata Michalska wrote:
Hi Teo,
On Tue, Nov 15, 2022 at 05:01:06PM +0000, Teo Couprie Diaz wrote:
Direct usage of brk is discouraged in favor of using malloc. Also, brk was removed from POSIX in POSIX.1-2001. In particular, the musl libc's brk always returns -ENOMEM which causes the LTP tests to fail. Invoking the syscall directly allows them to pass.
How about: ... which causes the LTP tests to fail prematurely. Invoking the syscall directly allows them to carry out their validation of brk behaviour. ?
That sounds a bit better, thanks ! Slightly reworded for v3.
Instead, use tst_syscall() and handle the failure cases ourselves, as we don't depend on the libc to do it anymore.
The patch also removes the dependency on sbrk to get the current break as it has the same issues. Instead, call tst_syscall(__NR_brk, 0) which should always fail, returning the current break.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
I believe this patch could be upstreamed, maybe at first as an RFC to gather interest, as it would fix the tests for Musl distributions.
testcases/kernel/syscalls/brk/brk01.c | 9 +++------ testcases/kernel/syscalls/brk/brk02.c | 14 ++++++-------- 2 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..919435755 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,6 +9,7 @@ #include <errno.h> #include "tst_test.h" +#include "lapi/syscalls.h" void verify_brk(void) { @@ -16,7 +17,7 @@ void verify_brk(void) size_t inc = getpagesize() * 2 - 1; unsigned int i;
- cur_brk = (uintptr_t)sbrk(0);
- cur_brk = (uintptr_t)tst_syscall(__NR_brk, 0);
for (i = 0; i < 33; i++) { switch (i % 3) { @@ -31,11 +32,7 @@ void verify_brk(void) break; }
TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()");if (!TST_PASS)return;cur_brk = (uintptr_t)sbrk(0);
cur_brk = tst_syscall(__NR_brk, new_brk);For consistency it would be good to add explicit cast here (as already mentioned by Tudor). On the other hand, the explicit cast is no longer necessary as tst_syscall is dealing with intptr_t already, so you gonna get implicit conversion here either way.
Agreed, I'll switch everything to void * for v3. The cast is not really necessary as you mention, but I will add it just to be consistent.
BR B.
Thanks for the review ! Téo
if (cur_brk != new_brk) { tst_res(TFAIL, diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c index 11e803cb4..dabda30c2 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -14,24 +14,22 @@ #include <unistd.h> #include <sys/mman.h> #include "tst_test.h" +#include "lapi/syscalls.h" void brk_down_vmas(void) {
- void *brk_addr = sbrk(0);
- if (brk_addr == (void *) -1)
tst_brk(TBROK, "sbrk() failed");
- void *brk_addr = (void *)tst_syscall(__NR_brk, 0);
unsigned long page_size = getpagesize(); void *addr = brk_addr + page_size;
- if (brk(addr)) {
- if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size"); return; }
addr += page_size;
- if (brk(addr)) {
- if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size"); return; }
@@ -42,12 +40,12 @@ void brk_down_vmas(void) } addr += page_size;
- if (brk(addr)) {
- if ((void *)tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect"); return; }
- if (brk(brk_addr)) {
- if ((void *)tst_syscall(__NR_brk, brk_addr) != brk_addr) { tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address"); return; }
-- 2.25.1
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org
brk is disabled in purecap, so only check that calling it returns -ENOSYS. Do it explicitly otherwise the test would be skipped by tst_syscall.
brk02 cannot be completed in purecap as brk is not implemented. We don't need to add anything as tst_syscall will skip it with TCONF in purecap by detecting the -ENOSYS return.
Add brk01 and brk02 to the morello_transitional_extended list.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com --- runtest/morello_transitional_extended | 3 +++ testcases/kernel/syscalls/brk/brk01.c | 10 ++++++++++ 2 files changed, 13 insertions(+)
diff --git a/runtest/morello_transitional_extended b/runtest/morello_transitional_extended index 067fe82da..9549f9aa5 100644 --- a/runtest/morello_transitional_extended +++ b/runtest/morello_transitional_extended @@ -1,5 +1,8 @@ #DESCRIPTION: Morello transitional extended ABI system calls
+brk01 brk01 +brk02 brk02 + epoll_create01 epoll_create01 epoll_create02 epoll_create02 epoll_create1_01 epoll_create1_01 diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 919435755..a5de55b0a 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -17,6 +17,16 @@ void verify_brk(void) size_t inc = getpagesize() * 2 - 1; unsigned int i;
+#ifdef __CHERI_PURE_CAPABILITY__ + /* + * tst_syscall skips the test with TCONF if the syscall returns -ENOSYS. + * Use a direct syscall to check that it does return -ENOSYS correctly + * and explicitly PASS or FAIL. + */ + TST_EXP_FAIL(syscall(__NR_brk, 0), ENOSYS, "brk is not implemented in purecap"); + return; +#endif + cur_brk = (uintptr_t)tst_syscall(__NR_brk, 0);
for (i = 0; i < 33; i++) {
Hi Teo,
On Tue, Nov 15, 2022 at 05:01:07PM +0000, Teo Couprie Diaz wrote:
brk is disabled in purecap, so only check that calling it returns -ENOSYS. Do it explicitly otherwise the test would be skipped by tst_syscall.
This line break looks bit odd. Is that intentional ? (I think similar break is in PATCH 1/2) Otherwise just thinking if we could add a reference to ABI specification [1] to give some background on why brk is not supported ?
[1] https://git.morello-project.org/morello/kernel/linux/-/wikis/Morello-pure-ca...
brk02 cannot be completed in purecap as brk is not implemented. We don't need to add anything as tst_syscall will skip it with TCONF in purecap by detecting the -ENOSYS return.
To be fair I think both brk01 & brk02 cannot be completed. Not sure I understand why only brk02 is to suffer from lack of support for brk (?) FWIW: brk01 is also using tst_syscall. I assume this is to check that brk is not supported with purecap mode only once? If so I believe both tests should verify that. Apologies if I suggested smth otherwise in our previous discussions.
Add brk01 and brk02 to the morello_transitional_extended list.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
runtest/morello_transitional_extended | 3 +++ testcases/kernel/syscalls/brk/brk01.c | 10 ++++++++++ 2 files changed, 13 insertions(+)
diff --git a/runtest/morello_transitional_extended b/runtest/morello_transitional_extended index 067fe82da..9549f9aa5 100644 --- a/runtest/morello_transitional_extended +++ b/runtest/morello_transitional_extended @@ -1,5 +1,8 @@ #DESCRIPTION: Morello transitional extended ABI system calls +brk01 brk01 +brk02 brk02
epoll_create01 epoll_create01 epoll_create02 epoll_create02 epoll_create1_01 epoll_create1_01 diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 919435755..a5de55b0a 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -17,6 +17,16 @@ void verify_brk(void) size_t inc = getpagesize() * 2 - 1; unsigned int i; +#ifdef __CHERI_PURE_CAPABILITY__
- /*
* tst_syscall skips the test with TCONF if the syscall returns -ENOSYS.* Use a direct syscall to check that it does return -ENOSYS correctly* and explicitly PASS or FAIL.*/- TST_EXP_FAIL(syscall(__NR_brk, 0), ENOSYS, "brk is not implemented in purecap");
- return;
+#endif
This is good, though we might add a 'setup' stage to both tests to validate if the syscall is supported prior to running those tests, if at all:
static void setup(void) { #ifdef __CHERI_PURE_CAPABILITY__ ... TST_EXP_FAIL(syscall(__NR_brk, 0), ENOSYS, "brk is not implemented in purecap"); tst_brk(TCONF, "Test not supported."); /* Or TST_SYSCALL_BRK__ to be compatible with LTP's 'old' setup */ #endif }
static struct tst_test test = { .setup = setup, .test_all = verify_brk, }
which would render smth below the lines: TPASS: brk is not implemented in purecap : ENOSYS (38) TCONF: Test not supported.
(second message to be tuned in)
--- BR B.
cur_brk = (uintptr_t)tst_syscall(__NR_brk, 0); for (i = 0; i < 33; i++) { -- 2.25.1
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org
Hi Beata,
On 18/11/2022 13:28, Beata Michalska wrote:
Hi Teo,
On Tue, Nov 15, 2022 at 05:01:07PM +0000, Teo Couprie Diaz wrote:
brk is disabled in purecap, so only check that calling it returns -ENOSYS. Do it explicitly otherwise the test would be skipped by tst_syscall.
This line break looks bit odd. Is that intentional ?
It is both intentional and a bit odd. I didn't think it would fit in the 75 characters without the break but it does so exactly, so moved back on one line.
(I think similar break is in PATCH 1/2) Otherwise just thinking if we could add a reference to ABI specification [1] to give some background on why brk is not supported ?
[1] https://git.morello-project.org/morello/kernel/linux/-/wikis/Morello-pure-ca...
Agreed, good point ! Added for v3.
brk02 cannot be completed in purecap as brk is not implemented. We don't need to add anything as tst_syscall will skip it with TCONF in purecap by detecting the -ENOSYS return.
To be fair I think both brk01 & brk02 cannot be completed.
Yeah, fair !
Not sure I understand why only brk02 is to suffer from lack of support for brk (?) FWIW: brk01 is also using tst_syscall. I assume this is to check that brk is not supported with purecap mode only once?
That was the idea indeed, this way brk01 does the testing and brk02 is just disabled. See my response below regarding using setup.
If so I believe both tests should verify that. Apologies if I suggested smth otherwise in our previous discussions.
Add brk01 and brk02 to the morello_transitional_extended list.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
runtest/morello_transitional_extended | 3 +++ testcases/kernel/syscalls/brk/brk01.c | 10 ++++++++++ 2 files changed, 13 insertions(+)
diff --git a/runtest/morello_transitional_extended b/runtest/morello_transitional_extended index 067fe82da..9549f9aa5 100644 --- a/runtest/morello_transitional_extended +++ b/runtest/morello_transitional_extended @@ -1,5 +1,8 @@ #DESCRIPTION: Morello transitional extended ABI system calls +brk01 brk01 +brk02 brk02
- epoll_create01 epoll_create01 epoll_create02 epoll_create02 epoll_create1_01 epoll_create1_01
diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 919435755..a5de55b0a 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -17,6 +17,16 @@ void verify_brk(void) size_t inc = getpagesize() * 2 - 1; unsigned int i; +#ifdef __CHERI_PURE_CAPABILITY__
- /*
* tst_syscall skips the test with TCONF if the syscall returns -ENOSYS.* Use a direct syscall to check that it does return -ENOSYS correctly* and explicitly PASS or FAIL.*/- TST_EXP_FAIL(syscall(__NR_brk, 0), ENOSYS, "brk is not implemented in purecap");
- return;
+#endif
This is good, though we might add a 'setup' stage to both tests to validate if the syscall is supported prior to running those tests, if at all:
static void setup(void) { #ifdef __CHERI_PURE_CAPABILITY__ ... TST_EXP_FAIL(syscall(__NR_brk, 0), ENOSYS, "brk is not implemented in purecap"); tst_brk(TCONF, "Test not supported."); /* Or TST_SYSCALL_BRK__ to be compatible with LTP's 'old' setup */ #endif }
static struct tst_test test = { .setup = setup, .test_all = verify_brk, }
which would render smth below the lines: TPASS: brk is not implemented in purecap : ENOSYS (38) TCONF: Test not supported.
(second message to be tuned in)
The setup makes it way more logical for me for both tests to check if it works correctly in purecap. Then it's just part of setting up the test, which go on to do (or skip) their own thing.
Will change them to use that for v3.
BR B.
Thanks for the review, Téo
cur_brk = (uintptr_t)tst_syscall(__NR_brk, 0); for (i = 0; i < 33; i++) { -- 2.25.1
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org
linux-morello-ltp@op-lists.linaro.org
-
Beata Michalska -
Teo Couprie Diaz -
Tudor Cretu