Hi Teo,
On 18-10-2022 09:46, Teo Couprie Diaz wrote:
Direct usage of brk is discouraged in favor of using malloc and
s/and/as maybe?
it was removed from POSIX in POSIX.1-2001. In particular, the musl libc always returns -ENOMEM for brk which means that the LTP tests fail when calling the kernel directly would pass.
I find this sentence a bit confusing, maybe something along this lines would be more clear:
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.
sbrk has the same issues so this patch reworks the brk tests slightly to only use direct brk syscalls.
Do you mean that you rewrite the sbrk calls to use the brk syscall instead? If so, I would just say that given that sbrk might have the same issues, you removed the dependency on it. Also, I would make it clear why tst_syscall(__NR_brk, 0) can replace sbrk(0). Up to you!
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
As per internal discussion I believe this patch could be proposed upstream once it is in a good shape. It would fix the brk tests for musl-based systems like Alpine where they currently fail. Note that sbrk is not fixed by this patch as it is fully implemented in the libc: there is no syscall.
testcases/kernel/syscalls/brk/brk01.c | 22 ++++++++++++++++++---- testcases/kernel/syscalls/brk/brk02.c | 17 ++++++++++------- 2 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..c16b46eaa 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c
Missing: #include "lapi/syscalls.h" I think
@@ -16,7 +16,15 @@ void verify_brk(void) size_t inc = getpagesize() * 2 - 1; unsigned int i;
- cur_brk = (uintptr_t)sbrk(0);
- long ret = tst_syscall(__NR_brk, 0);
Is there any need to have ret? I suggest keeping cur_brk.
- if (ret < 0)
- {
Open brace should be on the previous line. Also below.
tst_res(TFAIL, "Failed to get current break"); > + return;- }
I am not sure that the result of the brk syscalls can ever be less than 0. The Linux syscall returns the new program break on success, while on failure, the syscall returns the current break. It should always fail if you call tst_syscall(__NR_brk, 0), so it should always give the current program break. I believe this is what Musl also does for sbrk(0). Therefore, there is no need to have this block. Also below.
- cur_brk = (uintptr_t)ret; > for (i = 0; i < 33; i++) { switch (i % 3) {
@@ -31,11 +39,17 @@ void verify_brk(void) break; }
TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()");if (!TST_PASS)
tst_syscall(__NR_brk, new_brk);
In the original code, if brk((void *)new_brk), "brk()") fails, the test prints a TFAIL message and ends. This doesn't happen anymore. Just keep the TST_EXP_PASS_SILENT and only replace the brk(..) with tst_syscall(__NR_brk, ...).
ret = tst_syscall(__NR_brk, 0);if (ret < 0)> + tst_res(TFAIL, "Failed to get current break"); return;}
cur_brk = (uintptr_t)sbrk(0);
cur_brk = (uintptr_t)ret;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..19514febb 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -14,24 +14,27 @@ #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);
- long ret = tst_syscall(__NR_brk, 0);
I suggest keeping brk_addr here as well. No need to store the result into a long that you cast to void* afterwards. You can change the brk_addr's type to uintptr_t though if you want.
- if (brk_addr == (void *) -1)
tst_brk(TBROK, "sbrk() failed");
- if (ret < 0)
tst_brk(TBROK, "Failed to get current break");
Same as above, tst_syscall(__NR_brk, 0) never returns below 0, so no need to check it.
Good patch overall, only a few nits! Make sure to verify the LTP patches against checkpatch as well.
- void *brk_addr = (void *) ret;
unsigned long page_size = getpagesize(); void *addr = brk_addr + page_size;
- if (brk(addr)) {
- if (tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size"); return; }
addr += page_size;
- if (brk(addr)) {
- if (tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size"); return; }
@@ -42,12 +45,12 @@ void brk_down_vmas(void) } addr += page_size;
- if (brk(addr)) {
- if (tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect"); return; }
- if (brk(brk_addr)) {
- if (tst_syscall(__NR_brk, brk_addr) != brk_addr) { tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address"); return; }