[LTP] [PATCH] syscalls/memfd_create: test memfd flags separately

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> This patch fixes the test on some kernels where memfd_create doesn't
> have  have sealing support. Calls to memfd_create are done so that we
> know what flags are available. Depending on that, some tests are skipped
> instead of incorrectly reporting breakage.
> 
> Signed-off-by: Jakub Racek <jracek@redhat.com>
> ---
>  .../kernel/syscalls/memfd_create/memfd_create01.c  | 16 ++++++-
>  .../kernel/syscalls/memfd_create/memfd_create02.c  | 13 +++++-
>  .../syscalls/memfd_create/memfd_create_common.c    | 50 +++++++++++++++++++---
>  .../syscalls/memfd_create/memfd_create_common.h    | 19 ++++++--
>  4 files changed, 86 insertions(+), 12 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/memfd_create/memfd_create01.c b/testcases/kernel/syscalls/memfd_create/memfd_create01.c
> index 54c817b..c5e76f2 100644
> --- a/testcases/kernel/syscalls/memfd_create/memfd_create01.c
> +++ b/testcases/kernel/syscalls/memfd_create/memfd_create01.c
> @@ -22,6 +22,7 @@
>  
>  #define _GNU_SOURCE
>  
> +#include <errno.h>
>  #include "tst_test.h"
>  #include "memfd_create_common.h"
>  
> @@ -259,7 +260,20 @@ static void verify_memfd_create(unsigned int n)
>  
>  static void setup(void)
>  {
> -	ASSERT_HAVE_MEMFD_CREATE();
> +
> +	int available_flags;
> +
> +	/*
> +	 * For now, all tests in this file require MFD_ALLOW_SEALING flag
> +	 * to be implemented, even though that flag isn't always set when
> +	 * memfd is created. So don't check anything else and TCONF right away
> +	 * is this flag is missing.
> +	 */
> +	available_flags = GET_MFD_ALL_AVAILABLE_FLAGS();
> +	if ((available_flags & MFD_ALLOW_SEALING) != MFD_ALLOW_SEALING) {
> +		tst_brk(TCONF | TTERRNO,
> +			"memfd_create(%u) not implemented", MFD_ALLOW_SEALING);
> +	}

Can't we call the IS_MFD_AVAILABLE_WITH_FLAGS(flag) here directly
instead? That would be more straightforward way to figure out if we
support sealing.

Also better name would be MFD_FLAGS_AVAILABLE() or something and the
function name to get all available flags should be shorther and to the
point as well, maybe MFD_ALL_AVAILABLE_FLAGS().

>  }
>  
>  static struct tst_test test = {
> diff --git a/testcases/kernel/syscalls/memfd_create/memfd_create02.c b/testcases/kernel/syscalls/memfd_create/memfd_create02.c
> index d65e496..bc01e44 100644
> --- a/testcases/kernel/syscalls/memfd_create/memfd_create02.c
> +++ b/testcases/kernel/syscalls/memfd_create/memfd_create02.c
> @@ -31,6 +31,8 @@
>  static char buf[2048];
>  static char term_buf[2048];
>  
> +static int available_flags;
> +
>  static const struct tcase {
>  	char *descr;
>  	char *memfd_name;
> @@ -62,7 +64,8 @@ static const struct tcase {
>  
>  static void setup(void)
>  {
> -	ASSERT_HAVE_MEMFD_CREATE();
> +
> +	available_flags = GET_MFD_ALL_AVAILABLE_FLAGS();
>  
>  	memset(buf, 0xff, sizeof(buf));
>  
> @@ -73,8 +76,16 @@ static void setup(void)
>  static void verify_memfd_create_errno(unsigned int n)
>  {
>  	const struct tcase *tc;
> +	int needed_flags;
>  
>  	tc = &tcases[n];
> +	needed_flags = tc->flags & FLAGS_ALL_MASK;
> +
> +	if ((available_flags & needed_flags) != needed_flags) {
> +		tst_res(TCONF, "test '%s' skipped, flag not implemented",
> +					tc->descr);
> +		return;
> +	}
>  
>  	TEST(sys_memfd_create(tc->memfd_name, tc->flags));
>  	if (TEST_ERRNO != tc->memfd_create_exp_err)
> diff --git a/testcases/kernel/syscalls/memfd_create/memfd_create_common.c b/testcases/kernel/syscalls/memfd_create/memfd_create_common.c
> index 4cf3bd5..939b046 100644
> --- a/testcases/kernel/syscalls/memfd_create/memfd_create_common.c
> +++ b/testcases/kernel/syscalls/memfd_create/memfd_create_common.c
> @@ -103,20 +103,56 @@ void check_ftruncate_fail(const char *filename, const int lineno,
>  		"ftruncate(%d, %ld) failed as expected", fd, length);
>  }
>  
> -void assert_have_memfd_create(const char *filename, const int lineno)
> +int get_mfd_all_available_flags(const char *filename, const int lineno)
>  {
> -	TEST(sys_memfd_create("dummy_call", 0));
> +	unsigned int i;
> +	int flag;
> +	int flags2test[] = FLAGS_ALL_ARRAY_INITIALIZER;
> +	int flags_available = 0;
> +	int any_available = 0;
> +
> +	if (!IS_MFD_AVAILABLE_WITH_FLAGS(0)) {
> +		tst_brk_(filename, lineno, TCONF,
> +				"memfd_create(0) not implemented");
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(flags2test); i++) {
> +		flag = flags2test[i];
> +
> +		TEST(IS_MFD_AVAILABLE_WITH_FLAGS(flag));

Since we check errno in the is_mfd_...() function and call tst_brk_() on
failure there is no point in doing anything else here than:

		if (is_mfd_...(flag))
			flags_available |= flag;

> +		if (!TEST_RETURN) {
> +			tst_res_(filename, lineno, TINFO | TTERRNO,
> +				"memfd_create(%u) not implemented", flag);
> +		} else {
> +			flags_available |= flag;
> +			any_available = 1;
> +		}
> +	}
> +
> +	if (!any_available) {
> +		tst_brk_(filename, lineno, TCONF,
> +				"memfd_create() not implemented at all");
> +	}

Isn't this check useless now? Since we assert that at least 0 passed as
flags is supported at the start of this function we should just return
flags_available here which would be 0 in this case.

> +	return flags_available;
> +}
> +
> +int is_mfd_available_with_flags(const char *filename, const int lineno,
> +		unsigned int flags)
> +{
> +	TEST(sys_memfd_create("dummy_call", flags));
>  	if (TEST_RETURN < 0) {
> -		if (TEST_ERRNO == EINVAL) {
> -			tst_brk_(filename, lineno, TCONF | TTERRNO,
> -				"memfd_create() not implemented");
> +		if (TEST_ERRNO != EINVAL) {
> +			tst_brk_(filename, lineno, TBROK | TTERRNO,
> +					"memfd_create() failed");
>  		}
>  
> -		tst_brk_(filename, lineno, TBROK | TTERRNO,
> -			"memfd_create() failed");
> +		return 0;
>  	}
>  
>  	SAFE_CLOSE(TEST_RETURN);
> +
> +	return 1;
>  }
>  
>  int check_mfd_new(const char *filename, const int lineno,
> diff --git a/testcases/kernel/syscalls/memfd_create/memfd_create_common.h b/testcases/kernel/syscalls/memfd_create/memfd_create_common.h
> index 6329ac3..616c7ef 100644
> --- a/testcases/kernel/syscalls/memfd_create/memfd_create_common.h
> +++ b/testcases/kernel/syscalls/memfd_create/memfd_create_common.h
> @@ -20,10 +20,19 @@
>  #include <lapi/fcntl.h>
>  #include <lapi/memfd.h>
>  
> +#define COMMA 					,
> +#define BITWISE_OR 				|
> +#define OPERATE_ON_FLAGS(x)			MFD_CLOEXEC x MFD_ALLOW_SEALING
> +#define FLAGS_ALL_ARRAY_INITIALIZER		{OPERATE_ON_FLAGS(COMMA)}
> +#define FLAGS_ALL_MASK				(OPERATE_ON_FLAGS(BITWISE_OR))

Really?

Do you realize that just defining the FLAGS_ALL_MASK and array
initializer directly as MFD_CLOEXEC | MFD_ALLOW_SEALING and
{MFD_CLOEXEC, MFD_ALLOW_SEALING} is not only less complex but shorther
as well?

What do we need this macro obscurity for?

>  #define MFD_DEF_SIZE 8192
>  
> -#define ASSERT_HAVE_MEMFD_CREATE() \
> -	assert_have_memfd_create(__FILE__, __LINE__)
> +#define GET_MFD_ALL_AVAILABLE_FLAGS() \
> +	get_mfd_all_available_flags(__FILE__, __LINE__)
> +
> +#define IS_MFD_AVAILABLE_WITH_FLAGS(flags) \
> +	is_mfd_available_with_flags(__FILE__, __LINE__, (flags))
>  
>  #define CHECK_MFD_NEW(name, sz, flags) \
>  	check_mfd_new(__FILE__, __LINE__, (name), (sz), (flags))
> @@ -89,7 +98,11 @@
>  #define CHECK_MFD_NON_GROWABLE_BY_WRITE(fd) \
>  	check_mfd_non_growable_by_write(__FILE__, __LINE__, (fd))
>  
> -void assert_have_memfd_create(const char *filename, const int lineno);
> +int is_mfd_available_with_flags(const char *filename, const int lineno,
> +		unsigned int flags);
> +
> +int get_mfd_all_available_flags(const char *filename, const int lineno);
> +
>  int sys_memfd_create(const char *name, unsigned int flags);
>  
>  int check_fallocate(const char *filename, const int lineno, int fd,
> -- 
> 1.8.3.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz