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

[Richard Palethorpe <rpalethorpe@suse.de>]

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