syscalls/memfd_create: test memfd flags separately

PATCH New Jakub Racek 2 participants 3 messages May 30, 2017 – Jun 1, 2017

Share & tools

Jakub Racek

author

2017-05-30 07:59 UTC #0 lore raw

PATCH 1/1 syscalls/memfd_create: test memfd flags separately

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 <[email protected]>

---
 .../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(-)

testcases/kernel/syscalls/memfd_create/memfd_create01.c +15 -1

index 54c817b..c5e76f2 100644
@@ -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 = {

testcases/kernel/syscalls/memfd_create/memfd_create02.c +16 -9

index d65e496..0e616d7 100644
@@ -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);
 }
 
 static struct tst_test test = {

testcases/kernel/syscalls/memfd_create/memfd_create_common.c +38 -6

58 lines not on this page · raw message

testcases/kernel/syscalls/memfd_create/memfd_create_common.h +9 -3

index 6329ac3..c90d1f7 100644
@@ -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,

Signature · 1 line

1.8.3.1

Cyril Hrubis

2017-05-31 10:56 UTC #1 lore raw

PATCH 1/1 syscalls/memfd_create: test memfd flags separately

Hi!

Quoted 84 lines

Signed-off-by Jakub Racek <[email protected]>

---
 .../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(-)

testcases/kernel/syscalls/memfd_create/memfd_create01.c +15 -1

index 54c817b..c5e76f2 100644
@@ -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 = {

testcases/kernel/syscalls/memfd_create/memfd_create02.c +6 -9

index d65e496..0e616d7 100644
@@ -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));

testcases/kernel/syscalls/memfd_create/memfd_create02.c static void verify_memfd_create_errno(unsigned int n) · line 93

Quoted 10 lines

testcases/kernel/syscalls/memfd_create/memfd_create02.c +10 -0

+		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(...));
	...

Quoted 31 lines

testcases/kernel/syscalls/memfd_create/memfd_create02.c +0 -0

 }
 
 static struct tst_test test = {

testcases/kernel/syscalls/memfd_create/memfd_create_common.c +17 -2

index 4cf3bd5..f87c858 100644
@@ -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);

testcases/kernel/syscalls/memfd_create/memfd_create_common.c void check_ftruncate_fail(const char *filename, const int lineno, · line 133

Quoted 10 lines

testcases/kernel/syscalls/memfd_create/memfd_create_common.c +10 -0

+		} 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?

Quoted 6 lines

testcases/kernel/syscalls/memfd_create/memfd_create_common.c +6 -0

+	return flags_available;
+}
+
+int check_mfd_available_with_flags(unsigned int flags)
+{
+	TEST(sys_memfd_create("dummy_call", flags));

testcases/kernel/syscalls/memfd_create/memfd_create_common.c void check_ftruncate_fail(const char *filename, const int lineno, · line 146

Quoted 10 lines

testcases/kernel/syscalls/memfd_create/memfd_create_common.c +2 -4

 	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.

Quoted 44 lines

testcases/kernel/syscalls/memfd_create/memfd_create_common.c +2 -0

 	}
 
 	SAFE_CLOSE(TEST_RETURN);
+
+	return MFD_AVAILABLE;
 }
 
 int check_mfd_new(const char *filename, const int lineno,

testcases/kernel/syscalls/memfd_create/memfd_create_common.h +9 -3

index 6329ac3..c90d1f7 100644
@@ -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,

Signature · 5 lines

1.8.3.1


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

Signature · 2 lines

Cyril Hrubis
[email protected]

Jakub Raček

author

2017-06-01 11:33 UTC #2 lore raw

PATCH 1/1 syscalls/memfd_create: test memfd flags separately

Hi,

Quoted 100 lines · Cyril Hrubis

Hi!

Quoted 84 lines

Signed-off-by Jakub Racek <[email protected]>

---
 .../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(-)

testcases/kernel/syscalls/memfd_create/memfd_create01.c +15 -1

index 54c817b..c5e76f2 100644
@@ -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 = {

testcases/kernel/syscalls/memfd_create/memfd_create02.c +6 -9

index d65e496..0e616d7 100644
@@ -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));

testcases/kernel/syscalls/memfd_create/memfd_create02.c static void verify_memfd_create_errno(unsigned int n) · line 93

Quoted 10 lines

testcases/kernel/syscalls/memfd_create/memfd_create02.c +10 -0

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

Quoted 10 lines

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

	TEST(sys_memfd_create(...));
	...
Quoted 2 lines
testcases/kernel/syscalls/memfd_create/memfd_create02.c +0 -0
 }
 

Good catch, this actually has to be changed even more than that since memfd_create02 tests how api responds to invalid flags.

That and all other suggestions are reflected in V2 I'm sending now.

Quoted 111 lines

Quoted 29 lines
testcases/kernel/syscalls/memfd_create/memfd_create02.c +0 -0
 static struct tst_test test = {
testcases/kernel/syscalls/memfd_create/memfd_create_common.c +17 -2
index 4cf3bd5..f87c858 100644
@@ -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);
testcases/kernel/syscalls/memfd_create/memfd_create_common.c void check_ftruncate_fail(const char *filename, const int lineno, · line 133
Quoted 10 lines
testcases/kernel/syscalls/memfd_create/memfd_create_common.c +10 -0
+		} 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?
Quoted 6 lines
testcases/kernel/syscalls/memfd_create/memfd_create_common.c +6 -0
+	return flags_available;
+}
+
+int check_mfd_available_with_flags(unsigned int flags)
+{
+	TEST(sys_memfd_create("dummy_call", flags));
testcases/kernel/syscalls/memfd_create/memfd_create_common.c void check_ftruncate_fail(const char *filename, const int lineno, · line 146
Quoted 10 lines
testcases/kernel/syscalls/memfd_create/memfd_create_common.c +2 -4
 	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.
Quoted 37 lines
testcases/kernel/syscalls/memfd_create/memfd_create_common.c +2 -0
 	}
 
 	SAFE_CLOSE(TEST_RETURN);
+
+	return MFD_AVAILABLE;
 }
 
 int check_mfd_new(const char *filename, const int lineno,
testcases/kernel/syscalls/memfd_create/memfd_create_common.h +9 -3
index 6329ac3..c90d1f7 100644
@@ -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);
 

Quoted 8 lines

Quoted 7 lines
testcases/kernel/syscalls/memfd_create/memfd_create_common.h +0 -1
 int check_fallocate(const char *filename, const int lineno, int fd,
--
1.8.3.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp

Thank you.

Signature · 2 lines

Regards,
     Jakub