[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  | 25 +++++++-----
>  .../syscalls/memfd_create/memfd_create_common.c    | 44 +++++++++++++++++++---
>  .../syscalls/memfd_create/memfd_create_common.h    | 12 ++++--
>  4 files changed, 78 insertions(+), 19 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);
> +	}
>  }
>  
>  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..0e616d7 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));
>  
> @@ -76,14 +79,18 @@ static void verify_memfd_create_errno(unsigned int n)
>  
>  	tc = &tcases[n];
>  
> -	TEST(sys_memfd_create(tc->memfd_name, tc->flags));
> -	if (TEST_ERRNO != tc->memfd_create_exp_err)
> -		tst_brk(TFAIL, "test '%s'", tc->descr);
> -	else
> -		tst_res(TPASS, "test '%s'", tc->descr);
> -
> -	if (TEST_RETURN > 0)
> -		SAFE_CLOSE(TEST_RETURN);
> +	if ((available_flags & tc->flags) == tc->flags) {
> +		TEST(sys_memfd_create(tc->memfd_name, tc->flags));
> +		if (tc->memfd_create_exp_err != TEST_ERRNO)
> +			tst_brk(TFAIL, "test '%s'", tc->descr);
> +		else
> +			tst_res(TPASS, "test '%s'", tc->descr);
> +
> +		if (TEST_RETURN > 0)
> +			SAFE_CLOSE(TEST_RETURN);
> +	} else
> +		tst_res(TCONF, "test '%s' skipped, flag not implemented",
> +			tc->descr);

It would be a bit cleaner to check for the flags first and return from
the function if flag is not available, i.e.


	if ((available_flags & tc->flags) != tc->flags) {
		tst_res(TCONF, ...);
		return;
	}

	TEST(sys_memfd_create(...));
	...

>  }
>  
>  static struct tst_test test = {
> diff --git a/testcases/kernel/syscalls/memfd_create/memfd_create_common.c b/testcases/kernel/syscalls/memfd_create/memfd_create_common.c
> index 4cf3bd5..f87c858 100644
> --- a/testcases/kernel/syscalls/memfd_create/memfd_create_common.c
> +++ b/testcases/kernel/syscalls/memfd_create/memfd_create_common.c
> @@ -103,20 +103,52 @@ 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[] = {0, MFD_CLOEXEC,  MFD_ALLOW_SEALING};
> +	int flags_available = 0;
> +	int any_available = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(flags2test); i++) {
> +		flag = flags2test[i];
> +
> +		TEST(check_mfd_available_with_flags(flag));
> +		if (TEST_RETURN == MFD_FLAG_NOT_IMPLEMENTED) {
> +			tst_res_(filename, lineno, TINFO | TTERRNO,
> +				"memfd_create(%u) not implemented", flag);
> +		} else if (TEST_RETURN == MFD_FAILED) {
> +			tst_res_(filename, lineno, TINFO | TTERRNO,
> +					"memfd_create(%u) failed", flag);
> +		} else {
> +			flags_available |= flag;
> +			any_available = 1;
> +		}
> +	}
> +
> +	if (!any_available) {
> +		tst_brk_(filename, lineno, TCONF,
> +				"memfd_create() not implemented at all");
> +	}

Hmm, I wonder if there could be a situation where memfd_create() with
flags = 0 will fail but memfd_creat() with non-zero flags will not. I
doubt so but as the code is that will create a situaion where tests
with flags = 0 will fail as well.

So what about checking for flags=0 first, doing tst_brk(TCONF, ...) if
that is not working, then loop over the MFD_CLOEXEC and
MFD_ALLOW_SEALING flags?

> +	return flags_available;
> +}
> +
> +int check_mfd_available_with_flags(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");
> +			return MFD_FLAG_NOT_IMPLEMENTED;
>  		}
>  
> -		tst_brk_(filename, lineno, TBROK | TTERRNO,
> -			"memfd_create() failed");
> +		return MFD_FAILED;

I would argue that if the mfd_create() has failed unexpectedly here the
right thing to do is tst_brk(TBROK | TTERRNO, ...), there is no point in
continuing the test if the syscall is failing for unknown reason, in
that case the best course of action is to die quickly with a good
description of the problem. Which also means that there is no point in
returning anything else than 0 and 1 in this function, just return 0 if
flags are not available and 1 if they are.

>  	}
>  
>  	SAFE_CLOSE(TEST_RETURN);
> +
> +	return MFD_AVAILABLE;
>  }
>  
>  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..c90d1f7 100644
> --- a/testcases/kernel/syscalls/memfd_create/memfd_create_common.h
> +++ b/testcases/kernel/syscalls/memfd_create/memfd_create_common.h
> @@ -22,8 +22,12 @@
>  
>  #define MFD_DEF_SIZE 8192
>  
> -#define ASSERT_HAVE_MEMFD_CREATE() \
> -	assert_have_memfd_create(__FILE__, __LINE__)
> +#define MFD_AVAILABLE            0
> +#define MFD_FAILED               1
> +#define MFD_FLAG_NOT_IMPLEMENTED 2
> +
> +#define GET_MFD_ALL_AVAILABLE_FLAGS() \
> +	get_mfd_all_available_flags(__FILE__, __LINE__)
>  
>  #define CHECK_MFD_NEW(name, sz, flags) \
>  	check_mfd_new(__FILE__, __LINE__, (name), (sz), (flags))
> @@ -89,7 +93,9 @@
>  #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 check_mfd_available_with_flags(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