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-v4 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. v4: Check if enabled in brk01 _and_ brk02 via setup, unify types, commit message improvements.
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 | 23 +++++++++++++++-------- testcases/kernel/syscalls/brk/brk02.c | 24 ++++++++++++++++-------- 3 files changed, 34 insertions(+), 16 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 prematurely. Invoking the syscall directly allows them to properly validate brk behavior.
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 --- testcases/kernel/syscalls/brk/brk01.c | 13 +++++-------- testcases/kernel/syscalls/brk/brk02.c | 14 ++++++-------- 2 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..d4596c20f 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,14 +9,15 @@ #include <errno.h>
#include "tst_test.h" +#include "lapi/syscalls.h"
void verify_brk(void) { - uintptr_t cur_brk, new_brk; + void *cur_brk, *new_brk; size_t inc = getpagesize() * 2 - 1; unsigned int i;
- cur_brk = (uintptr_t)sbrk(0); + cur_brk = (void *)tst_syscall(__NR_brk, 0);
for (i = 0; i < 33; i++) { switch (i % 3) { @@ -31,16 +32,12 @@ 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 = (void *)tst_syscall(__NR_brk, new_brk);
if (cur_brk != new_brk) { tst_res(TFAIL, "brk() failed to set address have %p expected %p", - (void *)cur_brk, (void *)new_brk); + cur_brk, new_brk); return; }
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; }
base-commit: 75fda47a48d1b6f1c6671fa9819ca9fd9169cf2a
On 18/11/2022 19:39, 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 prematurely. Invoking the syscall directly allows them to
*exit* prematurely?
properly validate brk behavior.
Instead, use tst_syscall() and handle the failure cases ourselves, as
I think s/Instead,// as you have already introduced that idea in the previous sentence.
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.
I think it can't both fail and return the current break :) Probably remove "which should always fail", or maybe s/always/never/?
Aside from that the code looks good to me, and so does patch 2.
Kevin
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
testcases/kernel/syscalls/brk/brk01.c | 13 +++++-------- testcases/kernel/syscalls/brk/brk02.c | 14 ++++++-------- 2 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..d4596c20f 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,14 +9,15 @@ #include <errno.h> #include "tst_test.h" +#include "lapi/syscalls.h" void verify_brk(void) {
- uintptr_t cur_brk, new_brk;
- void *cur_brk, *new_brk; size_t inc = getpagesize() * 2 - 1; unsigned int i;
- cur_brk = (uintptr_t)sbrk(0);
- cur_brk = (void *)tst_syscall(__NR_brk, 0);
for (i = 0; i < 33; i++) { switch (i % 3) { @@ -31,16 +32,12 @@ 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 = (void *)tst_syscall(__NR_brk, new_brk);if (cur_brk != new_brk) { tst_res(TFAIL, "brk() failed to set address have %p expected %p",
(void *)cur_brk, (void *)new_brk);
cur_brk, new_brk); return;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; }
base-commit: 75fda47a48d1b6f1c6671fa9819ca9fd9169cf2a
Hi Kevin,
On 21/11/2022 16:44, Kevin Brodsky wrote:
On 18/11/2022 19:39, 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 prematurely. Invoking the syscall directly allows them to
*exit* prematurely?
Correct, I did forget it.
properly validate brk behavior.
Instead, use tst_syscall() and handle the failure cases ourselves, as
I think s/Instead,// as you have already introduced that idea in the previous sentence.
That does sound better, thanks.
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.
I think it can't both fail and return the current break :) Probably remove "which should always fail", or maybe s/always/never/?
Well, from my understanding that's exactly the expected behavior ! From the man page BRK(2):
[...], the actual Linux system call returns the new program break on success. *On
failure, the
system call returns the current break.* The glibc wrapper
function does
some work (i.e., checks whether the new break is less than
addr) to
provide the 0 and -1 return values described above.
Also in mm/mmap.c, at the end of SYSCALL_DEFINE1(brk, unsigned long, brk) :
out: mmap_write_unlock(mm); return origbrk;
Where origbrk is saved at the beginning and out is jumped to whenever there is an error.
So making a brk syscall with 0 should always fail (it's smaller than the minimum break), and also return the current(/original ?) break unchanged.
But maybe I am misunderstanding your comment itself ? Please let me know !
Aside from that the code looks good to me, and so does patch 2.
Kevin
Thanks for the review Kevin ! Regards, Téo
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
testcases/kernel/syscalls/brk/brk01.c | 13 +++++-------- testcases/kernel/syscalls/brk/brk02.c | 14 ++++++-------- 2 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..d4596c20f 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,14 +9,15 @@ #include <errno.h> #include "tst_test.h" +#include "lapi/syscalls.h" void verify_brk(void) {
- uintptr_t cur_brk, new_brk;
- void *cur_brk, *new_brk; size_t inc = getpagesize() * 2 - 1; unsigned int i;
- cur_brk = (uintptr_t)sbrk(0);
- cur_brk = (void *)tst_syscall(__NR_brk, 0);
for (i = 0; i < 33; i++) { switch (i % 3) { @@ -31,16 +32,12 @@ 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 = (void *)tst_syscall(__NR_brk, new_brk);if (cur_brk != new_brk) { tst_res(TFAIL, "brk() failed to set address have %p expected %p",
(void *)cur_brk, (void *)new_brk);
cur_brk, new_brk); return;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; }
base-commit: 75fda47a48d1b6f1c6671fa9819ca9fd9169cf2a
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
On 23/11/2022 11:57, Teo Couprie Diaz wrote:
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.
I think it can't both fail and return the current break 😄 Probably remove "which should always fail", or maybe s/always/never/?
Well, from my understanding that's exactly the expected behavior ! From the man page BRK(2):
[...], the actual Linux system call returns the new program break on success. *On
failure, the
system call returns the current break.* The glibc wrapper
function does
some work (i.e., checks whether the new break is less than
addr) to
provide the 0 and -1 return values described above.
Also in mm/mmap.c, at the end of SYSCALL_DEFINE1(brk, unsigned long, brk) :
out: mmap_write_unlock(mm); return origbrk;
Where origbrk is saved at the beginning and out is jumped to whenever there is an error.
So making a brk syscall with 0 should always fail (it's smaller than the minimum break), and also return the current(/original ?) break unchanged.
But maybe I am misunderstanding your comment itself ? Please let me know !
Right yes sorry I was thinking about it from the wrong angle, that is that no *wrapper* is failing (because tst_syscall() just makes a raw syscall). But yes indeed the syscall is technically failing when you do that. Maybe it's better to just say that calling tst_syscall this way always returns the current break without elaborating further?
Kevin
On 23/11/2022 16:08, Kevin Brodsky wrote:
On 23/11/2022 11:57, Teo Couprie Diaz wrote:
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.
I think it can't both fail and return the current break 😄 Probably remove "which should always fail", or maybe s/always/never/?
Well, from my understanding that's exactly the expected behavior ! From the man page BRK(2):
[...], the actual Linux system call returns the new program break on success. *On
failure, the
system call returns the current break.* The glibc wrapper
function does
some work (i.e., checks whether the new break is less than
addr) to
provide the 0 and -1 return values described above.
Also in mm/mmap.c, at the end of SYSCALL_DEFINE1(brk, unsigned long, brk) :
out: mmap_write_unlock(mm); return origbrk;
Where origbrk is saved at the beginning and out is jumped to whenever there is an error.
So making a brk syscall with 0 should always fail (it's smaller than the minimum break), and also return the current(/original ?) break unchanged.
But maybe I am misunderstanding your comment itself ? Please let me know !
Right yes sorry I was thinking about it from the wrong angle, that is that no *wrapper* is failing (because tst_syscall() just makes a raw syscall). But yes indeed the syscall is technically failing when you do that. Maybe it's better to just say that calling tst_syscall this way always returns the current break without elaborating further?
Ah yeah ok, I see what you mean.
"Instead, call tst_syscall(__NR_brk, 0) which returns the current break."
Should be alright then, if that's ok with you ?
Kevin
Téo
On 23/11/2022 17:23, Teo Couprie Diaz wrote:
On 23/11/2022 16:08, Kevin Brodsky wrote:
On 23/11/2022 11:57, Teo Couprie Diaz wrote:
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.
I think it can't both fail and return the current break 😄 Probably remove "which should always fail", or maybe s/always/never/?
Well, from my understanding that's exactly the expected behavior ! From the man page BRK(2):
[...], the actual Linux system call returns the new program break on success. *On
failure, the
system call returns the current break.* The glibc wrapper
function does
some work (i.e., checks whether the new break is less than
addr) to
provide the 0 and -1 return values described above.
Also in mm/mmap.c, at the end of SYSCALL_DEFINE1(brk, unsigned long, brk) :
out: mmap_write_unlock(mm); return origbrk;
Where origbrk is saved at the beginning and out is jumped to whenever there is an error.
So making a brk syscall with 0 should always fail (it's smaller than the minimum break), and also return the current(/original ?) break unchanged.
But maybe I am misunderstanding your comment itself ? Please let me know !
Right yes sorry I was thinking about it from the wrong angle, that is that no *wrapper* is failing (because tst_syscall() just makes a raw syscall). But yes indeed the syscall is technically failing when you do that. Maybe it's better to just say that calling tst_syscall this way always returns the current break without elaborating further?
Ah yeah ok, I see what you mean.
"Instead, call tst_syscall(__NR_brk, 0) which returns the current break."
Should be alright then, if that's ok with you ?
Fine by me, you can add "always" to make it clear it doesn't... fail to fail? :D
Kevin
On 23/11/2022 16:25, Kevin Brodsky wrote:
On 23/11/2022 17:23, Teo Couprie Diaz wrote:
On 23/11/2022 16:08, Kevin Brodsky wrote:
On 23/11/2022 11:57, Teo Couprie Diaz wrote:
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.
I think it can't both fail and return the current break 😄 Probably remove "which should always fail", or maybe s/always/never/?
Well, from my understanding that's exactly the expected behavior ! From the man page BRK(2):
[...], the actual Linux system call returns the new program break on success. *On
failure, the
system call returns the current break.* The glibc wrapper
function does
some work (i.e., checks whether the new break is less than
addr) to
provide the 0 and -1 return values described above.
Also in mm/mmap.c, at the end of SYSCALL_DEFINE1(brk, unsigned long, brk) :
out: mmap_write_unlock(mm); return origbrk;
Where origbrk is saved at the beginning and out is jumped to whenever there is an error.
So making a brk syscall with 0 should always fail (it's smaller than the minimum break), and also return the current(/original ?) break unchanged.
But maybe I am misunderstanding your comment itself ? Please let me know !
Right yes sorry I was thinking about it from the wrong angle, that is that no *wrapper* is failing (because tst_syscall() just makes a raw syscall). But yes indeed the syscall is technically failing when you do that. Maybe it's better to just say that calling tst_syscall this way always returns the current break without elaborating further?
Ah yeah ok, I see what you mean.
"Instead, call tst_syscall(__NR_brk, 0) which returns the current break."
Should be alright then, if that's ok with you ?
Fine by me, you can add "always" to make it clear it doesn't... fail to fail? :D
Makes sense, all proposed changes were added in the v5 !
Kevin
Thanks for the review, Téo
brk is disabled in purecap, see the ABI for reference[1], so only check that calling it returns -ENOSYS. Do it explicitly in the test setup otherwise the tests would be skipped by tst_syscall. Skip them in purecap if it is correctly disabled.
Add brk01 and brk02 to the morello_transitional_extended list.
[1]: https://git.morello-project.org/morello/kernel/linux/-/wikis/Morello-pure-ca...
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com --- runtest/morello_transitional_extended | 3 +++ testcases/kernel/syscalls/brk/brk01.c | 10 ++++++++++ testcases/kernel/syscalls/brk/brk02.c | 10 ++++++++++ 3 files changed, 23 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 d4596c20f..adc6153b2 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -49,6 +49,16 @@ void verify_brk(void) tst_res(TPASS, "brk() works fine"); }
+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, "Cannot test brk in purecap: syscall disabled."); +#endif +} + static struct tst_test test = { + .setup = setup, .test_all = verify_brk, }; diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c index dabda30c2..ec1db545e 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -53,6 +53,16 @@ void brk_down_vmas(void) tst_res(TPASS, "munmap at least two VMAs of brk() passed"); }
+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, "Cannot test brk in purecap: syscall disabled."); +#endif +} + static struct tst_test test = { + .setup = setup, .test_all = brk_down_vmas, };
linux-morello-ltp@op-lists.linaro.org
-
Kevin Brodsky -
Teo Couprie Diaz