[LTP] [PATCH v2] syscalls/brk: add direct syscall tst_variant

[LTP] [PATCH v2] syscalls/brk: add direct syscall tst_variant

[Teo Couprie Diaz]

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


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/brk: add direct syscall tst_variant

[Richard Palethorpe]

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


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/brk: add direct syscall tst_variant

[Petr Vorel]

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/

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/brk: add direct syscall tst_variant

[Teo Couprie Diaz]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/brk: add direct syscall tst_variant

[Petr Vorel]

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;
 

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/brk: add direct syscall tst_variant

[Richard Palethorpe]

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?

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/brk: add direct syscall tst_variant

[Teo Couprie Diaz]

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;
>   

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/brk: add direct syscall tst_variant

[Petr Vorel]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/brk: add direct syscall tst_variant

[Teo Couprie Diaz]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp