[LTP] [RFC PATCH 0/1] brk: use direct syscalls

[LTP] [RFC PATCH 0/1] brk: use direct syscalls

[Teo Couprie Diaz]

Hello LTP maintainers,

This patch slightly reworks the brk01 and brk02 tests to use direct
syscalls with tst_syscall rather than calling through the libc.

While running LTP on a musl-based distribution, we noticed that the brk
tests were failing. It turns out that Musl prevents the use of brk
with their wrapper: it always returns an error.
This isn't too egregious as using brk is deprecated in favor of malloc
and it isn't part of POSIX anymore since POSIX.1-2001, but it _is_
different from glibc's beavior, which allows using it.

This patch allows proper testing of brk's functionality, independent of
libc specifics, and thus allows the tests to pass on Musl-based
distributions like Alpine.

Do you think this is a correct approach for LTP ?
From what I could see there are a few tests that use tst_syscall directly
and it doesn't affect the logic much for brk.

Green build:
https://github.com/Teo-CD/ltp/actions/runs/3563193507

This was discovered in the context of the Morello project[0].
[0]: https://www.morello-project.org/

Teo Couprie Diaz (1):
  syscalls/brk: use direct syscall

 testcases/kernel/syscalls/brk/brk01.c | 15 ++++++---------
 testcases/kernel/syscalls/brk/brk02.c | 14 ++++++--------
 2 files changed, 12 insertions(+), 17 deletions(-)


base-commit: 498247917f40b0a82cb149e2ec1cb518acd7b632
-- 
2.25.1


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

[LTP] [RFC PATCH 1/1] syscalls/brk: use direct syscall

[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.

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
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>
---
 testcases/kernel/syscalls/brk/brk01.c | 15 ++++++---------
 testcases/kernel/syscalls/brk/brk02.c | 14 ++++++--------
 2 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c
index a9b89bce5..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;
-	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);
+	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;
 	}
-- 
2.25.1


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

Re: [LTP] [RFC PATCH 0/1] brk: use direct syscalls

[Petr Vorel]

> Hello LTP maintainers,

> This patch slightly reworks the brk01 and brk02 tests to use direct
> syscalls with tst_syscall rather than calling through the libc.

> While running LTP on a musl-based distribution, we noticed that the brk
> tests were failing. It turns out that Musl prevents the use of brk
> with their wrapper: it always returns an error.
> This isn't too egregious as using brk is deprecated in favor of malloc
> and it isn't part of POSIX anymore since POSIX.1-2001, but it _is_
> different from glibc's beavior, which allows using it.

> This patch allows proper testing of brk's functionality, independent of
> libc specifics, and thus allows the tests to pass on Musl-based
> distributions like Alpine.

> Do you think this is a correct approach for LTP ?
> From what I could see there are a few tests that use tst_syscall directly
> and it doesn't affect the logic much for brk.

LGTM.

I wonder if it makes sense to add .test_variants [1] for glibc and uclibc,
e.g. for brk01():

void verify_brk(void)
{
	if (tst_variant) {
		tst_res(TINFO, "Testing sbrk()");
		cur_brk = (uintptr_t)sbrk(0);
	} else {
		tst_res(TINFO, "Testing __NR_brk");
		cur_brk = (void *)tst_syscall(__NR_brk, 0);
	}

}

