| Message ID | 20221207092428.11798-1-teo.coupriediaz@arm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] syscalls/brk: add direct syscall tst_variant | expand |
Cyril, Petr, Would you like to add your reviewed by tags? Then we can merge this. Teo Couprie Diaz <teo.coupriediaz@arm.com> writes: > 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 exit prematurely. Invoking the syscall directly allows > them to properly validate brk behavior. Add a new test variant handling if > the libc wrappers are not implemented and testing the direct syscall. > > Use tst_syscall() and handle the failure cases ourselves, as > we don't depend on the libc to do it anymore. > > The patch also works around the dependency on sbrk to get the current break > as it has the same issues. Instead, call tst_syscall(__NR_brk, 0) which > always returns the current break. > > Update brk01 to use void* to unify it with brk02. > > Signed-off-by: Teo Couprie Diaz <teo.coupriediaz@arm.com> > --- > v1: Reworked from RFC by adding a variant rather than replacing the > existing tests. Thanks Petr and Cyril ! > v2: brk doesn't return the new break, just if it error'd or not. > Fix styling and warnings. > > testcases/kernel/syscalls/brk/brk01.c | 37 ++++++++++++++++------ > testcases/kernel/syscalls/brk/brk02.c | 44 ++++++++++++++++++++++----- > 2 files changed, 65 insertions(+), 16 deletions(-) > > diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c > index a9b89bce5..93e430328 100644 > --- a/testcases/kernel/syscalls/brk/brk01.c > +++ b/testcases/kernel/syscalls/brk/brk01.c > @@ -9,14 +9,31 @@ > #include <errno.h> > > #include "tst_test.h" > +#include "lapi/syscalls.h" > > void verify_brk(void) > { > - uintptr_t cur_brk, new_brk; > - uintptr_t inc = getpagesize() * 2 - 1; > + void *cur_brk, *new_brk; > + size_t inc = getpagesize() * 2 - 1; > unsigned int i; > > - cur_brk = (uintptr_t)sbrk(0); > + if (tst_variant) { > + tst_res(TINFO, "Testing syscall variant"); > + cur_brk = (void *)tst_syscall(__NR_brk, 0); > + } else { > + tst_res(TINFO, "Testing libc variant"); > + cur_brk = (void *)sbrk(0); > + > + if (cur_brk == (void *)-1) > + tst_brk(TCONF, "sbrk() not implemented"); > + > + /* > + * Check if brk itself is implemented: updating to the current break > + * should be a no-op. > + */ > + if (brk(cur_brk) != 0) > + tst_brk(TCONF, "brk() not implemented"); > + } > > for (i = 0; i < 33; i++) { > switch (i % 3) { > @@ -31,16 +48,17 @@ void verify_brk(void) > break; > } > > - TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()"); > - if (!TST_PASS) > - return; > - > - cur_brk = (uintptr_t)sbrk(0); > + if (tst_variant) { > + cur_brk = (void *)tst_syscall(__NR_brk, new_brk); > + } else { > + TST_EXP_PASS_SILENT(brk(new_brk), "brk()"); > + cur_brk = sbrk(0); > + } > > 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; > } > > @@ -54,4 +72,5 @@ void verify_brk(void) > > static struct tst_test test = { > .test_all = verify_brk, > + .test_variants = 2, > }; > diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c > index 11e803cb4..3a97d279b 100644 > --- a/testcases/kernel/syscalls/brk/brk02.c > +++ b/testcases/kernel/syscalls/brk/brk02.c > @@ -14,24 +14,53 @@ > #include <unistd.h> > #include <sys/mman.h> > #include "tst_test.h" > +#include "lapi/syscalls.h" > + > +void *brk_variants(void *addr) > +{ > + void *brk_addr; > + > + if (tst_variant) { > + brk_addr = (void *)tst_syscall(__NR_brk, addr); > + } else { > + TST_EXP_PASS_SILENT(brk(addr), "brk()"); > + brk_addr = (void *)sbrk(0); > + } > + return brk_addr; > +} > > void brk_down_vmas(void) > { > - void *brk_addr = sbrk(0); > + void *brk_addr; > + > + if (tst_variant) { > + tst_res(TINFO, "Testing syscall variant"); > + brk_addr = (void *)tst_syscall(__NR_brk, 0); > + } else { > + tst_res(TINFO, "Testing libc variant"); > + brk_addr = (void *)sbrk(0); > > - if (brk_addr == (void *) -1) > - tst_brk(TBROK, "sbrk() failed"); > + if (brk_addr == (void *)-1) > + tst_brk(TCONF, "sbrk() not implemented"); > + > + /* > + * Check if brk itself is implemented: updating to the current break > + * should be a no-op. > + */ > + if (brk(brk_addr) != 0) > + tst_brk(TCONF, "brk() not implemented"); > + } > > unsigned long page_size = getpagesize(); > void *addr = brk_addr + page_size; > > - if (brk(addr)) { > + if (brk_variants(addr) < addr) { > tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size"); > return; > } > > addr += page_size; > - if (brk(addr)) { > + if (brk_variants(addr) < addr) { > tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size"); > return; > } > @@ -42,12 +71,12 @@ void brk_down_vmas(void) > } > > addr += page_size; > - if (brk(addr)) { > + if (brk_variants(addr) < addr) { > tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect"); > return; > } > > - if (brk(brk_addr)) { > + if (brk_variants(brk_addr) != brk_addr) { > tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address"); > return; > } > @@ -57,4 +86,5 @@ void brk_down_vmas(void) > > static struct tst_test test = { > .test_all = brk_down_vmas, > + .test_variants = 2, > }; > > base-commit: 990c0b48f8508a747863b1dea5556fba57e1c304 > -- > 2.25.1
Hi Richie, > Cyril, Petr, Would you like to add your reviewed by tags? Then we can > merge this. By accident I reply to my points to v1 [1]. To copy it here: 1) There are warnings: brk02.c: In function ‘brk_variants’: brk02.c:26:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] 26 | brk_addr = (void *)brk(addr); | ^ 2) make check reports errors which are easily fixed. Teo replied [2], that he's going to fix it. I thought I had set it "Changes requested", but now I see "Needs Review / ACK". Setting it to "Changes requested". Kind regards, Petr [1] https://lore.kernel.org/ltp/20221206140421.GB21839@pevik/ [2] https://lore.kernel.org/ltp/fe1c5bac-0ed1-92ef-3c28-e3758dc3465d@arm.com/
Hi Petr, On 12/12/2022 15:22, Petr Vorel wrote: > Hi Richie, > >> Cyril, Petr, Would you like to add your reviewed by tags? Then we can >> merge this. > By accident I reply to my points to v1 [1]. > To copy it here: > > 1) There are warnings: > brk02.c: In function ‘brk_variants’: > brk02.c:26:28: warning: cast to pointer from integer of different size > [-Wint-to-pointer-cast] > 26 | brk_addr = (void *)brk(addr); > | ^ > > 2) make check reports errors which are easily fixed. > > Teo replied [2], that he's going to fix it. I thought I had set it > "Changes requested", but now I see "Needs Review / ACK". Setting it to > "Changes requested". I believe the points you raised are fixed in the v2, on top of this thread. Re-applying it on top of master on my side doesn't give me any warnings for the brk tests, as I do not cast the result from the libc brk anymore, and make check reports existing issues with the name of the function, but no style problems that did exist in v1. (I don't mind changing them if you want, but they are present on master as well). If you give a quick look at the patch v2 you'll see that indeed there is no more (void *)brk(addr) or such that generated the warnings, for example. (The syscalls still need it, as they return the break directly rather than an error, which is what the libc wrapper does.) I might be missing something, please do tell me if that's the case ! But I believe that the v2 _should_ be free of those issues. > Kind regards, > Petr > > [1] https://lore.kernel.org/ltp/20221206140421.GB21839@pevik/ > [2] https://lore.kernel.org/ltp/fe1c5bac-0ed1-92ef-3c28-e3758dc3465d@arm.com/ Thanks for taking the time. Best regards, Téo
Hi Teo, > Hi Petr, > On 12/12/2022 15:22, Petr Vorel wrote: > > Hi Richie, > > > Cyril, Petr, Would you like to add your reviewed by tags? Then we can > > > merge this. > > By accident I reply to my points to v1 [1]. > > To copy it here: > > 1) There are warnings: > > brk02.c: In function ‘brk_variants’: > > brk02.c:26:28: warning: cast to pointer from integer of different size > > [-Wint-to-pointer-cast] > > 26 | brk_addr = (void *)brk(addr); > > | ^ > > 2) make check reports errors which are easily fixed. > > Teo replied [2], that he's going to fix it. I thought I had set it > > "Changes requested", but now I see "Needs Review / ACK". Setting it to > > "Changes requested". > I believe the points you raised are fixed in the v2, on top of this thread. > Re-applying it on top of master on my side doesn't give me any warnings for > the brk tests, as I do not cast the result from the libc brk anymore, and > make check reports existing issues with the name of the function, but no > style problems that did exist in v1. (I don't mind changing them if you > want, but they are present on master as well). > If you give a quick look at the patch v2 you'll see that indeed there is no > more (void *)brk(addr) or such that generated the warnings, for example. > (The syscalls still need it, as they return the break directly rather than > an error, which is what the libc wrapper does.) > I might be missing something, please do tell me if that's the case ! But I > believe that the v2 _should_ be free of those issues. I thought I must have put v1 code into my v2 branch. But the warnings are actually when compiling on Alpine: $ make V=1 gcc -I../../../../include -I../../../../include -I../../../../include/old/ -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -L../../../../lib brk01.c -lltp -o brk01 brk01.c: In function 'verify_brk': brk01.c:22:27: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] 22 | cur_brk = (void *)tst_syscall(__NR_brk, 0); | ^ brk01.c:52:35: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] 52 | cur_brk = (void *)tst_syscall(__NR_brk, new_brk); | ^ gcc -I../../../../include -I../../../../include -I../../../../include/old/ -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -L../../../../lib brk02.c -lltp -o brk02 brk02.c: In function 'brk_variants': brk02.c:24:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] 24 | brk_addr = (void *)tst_syscall(__NR_brk, addr); | ^ brk02.c: In function 'brk_down_vmas': brk02.c:38:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] 38 | brk_addr = (void *)tst_syscall(__NR_brk, 0); | ^ $ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-alpine-linux-musl/12.1.1/lto-wrapper Target: x86_64-alpine-linux-musl Configured with: /home/buildozer/aports/main/gcc/src/gcc-12.1.1_git20220630/configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --build=x86_64-alpine-linux-musl --host=x86_64-alpine-linux-musl --target=x86_64-alpine-linux-musl --with-pkgversion='Alpine 12.1.1_git20220630' --enable-checking=release --disable-fixed-point --disable-libstdcxx-pch --disable-multilib --disable-nls --disable-werror --disable-symvers --enable-__cxa_atexit --enable-default-pie --enable-default-ssp --enable-cloog-backend --enable-languages=c,c++,d,objc,go,fortran,ada,jit --disable-libssp --disable-libmpx --disable-libmudflap --disable-libsanitizer --enable-shared --enable-threads --enable-tls --enable-host-shared --with-system-zlib --with-linker-hash-style=gnu Thread model: posix Supported LTO compression algorithms: zlib gcc version 12.1.1 20220630 (Alpine 12.1.1_git20220630) $ sudo ./brk01 tst_test.c:1558: TINFO: Timeout per run is 0h 00m 30s brk01.c:24: TINFO: Testing libc variant brk01.c:35: TCONF: brk() not implemented brk01.c:21: TINFO: Testing syscall variant brk01.c:59: TFAIL: brk() failed to set address have 0x4d3c4000 expected 0x4d3c5fff $ sudo ./brk02 tst_test.c:1558: TINFO: Timeout per run is 0h 00m 30s brk02.c:40: TINFO: Testing libc variant brk02.c:51: TCONF: brk() not implemented brk02.c:37: TINFO: Testing syscall variant brk02.c:58: TFAIL: Cannot expand brk() by page size: EFAULT (14) Looking on extra brk() support detection, you must have tested it on Alpine. What am I missing? Have you also compiled the code on Alpine or musl related distro? If tested on musl, but not Alpine, what do you use? (maybe Alpine issue?) I tested on some old random Alpine (3.17_alpha20220809), I'll try on some updated version. Also I wonder if we should bother with adding brk() / sbrk() logic into separate function, added into brk.h. Probably not, although I don't like repeating code, it's not worth for single function. There are still missing static found by 'make check', diff below will fix that (+ some additional space for readability). Kind regards, Petr diff --git testcases/kernel/syscalls/brk/brk01.c testcases/kernel/syscalls/brk/brk01.c index 93e430328..978e1f211 100644 --- testcases/kernel/syscalls/brk/brk01.c +++ testcases/kernel/syscalls/brk/brk01.c @@ -11,7 +11,7 @@ #include "tst_test.h" #include "lapi/syscalls.h" -void verify_brk(void) +static void verify_brk(void) { void *cur_brk, *new_brk; size_t inc = getpagesize() * 2 - 1; diff --git testcases/kernel/syscalls/brk/brk02.c testcases/kernel/syscalls/brk/brk02.c index 3a97d279b..64931bc80 100644 --- testcases/kernel/syscalls/brk/brk02.c +++ testcases/kernel/syscalls/brk/brk02.c @@ -5,6 +5,7 @@ /*\ * [Description] + * * Expand brk() by at least 2 pages to ensure there is a newly created VMA * and not expanding the original due to multiple anon pages. mprotect() that * new VMA then brk() back to the original address therefore causing a munmap of @@ -16,7 +17,7 @@ #include "tst_test.h" #include "lapi/syscalls.h" -void *brk_variants(void *addr) +static void *brk_variants(void *addr) { void *brk_addr; @@ -26,10 +27,11 @@ void *brk_variants(void *addr) TST_EXP_PASS_SILENT(brk(addr), "brk()"); brk_addr = (void *)sbrk(0); } + return brk_addr; } -void brk_down_vmas(void) +static void brk_down_vmas(void) { void *brk_addr;
Hello, Petr Vorel <pvorel@suse.cz> writes: > Hi Teo, > >> Hi Petr, > >> On 12/12/2022 15:22, Petr Vorel wrote: >> > Hi Richie, > >> > > Cyril, Petr, Would you like to add your reviewed by tags? Then we can >> > > merge this. >> > By accident I reply to my points to v1 [1]. >> > To copy it here: > >> > 1) There are warnings: >> > brk02.c: In function ‘brk_variants’: >> > brk02.c:26:28: warning: cast to pointer from integer of different size >> > [-Wint-to-pointer-cast] >> > 26 | brk_addr = (void *)brk(addr); >> > | ^ > >> > 2) make check reports errors which are easily fixed. > >> > Teo replied [2], that he's going to fix it. I thought I had set it >> > "Changes requested", but now I see "Needs Review / ACK". Setting it to >> > "Changes requested". > >> I believe the points you raised are fixed in the v2, on top of this thread. >> Re-applying it on top of master on my side doesn't give me any warnings for >> the brk tests, as I do not cast the result from the libc brk anymore, and >> make check reports existing issues with the name of the function, but no >> style problems that did exist in v1. (I don't mind changing them if you >> want, but they are present on master as well). > >> If you give a quick look at the patch v2 you'll see that indeed there is no >> more (void *)brk(addr) or such that generated the warnings, for example. >> (The syscalls still need it, as they return the break directly rather than >> an error, which is what the libc wrapper does.) > >> I might be missing something, please do tell me if that's the case ! But I >> believe that the v2 _should_ be free of those issues. > > I thought I must have put v1 code into my v2 branch. But the warnings are > actually when compiling on Alpine: > > $ make V=1 > gcc -I../../../../include -I../../../../include > -I../../../../include/old/ -g -O2 -g -O2 -fno-strict-aliasing -pipe > -Wall -W -Wold-style-definition -L../../../../lib brk01.c -lltp -o > brk01 > brk01.c: In function 'verify_brk': > brk01.c:22:27: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > 22 | cur_brk = (void *)tst_syscall(__NR_brk, 0); > | ^ > brk01.c:52:35: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > 52 | cur_brk = (void *)tst_syscall(__NR_brk, new_brk); > | ^ > gcc -I../../../../include -I../../../../include > -I../../../../include/old/ -g -O2 -g -O2 -fno-strict-aliasing -pipe > -Wall -W -Wold-style-definition -L../../../../lib brk02.c -lltp -o > brk02 > brk02.c: In function 'brk_variants': > brk02.c:24:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > 24 | brk_addr = (void *)tst_syscall(__NR_brk, addr); > | ^ > brk02.c: In function 'brk_down_vmas': > brk02.c:38:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > 38 | brk_addr = (void *)tst_syscall(__NR_brk, 0); > | ^ > How is this possible when tst_syscall (now) uses intptr_t?
Hi Petr, Thanks again for taking the time to look into this ! On 12/12/2022 19:28, Petr Vorel wrote: > Hi Teo, > >> Hi Petr, >> On 12/12/2022 15:22, Petr Vorel wrote: >>> Hi Richie, >>>> Cyril, Petr, Would you like to add your reviewed by tags? Then we can >>>> merge this. >>> By accident I reply to my points to v1 [1]. >>> To copy it here: >>> 1) There are warnings: >>> brk02.c: In function ‘brk_variants’: >>> brk02.c:26:28: warning: cast to pointer from integer of different size >>> [-Wint-to-pointer-cast] >>> 26 | brk_addr = (void *)brk(addr); >>> | ^ >>> 2) make check reports errors which are easily fixed. >>> Teo replied [2], that he's going to fix it. I thought I had set it >>> "Changes requested", but now I see "Needs Review / ACK". Setting it to >>> "Changes requested". >> I believe the points you raised are fixed in the v2, on top of this thread. >> Re-applying it on top of master on my side doesn't give me any warnings for >> the brk tests, as I do not cast the result from the libc brk anymore, and >> make check reports existing issues with the name of the function, but no >> style problems that did exist in v1. (I don't mind changing them if you >> want, but they are present on master as well). >> If you give a quick look at the patch v2 you'll see that indeed there is no >> more (void *)brk(addr) or such that generated the warnings, for example. >> (The syscalls still need it, as they return the break directly rather than >> an error, which is what the libc wrapper does.) >> I might be missing something, please do tell me if that's the case ! But I >> believe that the v2 _should_ be free of those issues. > I thought I must have put v1 code into my v2 branch. But the warnings are > actually when compiling on Alpine: > > $ make V=1 > gcc -I../../../../include -I../../../../include -I../../../../include/old/ -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -L../../../../lib brk01.c -lltp -o brk01 > brk01.c: In function 'verify_brk': > brk01.c:22:27: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > 22 | cur_brk = (void *)tst_syscall(__NR_brk, 0); > | ^ > brk01.c:52:35: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > 52 | cur_brk = (void *)tst_syscall(__NR_brk, new_brk); > | ^ > gcc -I../../../../include -I../../../../include -I../../../../include/old/ -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -L../../../../lib brk02.c -lltp -o brk02 > brk02.c: In function 'brk_variants': > brk02.c:24:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > 24 | brk_addr = (void *)tst_syscall(__NR_brk, addr); > | ^ > brk02.c: In function 'brk_down_vmas': > brk02.c:38:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > 38 | brk_addr = (void *)tst_syscall(__NR_brk, 0); > | ^ > > $ gcc -v > Using built-in specs. > COLLECT_GCC=gcc > COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-alpine-linux-musl/12.1.1/lto-wrapper > Target: x86_64-alpine-linux-musl > Configured with: /home/buildozer/aports/main/gcc/src/gcc-12.1.1_git20220630/configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --build=x86_64-alpine-linux-musl --host=x86_64-alpine-linux-musl --target=x86_64-alpine-linux-musl --with-pkgversion='Alpine 12.1.1_git20220630' --enable-checking=release --disable-fixed-point --disable-libstdcxx-pch --disable-multilib --disable-nls --disable-werror --disable-symvers --enable-__cxa_atexit --enable-default-pie --enable-default-ssp --enable-cloog-backend --enable-languages=c,c++,d,objc,go,fortran,ada,jit --disable-libssp --disable-libmpx --disable-libmudflap --disable-libsanitizer --enable-shared --enable-threads --enable-tls --enable-host-shared --with-system-zlib --with-linker-hash-style=gnu > Thread model: posix > Supported LTO compression algorithms: zlib > gcc version 12.1.1 20220630 (Alpine 12.1.1_git20220630) > > $ sudo ./brk01 > tst_test.c:1558: TINFO: Timeout per run is 0h 00m 30s > brk01.c:24: TINFO: Testing libc variant > brk01.c:35: TCONF: brk() not implemented > brk01.c:21: TINFO: Testing syscall variant > brk01.c:59: TFAIL: brk() failed to set address have 0x4d3c4000 expected 0x4d3c5fff > > $ sudo ./brk02 > tst_test.c:1558: TINFO: Timeout per run is 0h 00m 30s > brk02.c:40: TINFO: Testing libc variant > brk02.c:51: TCONF: brk() not implemented > brk02.c:37: TINFO: Testing syscall variant > brk02.c:58: TFAIL: Cannot expand brk() by page size: EFAULT (14) > > Looking on extra brk() support detection, you must have tested it on Alpine. > What am I missing? That is quite strange indeed. As Richard pointed out in his reply to this message, those warnings should not happen anymore since my patch that changed tst_syscall to use intptr_t. ( e5d2a05a90e5 : regen.sh: Use intptr_t for tst_syscall return ) However, I believe that the relevant header is only regenerated when running ./configure , not when building normally. I know that I forgot to do it a few times myself while testing the change to tst_syscall ! To be sure that it is supposed to work, I did the following on an Alpine VM I used for testing : make clean make autotools ./configure And then built the brk tests, with the following results : root# make V=1 gcc -I../../../../include -I../../../../include -I../../../../include/old/ -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -std=gnu99 -L../../../../lib brk01.c -lltp -o brk01 gcc -I../../../../include -I../../../../include -I../../../../include/old/ -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -std=gnu99 -L../../../../lib brk02.c -lltp -o brk02 root# ./brk01 ; ./brk02 tst_test.c:1559: TINFO: Timeout per run is 0h 00m 30s brk01.c:24: TINFO: Testing libc variant brk01.c:35: TCONF: brk() not implemented brk01.c:21: TINFO: Testing syscall variant brk01.c:70: TPASS: brk() works fine [...] tst_test.c:1559: TINFO: Timeout per run is 0h 00m 30s brk02.c:40: TINFO: Testing libc variant brk02.c:51: TCONF: brk() not implemented brk02.c:37: TINFO: Testing syscall variant brk02.c:84: TPASS: munmap at least two VMAs of brk() passed [...] This is with the v2 of the patch applied on top of master ( ca1b0b0cc938: tst_fill_fs: Remove O_FSYNC ). > > Have you also compiled the code on Alpine or musl related distro? > If tested on musl, but not Alpine, what do you use? (maybe Alpine issue?) I usually test on a custom Debian-based musl distribution[1]. It's what we use for porting the linux kernel on Morello[0], where I ran into the issue. When sharing something with upstream I test on Alpine as well, to be sure it's just not our weird environment ! Just to be sure, I ran the steps above on Alpine and on my side I don't seem to have the same issues you observe. > I tested on some old random Alpine (3.17_alpha20220809), I'll try on some > updated version. My Alpine VM seems to be an even older version, 3.16, I can try an updated version as well if you think it's valuable. My GCC is older than yours as well : root# gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-alpine-linux-musl/11.2.1/lto-wrapper Target: x86_64-alpine-linux-musl Configured with: /home/buildozer/aports/main/gcc/src/gcc-11.2.1_git20220219/configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --build=x86_64-alpine-linux-musl --host=x86_64-alpine-linux-musl --target=x86_64-alpine-linux-musl --with-pkgversion='Alpine 11.2.1_git20220219' --enable-checking=release --disable-fixed-point --disable-libstdcxx-pch --disable-multilib --disable-nls --disable-werror --disable-symvers --enable-__cxa_atexit --enable-default-pie --enable-default-ssp --enable-cloog-backend --enable-languages=c,c++,d,objc,go,fortran,ada,jit --disable-libssp --disable-libmpx --disable-libmudflap --disable-libsanitizer --enable-shared --enable-threads --enable-tls --enable-host-shared --with-system-zlib --with-linker-hash-style=gnu Thread model: posix Supported LTO compression algorithms: zlib gcc version 11.2.1 20220219 (Alpine 11.2.1_git20220219) > Also I wonder if we should bother with adding brk() / sbrk() logic into separate > function, added into brk.h. Probably not, although I don't like repeating code, > it's not worth for single function. I did wonder about that too, and reached the same conclusion as you : it seems to be a bit much for a single function. Happy to do it if you change your mind or if another maintainer disagrees ! > > There are still missing static found by 'make check', diff below will fix that > (+ some additional space for readability). Indeed, but as they were missing in the original code I did not know if they should be changed. Thanks for clarifying and for the diff ! > Kind regards, > Petr Best regards, Téo [0]: https://git.morello-project.org/morello/kernel/linux [1]: https://git.morello-project.org/morello/docs/-/blob/morello/mainline/morello-linux-purecap-environment.rst > diff --git testcases/kernel/syscalls/brk/brk01.c testcases/kernel/syscalls/brk/brk01.c > index 93e430328..978e1f211 100644 > --- testcases/kernel/syscalls/brk/brk01.c > +++ testcases/kernel/syscalls/brk/brk01.c > @@ -11,7 +11,7 @@ > #include "tst_test.h" > #include "lapi/syscalls.h" > > -void verify_brk(void) > +static void verify_brk(void) > { > void *cur_brk, *new_brk; > size_t inc = getpagesize() * 2 - 1; > diff --git testcases/kernel/syscalls/brk/brk02.c testcases/kernel/syscalls/brk/brk02.c > index 3a97d279b..64931bc80 100644 > --- testcases/kernel/syscalls/brk/brk02.c > +++ testcases/kernel/syscalls/brk/brk02.c > @@ -5,6 +5,7 @@ > > /*\ > * [Description] > + * > * Expand brk() by at least 2 pages to ensure there is a newly created VMA > * and not expanding the original due to multiple anon pages. mprotect() that > * new VMA then brk() back to the original address therefore causing a munmap of > @@ -16,7 +17,7 @@ > #include "tst_test.h" > #include "lapi/syscalls.h" > > -void *brk_variants(void *addr) > +static void *brk_variants(void *addr) > { > void *brk_addr; > > @@ -26,10 +27,11 @@ void *brk_variants(void *addr) > TST_EXP_PASS_SILENT(brk(addr), "brk()"); > brk_addr = (void *)sbrk(0); > } > + > return brk_addr; > } > > -void brk_down_vmas(void) > +static void brk_down_vmas(void) > { > void *brk_addr; >
Hi Teo, ... > > Looking on extra brk() support detection, you must have tested it on Alpine. > > What am I missing? > That is quite strange indeed. As Richard pointed out in his reply to this > message, those warnings should not happen anymore since my patch that > changed tst_syscall to use intptr_t. ( e5d2a05a90e5 : regen.sh: Use intptr_t > for tst_syscall return ) > However, I believe that the relevant header is only regenerated when running > ./configure , not when building normally. I know that I forgot to do it a > few times myself while testing the change to tst_syscall ! > To be sure that it is supposed to work, I did the following on an Alpine VM > I used for testing : > make clean > make autotools > ./configure I'm sorry to forget basic LTP things. Setup was really old, thus rerunning configure was needed. No more objections from my side, merged with changes I previously posted (to keep checks happy). Thank you! Kind regards, Petr
Hi Petr, On 13/12/2022 19:00, Petr Vorel wrote: > Hi Teo, > > ... >>> Looking on extra brk() support detection, you must have tested it on Alpine. >>> What am I missing? >> That is quite strange indeed. As Richard pointed out in his reply to this >> message, those warnings should not happen anymore since my patch that >> changed tst_syscall to use intptr_t. ( e5d2a05a90e5 : regen.sh: Use intptr_t >> for tst_syscall return ) >> However, I believe that the relevant header is only regenerated when running >> ./configure , not when building normally. I know that I forgot to do it a >> few times myself while testing the change to tst_syscall ! >> To be sure that it is supposed to work, I did the following on an Alpine VM >> I used for testing : >> make clean >> make autotools >> ./configure > I'm sorry to forget basic LTP things. Setup was really old, thus rerunning > configure was needed. > No more objections from my side, merged with changes I previously posted > (to keep checks happy). That makes sense, no worries ! > > Thank you! > > Kind regards, > Petr Thanks for the thorough review and testing ! Kind regards, Téo
diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index a9b89bce5..93e430328 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,14 +9,31 @@ #include <errno.h> #include "tst_test.h" +#include "lapi/syscalls.h" void verify_brk(void) { - uintptr_t cur_brk, new_brk; - uintptr_t inc = getpagesize() * 2 - 1; + void *cur_brk, *new_brk; + size_t inc = getpagesize() * 2 - 1; unsigned int i; - cur_brk = (uintptr_t)sbrk(0); + if (tst_variant) { + tst_res(TINFO, "Testing syscall variant"); + cur_brk = (void *)tst_syscall(__NR_brk, 0); + } else { + tst_res(TINFO, "Testing libc variant"); + cur_brk = (void *)sbrk(0); + + if (cur_brk == (void *)-1) + tst_brk(TCONF, "sbrk() not implemented"); + + /* + * Check if brk itself is implemented: updating to the current break + * should be a no-op. + */ + if (brk(cur_brk) != 0) + tst_brk(TCONF, "brk() not implemented"); + } for (i = 0; i < 33; i++) { switch (i % 3) { @@ -31,16 +48,17 @@ void verify_brk(void) break; } - TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()"); - if (!TST_PASS) - return; - - cur_brk = (uintptr_t)sbrk(0); + if (tst_variant) { + cur_brk = (void *)tst_syscall(__NR_brk, new_brk); + } else { + TST_EXP_PASS_SILENT(brk(new_brk), "brk()"); + cur_brk = sbrk(0); + } 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; } @@ -54,4 +72,5 @@ void verify_brk(void) static struct tst_test test = { .test_all = verify_brk, + .test_variants = 2, }; diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c index 11e803cb4..3a97d279b 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -14,24 +14,53 @@ #include <unistd.h> #include <sys/mman.h> #include "tst_test.h" +#include "lapi/syscalls.h" + +void *brk_variants(void *addr) +{ + void *brk_addr; + + if (tst_variant) { + brk_addr = (void *)tst_syscall(__NR_brk, addr); + } else { + TST_EXP_PASS_SILENT(brk(addr), "brk()"); + brk_addr = (void *)sbrk(0); + } + return brk_addr; +} void brk_down_vmas(void) { - void *brk_addr = sbrk(0); + void *brk_addr; + + if (tst_variant) { + tst_res(TINFO, "Testing syscall variant"); + brk_addr = (void *)tst_syscall(__NR_brk, 0); + } else { + tst_res(TINFO, "Testing libc variant"); + brk_addr = (void *)sbrk(0); - if (brk_addr == (void *) -1) - tst_brk(TBROK, "sbrk() failed"); + if (brk_addr == (void *)-1) + tst_brk(TCONF, "sbrk() not implemented"); + + /* + * Check if brk itself is implemented: updating to the current break + * should be a no-op. + */ + if (brk(brk_addr) != 0) + tst_brk(TCONF, "brk() not implemented"); + } unsigned long page_size = getpagesize(); void *addr = brk_addr + page_size; - if (brk(addr)) { + if (brk_variants(addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size"); return; } addr += page_size; - if (brk(addr)) { + if (brk_variants(addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size"); return; } @@ -42,12 +71,12 @@ void brk_down_vmas(void) } addr += page_size; - if (brk(addr)) { + if (brk_variants(addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect"); return; } - if (brk(brk_addr)) { + if (brk_variants(brk_addr) != brk_addr) { tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address"); return; } @@ -57,4 +86,5 @@ void brk_down_vmas(void) static struct tst_test test = { .test_all = brk_down_vmas, + .test_variants = 2, };
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 exit prematurely. Invoking the syscall directly allows them to properly validate brk behavior. Add a new test variant handling if the libc wrappers are not implemented and testing the direct syscall. Use tst_syscall() and handle the failure cases ourselves, as we don't depend on the libc to do it anymore. The patch also works around the dependency on sbrk to get the current break as it has the same issues. Instead, call tst_syscall(__NR_brk, 0) which always returns the current break. Update brk01 to use void* to unify it with brk02. Signed-off-by: Teo Couprie Diaz <teo.coupriediaz@arm.com> --- v1: Reworked from RFC by adding a variant rather than replacing the existing tests. Thanks Petr and Cyril ! v2: brk doesn't return the new break, just if it error'd or not. Fix styling and warnings. testcases/kernel/syscalls/brk/brk01.c | 37 ++++++++++++++++------ testcases/kernel/syscalls/brk/brk02.c | 44 ++++++++++++++++++++++----- 2 files changed, 65 insertions(+), 16 deletions(-) base-commit: 990c0b48f8508a747863b1dea5556fba57e1c304