struct tst_test test = {
	...
#ifdef __GLIBC__
	.test_variants = 2,
#else
	.test_variants = 1,
#endif
	...

Not sure if it should be testeed also on android, i.e:
#if defined(__GLIBC__) || defined(__ANDROID__)

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/wiki/C-Test-API#130-testing-similar-syscalls-in-one-test

> Green build:
> https://github.com/Teo-CD/ltp/actions/runs/3563193507

> This was discovered in the context of the Morello project[0].
> [0]: https://www.morello-project.org/

> Teo Couprie Diaz (1):
>   syscalls/brk: use direct syscall

>  testcases/kernel/syscalls/brk/brk01.c | 15 ++++++---------
>  testcases/kernel/syscalls/brk/brk02.c | 14 ++++++--------
>  2 files changed, 12 insertions(+), 17 deletions(-)

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

Re: [LTP] [RFC PATCH 0/1] brk: use direct syscalls

[Cyril Hrubis]

Hi!
> void verify_brk(void)
> {
> 	if (tst_variant) {
> 		tst_res(TINFO, "Testing sbrk()");
> 		cur_brk = (uintptr_t)sbrk(0);
> 	} else {
> 		tst_res(TINFO, "Testing __NR_brk");
> 		cur_brk = (void *)tst_syscall(__NR_brk, 0);
> 	}
> 
> }
> 
> struct tst_test test = {
> 	...
> #ifdef __GLIBC__
> 	.test_variants = 2,
> #else
> 	.test_variants = 1,
> #endif
> 	...
> 
> Not sure if it should be testeed also on android, i.e:
> #if defined(__GLIBC__) || defined(__ANDROID__)

Can we rather than ifdefing things out check the actual return values?

Should be as easy as doing:

	if (cur_brk == (void*)-1)
		tst_brk(TCONF, "sbrk() not implemented");

in the libc test variant.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [RFC PATCH 0/1] brk: use direct syscalls

[Petr Vorel]

> Hi!
> > void verify_brk(void)
> > {
> > 	if (tst_variant) {
> > 		tst_res(TINFO, "Testing sbrk()");
> > 		cur_brk = (uintptr_t)sbrk(0);
> > 	} else {
> > 		tst_res(TINFO, "Testing __NR_brk");
> > 		cur_brk = (void *)tst_syscall(__NR_brk, 0);
> > 	}

> > }

> > struct tst_test test = {
> > 	...
> > #ifdef __GLIBC__
> > 	.test_variants = 2,
> > #else
> > 	.test_variants = 1,
> > #endif
> > 	...

> > Not sure if it should be testeed also on android, i.e:
> > #if defined(__GLIBC__) || defined(__ANDROID__)

> Can we rather than ifdefing things out check the actual return values?

> Should be as easy as doing:

> 	if (cur_brk == (void*)-1)
> 		tst_brk(TCONF, "sbrk() not implemented");

> in the libc test variant.

+1.

WDYT about .test_variants? In that case it'd be tested also on musl
or any platform where it's implemented.

Kind regards,
Petr

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

Re: [LTP] [RFC PATCH 0/1] brk: use direct syscalls

[Cyril Hrubis]

Hi!
> WDYT about .test_variants? In that case it'd be tested also on musl
> or any platform where it's implemented.

I would got for it, that way we would test both the kernel implemntation
and that libc does something sensible, e.g. returns error.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [RFC PATCH 0/1] brk: use direct syscalls

[Teo Couprie Diaz]

Hi Cyril and Petr, thanks for having a look and the discussion !

On 29/11/2022 13:40, Cyril Hrubis wrote:
> Hi!
>> WDYT about .test_variants? In that case it'd be tested also on musl
>> or any platform where it's implemented.
> I would got for it, that way we would test both the kernel implemntation
> and that libc does something sensible, e.g. returns error.
>
I'm still quite new to LTP, so I might be understanding things wrong.

My understanding of your conversation is that you're suggesting using 
the .test_variants to have one version of the tests going through the 
libc wrappers as usual, eventually skipping the test with TCONF if the 
libc wrapper does not implement the syscall, and one version which would 
be the the direct syscall I am suggesting in this patch.

Would that be correct ? If so it does make sense to me, I could try 
implementing that.


Best regards,
Téo


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

Re: [LTP] [RFC PATCH 0/1] brk: use direct syscalls

[Cyril Hrubis]

Hi!
> >> WDYT about .test_variants? In that case it'd be tested also on musl
> >> or any platform where it's implemented.
> > I would got for it, that way we would test both the kernel implemntation
> > and that libc does something sensible, e.g. returns error.
> >
> I'm still quite new to LTP, so I might be understanding things wrong.
> 
> My understanding of your conversation is that you're suggesting using 
> the .test_variants to have one version of the tests going through the 
> libc wrappers as usual, eventually skipping the test with TCONF if the 
> libc wrapper does not implement the syscall, and one version which would 
> be the the direct syscall I am suggesting in this patch.
> 
> Would that be correct ? If so it does make sense to me, I could try 
> implementing that.

Yes, exactly this.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [RFC PATCH 0/1] brk: use direct syscalls

[Teo Couprie Diaz]

Hi,

On 30/11/2022 13:15, Cyril Hrubis wrote:
> Hi!
>>>> WDYT about .test_variants? In that case it'd be tested also on musl
>>>> or any platform where it's implemented.
>>> I would got for it, that way we would test both the kernel implemntation
>>> and that libc does something sensible, e.g. returns error.
>>>
>> I'm still quite new to LTP, so I might be understanding things wrong.
>>
>> My understanding of your conversation is that you're suggesting using
>> the .test_variants to have one version of the tests going through the
>> libc wrappers as usual, eventually skipping the test with TCONF if the
>> libc wrapper does not implement the syscall, and one version which would
>> be the the direct syscall I am suggesting in this patch.
>>
>> Would that be correct ? If so it does make sense to me, I could try
>> implementing that.
> Yes, exactly this.

Great, thanks : I will implement this and send a revision when it's done !

Best regards,
Téo


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

Re: [LTP] [RFC PATCH 0/1] brk: use direct syscalls

[Teo Couprie Diaz]

On 28/11/2022 19:30, Petr Vorel wrote:

>> Hello LTP maintainers,
>> This patch slightly reworks the brk01 and brk02 tests to use direct
>> syscalls with tst_syscall rather than calling through the libc.
>> While running LTP on a musl-based distribution, we noticed that the brk
>> tests were failing. It turns out that Musl prevents the use of brk
>> with their wrapper: it always returns an error.
>> This isn't too egregious as using brk is deprecated in favor of malloc
>> and it isn't part of POSIX anymore since POSIX.1-2001, but it _is_
>> different from glibc's beavior, which allows using it.
>> This patch allows proper testing of brk's functionality, independent of
>> libc specifics, and thus allows the tests to pass on Musl-based
>> distributions like Alpine.
>> Do you think this is a correct approach for LTP ?
>>  From what I could see there are a few tests that use tst_syscall directly
>> and it doesn't affect the logic much for brk.
> LGTM.
Thanks for having a look !
>
> I wonder if it makes sense to add .test_variants [1] for glibc and uclibc,
> e.g. for brk01():
>
> void verify_brk(void)
> {
> 	if (tst_variant) {
> 		tst_res(TINFO, "Testing sbrk()");
> 		cur_brk = (uintptr_t)sbrk(0);
> 	} else {
> 		tst_res(TINFO, "Testing __NR_brk");
> 		cur_brk = (void *)tst_syscall(__NR_brk, 0);
> 	}
>
> }

I guess I can see how it could be useful, but LTP already has tests for 
sbrk: should there really be testing for sbrk in the brk tests ?

I will send another reply down the thread regarding the rest of the 
discussion !

Best regards,
Téo

>
> struct tst_test test = {
> 	...
> #ifdef __GLIBC__
> 	.test_variants = 2,
> #else
> 	.test_variants = 1,
> #endif
> 	...
>
> Not sure if it should be testeed also on android, i.e:
> #if defined(__GLIBC__) || defined(__ANDROID__)
>
> Kind regards,
> Petr
>
> [1] https://github.com/linux-test-project/ltp/wiki/C-Test-API#130-testing-similar-syscalls-in-one-test
>
>> Green build:
>> https://github.com/Teo-CD/ltp/actions/runs/3563193507
>> This was discovered in the context of the Morello project[0].
>> [0]: https://www.morello-project.org/
>> Teo Couprie Diaz (1):
>>    syscalls/brk: use direct syscall
>>   testcases/kernel/syscalls/brk/brk01.c | 15 ++++++---------
>>   testcases/kernel/syscalls/brk/brk02.c | 14 ++++++--------
>>   2 files changed, 12 insertions(+), 17 deletions(-)

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