[PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Jakub Kicinski]

Hi!

When running selftests for our subsystem in our CI we'd like all
tests to pass. Currently some tests use SKIP for cases they
expect to fail, because the kselftest_harness limits the return
codes to pass/fail/skip. XFAIL which would be a great match
here cannot be used.

Remove the no_print handling and use vfork() to run the test in
a different process than the setup. This way we don't need to
pass "failing step" via the exit code. Further clean up the exit
codes so that we can use all KSFT_* values. Rewrite the result
printing to make handling XFAIL/XPASS easier. Support tests
declaring combinations of fixture + variant they expect to fail.


Merge plan is to put it on top of -rc6 and merge into net-next.
That way others should be able to pull the patches without
any networking changes.

v4:
 - rebase on top of Mickael's vfork() changes
v3: https://lore.kernel.org/all/20240220192235.2953484-1-kuba@kernel.org/
 - combine multiple series
 - change to "list of expected failures" rather than SKIP()-like handling
v2: https://lore.kernel.org/all/20240216002619.1999225-1-kuba@kernel.org/
 - fix alignment
follow up RFC: https://lore.kernel.org/all/20240216004122.2004689-1-kuba@kernel.org/
v1: https://lore.kernel.org/all/20240213154416.422739-1-kuba@kernel.org/

Jakub Kicinski (10):
  selftests: kselftest_harness: use KSFT_* exit codes
  selftests: kselftest_harness: generate test name once
  selftests: kselftest_harness: save full exit code in metadata
  selftests: kselftest_harness: use exit code to store skip
  selftests: kselftest: add ksft_test_result_code(), handling all exit
    codes
  selftests: kselftest_harness: print test name for SKIP
  selftests: kselftest_harness: separate diagnostic message with # in
    ksft_test_result_code()
  selftests: kselftest_harness: let PASS / FAIL provide diagnostic
  selftests: kselftest_harness: support using xfail
  selftests: ip_local_port_range: use XFAIL instead of SKIP

Mickaël Salaün (2):
  selftests/landlock: Redefine TEST_F() as TEST_F_FORK()
  selftests/harness: Merge TEST_F_FORK() into TEST_F()

 tools/testing/selftests/kselftest.h           |  45 +++++
 tools/testing/selftests/kselftest_harness.h   | 182 +++++++++++-------
 tools/testing/selftests/landlock/base_test.c  |   2 +-
 tools/testing/selftests/landlock/common.h     |  58 +-----
 tools/testing/selftests/landlock/fs_test.c    |   4 +-
 tools/testing/selftests/landlock/net_test.c   |   4 +-
 .../testing/selftests/landlock/ptrace_test.c  |   7 +-
 .../selftests/net/ip_local_port_range.c       |   6 +-
 tools/testing/selftests/net/tls.c             |   2 +-
 tools/testing/selftests/seccomp/seccomp_bpf.c |   9 +-
 10 files changed, 178 insertions(+), 141 deletions(-)

-- 
2.43.2

[PATCH v4 01/12] selftests/landlock: Redefine TEST_F() as TEST_F_FORK()

[Jakub Kicinski]

From: Mickaël Salaün <mic@digikod.net>

This has the effect of creating a new test process for either TEST_F()
or TEST_F_FORK(), which doesn't change tests but will ease potential
backports.  See next commit for the TEST_F_FORK() merge into TEST_F().

Cc: Günther Noack <gnoack@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Will Drewry <wad@chromium.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/landlock/common.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
index e64bbdf0e86e..f40146d40763 100644
--- a/tools/testing/selftests/landlock/common.h
+++ b/tools/testing/selftests/landlock/common.h
@@ -37,7 +37,7 @@
 		struct __test_metadata *_metadata, \
 		FIXTURE_DATA(fixture_name) *self, \
 		const FIXTURE_VARIANT(fixture_name) *variant); \
-	TEST_F(fixture_name, test_name) \
+	__TEST_F_IMPL(fixture_name, test_name, -1, TEST_TIMEOUT_DEFAULT) \
 	{ \
 		int status; \
 		const pid_t child = fork(); \
@@ -80,6 +80,10 @@
 			__attribute__((unused)) *variant)
 /* clang-format on */
 
+/* Makes backporting easier. */
+#undef TEST_F
+#define TEST_F(fixture_name, test_name) TEST_F_FORK(fixture_name, test_name)
+
 #ifndef landlock_create_ruleset
 static inline int
 landlock_create_ruleset(const struct landlock_ruleset_attr *const attr,
-- 
2.43.2

[PATCH v4 02/12] selftests/harness: Merge TEST_F_FORK() into TEST_F()

[Jakub Kicinski]

From: Mickaël Salaün <mic@digikod.net>

Replace Landlock-specific TEST_F_FORK() with an improved TEST_F() which
brings four related changes:

Run TEST_F()'s tests in a grandchild process to make it possible to
drop privileges and delegate teardown to the parent.

Compared to TEST_F_FORK(), simplify handling of the test grandchild
process thanks to vfork(2), and makes it generic (e.g. no explicit
conversion between exit code and _metadata).

Compared to TEST_F_FORK(), run teardown even when tests failed with an
assert thanks to commit 63e6b2a42342 ("selftests/harness: Run TEARDOWN
for ASSERT failures").

Simplify the test harness code by removing the no_print and step fields
which are not used.  I added this feature just after I made
kselftest_harness.h more broadly available but this step counter
remained even though it wasn't needed after all. See commit 369130b63178
("selftests: Enhance kselftest_harness.h to print which assert failed").

Replace spaces with tabs in one line of __TEST_F_IMPL().

Cc: Günther Noack <gnoack@google.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Will Drewry <wad@chromium.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
v4:
 - GAND -> GRAND
 - init child to 1, otherwise assert in setup triggers a longjmp
   which in turn reads child without it ever getting initialized
   (or being 0, i.e. we mistakenly assume we're in the grandchild)
---
 tools/testing/selftests/kselftest_harness.h | 56 +++++++++----------
 tools/testing/selftests/landlock/common.h   | 62 +--------------------
 2 files changed, 27 insertions(+), 91 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index e05ac8261046..ad49832457af 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -95,14 +95,6 @@
  * E.g., #define TH_LOG_ENABLED 1
  *
  * If no definition is provided, logging is enabled by default.
- *
- * If there is no way to print an error message for the process running the
- * test (e.g. not allowed to write to stderr), it is still possible to get the
- * ASSERT_* number for which the test failed.  This behavior can be enabled by
- * writing `_metadata->no_print = true;` before the check sequence that is
- * unable to print.  When an error occur, instead of printing an error message
- * and calling `abort(3)`, the test process call `_exit(2)` with the assert
- * number as argument, which is then printed by the parent process.
  */
 #define TH_LOG(fmt, ...) do { \
 	if (TH_LOG_ENABLED) \
@@ -363,6 +355,11 @@
  * Defines a test that depends on a fixture (e.g., is part of a test case).
  * Very similar to TEST() except that *self* is the setup instance of fixture's
  * datatype exposed for use by the implementation.
+ *
+ * The @test_name code is run in a separate process sharing the same memory
+ * (i.e. vfork), which means that the test process can update its privileges
+ * without impacting the related FIXTURE_TEARDOWN() (e.g. to remove files from
+ * a directory where write access was dropped).
  */
 #define TEST_F(fixture_name, test_name) \
 	__TEST_F_IMPL(fixture_name, test_name, -1, TEST_TIMEOUT_DEFAULT)
@@ -384,15 +381,28 @@
 	{ \
 		/* fixture data is alloced, setup, and torn down per call. */ \
 		FIXTURE_DATA(fixture_name) self; \
+		pid_t child = 1; \
 		memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
 		if (setjmp(_metadata->env) == 0) { \
 			fixture_name##_setup(_metadata, &self, variant->data); \
 			/* Let setup failure terminate early. */ \
-                       if (!_metadata->passed || _metadata->skip) \
+			if (!_metadata->passed || _metadata->skip) \
 				return; \
 			_metadata->setup_completed = true; \
-			fixture_name##_##test_name(_metadata, &self, variant->data); \
+			/* Use the same _metadata. */ \
+			child = vfork(); \
+			if (child == 0) { \
+				fixture_name##_##test_name(_metadata, &self, variant->data); \
+				_exit(0); \
+			} \
+			if (child < 0) { \
+				ksft_print_msg("ERROR SPAWNING TEST GRANDCHILD\n"); \
+				_metadata->passed = 0; \
+			} \
 		} \
+		if (child == 0) \
+			/* Child failed and updated the shared _metadata. */ \
+			_exit(0); \
 		if (_metadata->setup_completed) \
 			fixture_name##_teardown(_metadata, &self, variant->data); \
 		__test_check_assert(_metadata); \
@@ -694,18 +704,12 @@
 	for (; _metadata->trigger; _metadata->trigger = \
 			__bail(_assert, _metadata))
 
-#define __INC_STEP(_metadata) \
-	/* Keep "step" below 255 (which is used for "SKIP" reporting). */	\
-	if (_metadata->passed && _metadata->step < 253) \
-		_metadata->step++;
-
 #define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
 
 #define __EXPECT(_expected, _expected_str, _seen, _seen_str, _t, _assert) do { \
 	/* Avoid multiple evaluation of the cases */ \
 	__typeof__(_expected) __exp = (_expected); \
 	__typeof__(_seen) __seen = (_seen); \
-	if (_assert) __INC_STEP(_metadata); \
 	if (!(__exp _t __seen)) { \
 		/* Report with actual signedness to avoid weird output. */ \
 		switch (is_signed_type(__exp) * 2 + is_signed_type(__seen)) { \
@@ -751,7 +755,6 @@
 #define __EXPECT_STR(_expected, _seen, _t, _assert) do { \
 	const char *__exp = (_expected); \
 	const char *__seen = (_seen); \
-	if (_assert) __INC_STEP(_metadata); \
 	if (!(strcmp(__exp, __seen) _t 0))  { \
 		__TH_LOG("Expected '%s' %s '%s'.", __exp, #_t, __seen); \
 		_metadata->passed = 0; \
@@ -837,8 +840,6 @@ struct __test_metadata {
 	int trigger; /* extra handler after the evaluation */
 	int timeout;	/* seconds to wait for test timeout */
 	bool timed_out;	/* did this test timeout instead of exiting? */
-	__u8 step;
-	bool no_print; /* manual trigger when TH_LOG_STREAM is not available */
 	bool aborted;	/* stopped test due to failed ASSERT */
 	bool setup_completed; /* did setup finish? */
 	jmp_buf env;	/* for exiting out of test early */
@@ -873,11 +874,8 @@ static inline int __bail(int for_realz, struct __test_metadata *t)
 
 static inline void __test_check_assert(struct __test_metadata *t)
 {
-	if (t->aborted) {
-		if (t->no_print)
-			_exit(t->step);
+	if (t->aborted)
 		abort();
-	}
 }
 
 struct __test_metadata *__active_test;
@@ -954,13 +952,12 @@ void __wait_for_test(struct __test_metadata *t)
 			case 0:
 				t->passed = 1;
 				break;
-			/* Other failure, assume step report. */
+			/* Failure */
 			default:
 				t->passed = 0;
 				fprintf(TH_LOG_STREAM,
-					"# %s: Test failed at step #%d\n",
-					t->name,
-					WEXITSTATUS(status));
+					"# %s: Test failed\n",
+					t->name);
 			}
 		}
 	} else if (WIFSIGNALED(status)) {
@@ -1114,8 +1111,6 @@ void __run_test(struct __fixture_metadata *f,
 	t->passed = 1;
 	t->skip = 0;
 	t->trigger = 0;
-	t->step = 1;
-	t->no_print = 0;
 	memset(t->results->reason, 0, sizeof(t->results->reason));
 
 	ksft_print_msg(" RUN           %s%s%s.%s ...\n",
@@ -1137,8 +1132,7 @@ void __run_test(struct __fixture_metadata *f,
 		/* Pass is exit 0 */
 		if (t->passed)
 			_exit(0);
-		/* Something else happened, report the step. */
-		_exit(t->step);
+		_exit(1);
 	} else {
 		__wait_for_test(t);
 	}
diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
index f40146d40763..401e2eb092a3 100644
--- a/tools/testing/selftests/landlock/common.h
+++ b/tools/testing/selftests/landlock/common.h
@@ -23,66 +23,8 @@
 #define __maybe_unused __attribute__((__unused__))
 #endif
 
-/*
- * TEST_F_FORK() is useful when a test drop privileges but the corresponding
- * FIXTURE_TEARDOWN() requires them (e.g. to remove files from a directory
- * where write actions are denied).  For convenience, FIXTURE_TEARDOWN() is
- * also called when the test failed, but not when FIXTURE_SETUP() failed.  For
- * this to be possible, we must not call abort() but instead exit smoothly
- * (hence the step print).
- */
-/* clang-format off */
-#define TEST_F_FORK(fixture_name, test_name) \
-	static void fixture_name##_##test_name##_child( \
-		struct __test_metadata *_metadata, \
-		FIXTURE_DATA(fixture_name) *self, \
-		const FIXTURE_VARIANT(fixture_name) *variant); \
-	__TEST_F_IMPL(fixture_name, test_name, -1, TEST_TIMEOUT_DEFAULT) \
-	{ \
-		int status; \
-		const pid_t child = fork(); \
-		if (child < 0) \
-			abort(); \
-		if (child == 0) { \
-			_metadata->no_print = 1; \
-			fixture_name##_##test_name##_child(_metadata, self, variant); \
-			if (_metadata->skip) \
-				_exit(255); \
-			if (_metadata->passed) \
-				_exit(0); \
-			_exit(_metadata->step); \
-		} \
-		if (child != waitpid(child, &status, 0)) \
-			abort(); \
-		if (WIFSIGNALED(status) || !WIFEXITED(status)) { \
-			_metadata->passed = 0; \
-			_metadata->step = 1; \
-			return; \
-		} \
-		switch (WEXITSTATUS(status)) { \
-		case 0: \
-			_metadata->passed = 1; \
-			break; \
-		case 255: \
-			_metadata->passed = 1; \
-			_metadata->skip = 1; \
-			break; \
-		default: \
-			_metadata->passed = 0; \
-			_metadata->step = WEXITSTATUS(status); \
-			break; \
-		} \
-	} \
-	static void fixture_name##_##test_name##_child( \
-		struct __test_metadata __attribute__((unused)) *_metadata, \
-		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \
-		const FIXTURE_VARIANT(fixture_name) \
-			__attribute__((unused)) *variant)
-/* clang-format on */
-
-/* Makes backporting easier. */
-#undef TEST_F
-#define TEST_F(fixture_name, test_name) TEST_F_FORK(fixture_name, test_name)
+/* TEST_F_FORK() should not be used for new tests. */
+#define TEST_F_FORK(fixture_name, test_name) TEST_F(fixture_name, test_name)
 
 #ifndef landlock_create_ruleset
 static inline int
-- 
2.43.2

[PATCH v4 03/12] selftests: kselftest_harness: use KSFT_* exit codes

[Jakub Kicinski]

Now that we no longer need low exit codes to communicate
assertion steps - use normal KSFT exit codes.

Acked-by: Kees Cook <keescook@chromium.org>
Tested-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: keescook@chromium.org
CC: luto@amacapital.net
CC: wad@chromium.org
CC: shuah@kernel.org
CC: mic@digikod.net
CC: linux-kselftest@vger.kernel.org
CC: linux-security-module@vger.kernel.org
---
 tools/testing/selftests/kselftest_harness.h | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index ad49832457af..62ce258b0853 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -936,7 +936,7 @@ void __wait_for_test(struct __test_metadata *t)
 		fprintf(TH_LOG_STREAM,
 			"# %s: Test terminated by timeout\n", t->name);
 	} else if (WIFEXITED(status)) {
-		if (WEXITSTATUS(status) == 255) {
+		if (WEXITSTATUS(status) == KSFT_SKIP) {
 			/* SKIP */
 			t->passed = 1;
 			t->skip = 1;
@@ -949,7 +949,7 @@ void __wait_for_test(struct __test_metadata *t)
 		} else {
 			switch (WEXITSTATUS(status)) {
 			/* Success */
-			case 0:
+			case KSFT_PASS:
 				t->passed = 1;
 				break;
 			/* Failure */
@@ -1128,11 +1128,10 @@ void __run_test(struct __fixture_metadata *f,
 		setpgrp();
 		t->fn(t, variant);
 		if (t->skip)
-			_exit(255);
-		/* Pass is exit 0 */
+			_exit(KSFT_SKIP);
 		if (t->passed)
-			_exit(0);
-		_exit(1);
+			_exit(KSFT_PASS);
+		_exit(KSFT_FAIL);
 	} else {
 		__wait_for_test(t);
 	}
-- 
2.43.2

[PATCH v4 04/12] selftests: kselftest_harness: generate test name once

[Jakub Kicinski]

Since we added variant support generating full test case
name takes 4 string arguments. We're about to need it
in another two places. Stop the duplication and print
once into a temporary buffer.

Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/kselftest_harness.h | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 62ce258b0853..4a2bda6a67ed 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -56,6 +56,7 @@
 #include <asm/types.h>
 #include <ctype.h>
 #include <errno.h>
+#include <limits.h>
 #include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
@@ -1107,14 +1108,18 @@ void __run_test(struct __fixture_metadata *f,
 		struct __fixture_variant_metadata *variant,
 		struct __test_metadata *t)
 {
+	char test_name[LINE_MAX];
+
 	/* reset test struct */
 	t->passed = 1;
 	t->skip = 0;
 	t->trigger = 0;
 	memset(t->results->reason, 0, sizeof(t->results->reason));
 
-	ksft_print_msg(" RUN           %s%s%s.%s ...\n",
-	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
+	snprintf(test_name, sizeof(test_name), "%s%s%s.%s",
+		 f->name, variant->name[0] ? "." : "", variant->name, t->name);
+
+	ksft_print_msg(" RUN           %s ...\n", test_name);
 
 	/* Make sure output buffers are flushed before fork */
 	fflush(stdout);
@@ -1135,15 +1140,14 @@ void __run_test(struct __fixture_metadata *f,
 	} else {
 		__wait_for_test(t);
 	}
-	ksft_print_msg("         %4s  %s%s%s.%s\n", t->passed ? "OK" : "FAIL",
-	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
+	ksft_print_msg("         %4s  %s\n",
+		       t->passed ? "OK" : "FAIL", test_name);
 
 	if (t->skip)
 		ksft_test_result_skip("%s\n", t->results->reason[0] ?
 					t->results->reason : "unknown");
 	else
-		ksft_test_result(t->passed, "%s%s%s.%s\n",
-			f->name, variant->name[0] ? "." : "", variant->name, t->name);
+		ksft_test_result(t->passed, "%s\n", test_name);
 }
 
 static int test_harness_run(int argc, char **argv)
-- 
2.43.2

[PATCH v4 05/12] selftests: kselftest_harness: save full exit code in metadata

[Jakub Kicinski]

Instead of tracking passed = 0/1 rename the field to exit_code
and invert the values so that they match the KSFT_* exit codes.
This will allow us to fold SKIP / XFAIL into the same value.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/kselftest_harness.h   | 48 +++++++++++--------
 tools/testing/selftests/landlock/base_test.c  |  2 +-
 tools/testing/selftests/landlock/fs_test.c    |  4 +-
 tools/testing/selftests/landlock/net_test.c   |  4 +-
 .../testing/selftests/landlock/ptrace_test.c  |  7 +--
 tools/testing/selftests/net/tls.c             |  2 +-
 tools/testing/selftests/seccomp/seccomp_bpf.c |  9 ++--
 7 files changed, 41 insertions(+), 35 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 4a2bda6a67ed..d90d4a9039ee 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -128,7 +128,7 @@
 		fprintf(TH_LOG_STREAM, "#      SKIP      %s\n", \
 			_metadata->results->reason); \
 	} \
-	_metadata->passed = 1; \
+	_metadata->exit_code = KSFT_PASS; \
 	_metadata->skip = 1; \
 	_metadata->trigger = 0; \
 	statement; \
@@ -387,7 +387,7 @@
 		if (setjmp(_metadata->env) == 0) { \
 			fixture_name##_setup(_metadata, &self, variant->data); \
 			/* Let setup failure terminate early. */ \
-			if (!_metadata->passed || _metadata->skip) \
+			if (!__test_passed(_metadata) || _metadata->skip) \
 				return; \
 			_metadata->setup_completed = true; \
 			/* Use the same _metadata. */ \
@@ -398,7 +398,7 @@
 			} \
 			if (child < 0) { \
 				ksft_print_msg("ERROR SPAWNING TEST GRANDCHILD\n"); \
-				_metadata->passed = 0; \
+				_metadata->exit_code = KSFT_FAIL; \
 			} \
 		} \
 		if (child == 0) \
@@ -747,7 +747,7 @@
 			break; \
 			} \
 		} \
-		_metadata->passed = 0; \
+		_metadata->exit_code = KSFT_FAIL; \
 		/* Ensure the optional handler is triggered */ \
 		_metadata->trigger = 1; \
 	} \
@@ -758,7 +758,7 @@
 	const char *__seen = (_seen); \
 	if (!(strcmp(__exp, __seen) _t 0))  { \
 		__TH_LOG("Expected '%s' %s '%s'.", __exp, #_t, __seen); \
-		_metadata->passed = 0; \
+		_metadata->exit_code = KSFT_FAIL; \
 		_metadata->trigger = 1; \
 	} \
 } while (0); OPTIONAL_HANDLER(_assert)
@@ -836,7 +836,7 @@ struct __test_metadata {
 	pid_t pid;	/* pid of test when being run */
 	struct __fixture_metadata *fixture;
 	int termsig;
-	int passed;
+	int exit_code;
 	int skip;	/* did SKIP get used? */
 	int trigger; /* extra handler after the evaluation */
 	int timeout;	/* seconds to wait for test timeout */
@@ -848,6 +848,12 @@ struct __test_metadata {
 	struct __test_metadata *prev, *next;
 };
 
+static inline bool __test_passed(struct __test_metadata *metadata)
+{
+	return metadata->exit_code != KSFT_FAIL &&
+	       metadata->exit_code <= KSFT_SKIP;
+}
+
 /*
  * Since constructors are called in reverse order, reverse the test
  * list so tests are run in source declaration order.
@@ -912,7 +918,7 @@ void __wait_for_test(struct __test_metadata *t)
 	int status;
 
 	if (sigaction(SIGALRM, &action, &saved_action)) {
-		t->passed = 0;
+		t->exit_code = KSFT_FAIL;
 		fprintf(TH_LOG_STREAM,
 			"# %s: unable to install SIGALRM handler\n",
 			t->name);
@@ -924,7 +930,7 @@ void __wait_for_test(struct __test_metadata *t)
 	waitpid(t->pid, &status, 0);
 	alarm(0);
 	if (sigaction(SIGALRM, &saved_action, NULL)) {
-		t->passed = 0;
+		t->exit_code = KSFT_FAIL;
 		fprintf(TH_LOG_STREAM,
 			"# %s: unable to uninstall SIGALRM handler\n",
 			t->name);
@@ -933,16 +939,16 @@ void __wait_for_test(struct __test_metadata *t)
 	__active_test = NULL;
 
 	if (t->timed_out) {
-		t->passed = 0;
+		t->exit_code = KSFT_FAIL;
 		fprintf(TH_LOG_STREAM,
 			"# %s: Test terminated by timeout\n", t->name);
 	} else if (WIFEXITED(status)) {
 		if (WEXITSTATUS(status) == KSFT_SKIP) {
 			/* SKIP */
-			t->passed = 1;
+			t->exit_code = KSFT_PASS;
 			t->skip = 1;
 		} else if (t->termsig != -1) {
-			t->passed = 0;
+			t->exit_code = KSFT_FAIL;
 			fprintf(TH_LOG_STREAM,
 				"# %s: Test exited normally instead of by signal (code: %d)\n",
 				t->name,
@@ -951,24 +957,24 @@ void __wait_for_test(struct __test_metadata *t)
 			switch (WEXITSTATUS(status)) {
 			/* Success */
 			case KSFT_PASS:
-				t->passed = 1;
+				t->exit_code = KSFT_PASS;
 				break;
 			/* Failure */
 			default:
-				t->passed = 0;
+				t->exit_code = KSFT_FAIL;
 				fprintf(TH_LOG_STREAM,
 					"# %s: Test failed\n",
 					t->name);
 			}
 		}
 	} else if (WIFSIGNALED(status)) {
-		t->passed = 0;
+		t->exit_code = KSFT_FAIL;
 		if (WTERMSIG(status) == SIGABRT) {
 			fprintf(TH_LOG_STREAM,
 				"# %s: Test terminated by assertion\n",
 				t->name);
 		} else if (WTERMSIG(status) == t->termsig) {
-			t->passed = 1;
+			t->exit_code = KSFT_PASS;
 		} else {
 			fprintf(TH_LOG_STREAM,
 				"# %s: Test terminated unexpectedly by signal %d\n",
@@ -1111,7 +1117,7 @@ void __run_test(struct __fixture_metadata *f,
 	char test_name[LINE_MAX];
 
 	/* reset test struct */
-	t->passed = 1;
+	t->exit_code = KSFT_PASS;
 	t->skip = 0;
 	t->trigger = 0;
 	memset(t->results->reason, 0, sizeof(t->results->reason));
@@ -1128,26 +1134,26 @@ void __run_test(struct __fixture_metadata *f,
 	t->pid = fork();
 	if (t->pid < 0) {
 		ksft_print_msg("ERROR SPAWNING TEST CHILD\n");
-		t->passed = 0;
+		t->exit_code = KSFT_FAIL;
 	} else if (t->pid == 0) {
 		setpgrp();
 		t->fn(t, variant);
 		if (t->skip)
 			_exit(KSFT_SKIP);
-		if (t->passed)
+		if (__test_passed(t))
 			_exit(KSFT_PASS);
 		_exit(KSFT_FAIL);
 	} else {
 		__wait_for_test(t);
 	}
 	ksft_print_msg("         %4s  %s\n",
-		       t->passed ? "OK" : "FAIL", test_name);
+		       __test_passed(t) ? "OK" : "FAIL", test_name);
 
 	if (t->skip)
 		ksft_test_result_skip("%s\n", t->results->reason[0] ?
 					t->results->reason : "unknown");
 	else
-		ksft_test_result(t->passed, "%s\n", test_name);
+		ksft_test_result(__test_passed(t), "%s\n", test_name);
 }
 
 static int test_harness_run(int argc, char **argv)
@@ -1195,7 +1201,7 @@ static int test_harness_run(int argc, char **argv)
 				t->results = results;
 				__run_test(f, v, t);
 				t->results = NULL;
-				if (t->passed)
+				if (__test_passed(t))
 					pass_count++;
 				else
 					ret = 1;
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 646f778dfb1e..a6f89aaea77d 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -307,7 +307,7 @@ TEST(ruleset_fd_transfer)
 		dir_fd = open("/tmp", O_RDONLY | O_DIRECTORY | O_CLOEXEC);
 		ASSERT_LE(0, dir_fd);
 		ASSERT_EQ(0, close(dir_fd));
-		_exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
+		_exit(_metadata->exit_code);
 		return;
 	}
 
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 2d6d9b43d958..98817a14c91b 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -1964,7 +1964,7 @@ static void test_execute(struct __test_metadata *const _metadata, const int err,
 			       strerror(errno));
 		};
 		ASSERT_EQ(err, errno);
-		_exit(_metadata->passed ? 2 : 1);
+		_exit(__test_passed(_metadata) ? 2 : 1);
 		return;
 	}
 	ASSERT_EQ(child, waitpid(child, &status, 0));
@@ -3807,7 +3807,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate_in_different_processes)
 
 		ASSERT_EQ(0, close(socket_fds[0]));
 
-		_exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
+		_exit(_metadata->exit_code);
 		return;
 	}
 
diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index 936cfc879f1d..f21cfbbc3638 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -539,7 +539,7 @@ static void test_bind_and_connect(struct __test_metadata *const _metadata,
 		}
 
 		EXPECT_EQ(0, close(connect_fd));
-		_exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
+		_exit(_metadata->exit_code);
 		return;
 	}
 
@@ -834,7 +834,7 @@ TEST_F(protocol, connect_unspec)
 		}
 
 		EXPECT_EQ(0, close(connect_fd));
-		_exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
+		_exit(_metadata->exit_code);
 		return;
 	}
 
diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
index 55e7871631a1..a19db4d0b3bd 100644
--- a/tools/testing/selftests/landlock/ptrace_test.c
+++ b/tools/testing/selftests/landlock/ptrace_test.c
@@ -314,7 +314,7 @@ TEST_F(hierarchy, trace)
 	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
 	if (variant->domain_both) {
 		create_domain(_metadata);
-		if (!_metadata->passed)
+		if (!__test_passed(_metadata))
 			/* Aborts before forking. */
 			return;
 	}
@@ -375,7 +375,7 @@ TEST_F(hierarchy, trace)
 
 		/* Waits for the parent PTRACE_ATTACH test. */
 		ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
-		_exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
+		_exit(_metadata->exit_code);
 		return;
 	}
 
@@ -430,9 +430,10 @@ TEST_F(hierarchy, trace)
 	/* Signals that the parent PTRACE_ATTACH test is done. */
 	ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
 	ASSERT_EQ(child, waitpid(child, &status, 0));
+
 	if (WIFSIGNALED(status) || !WIFEXITED(status) ||
 	    WEXITSTATUS(status) != EXIT_SUCCESS)
-		_metadata->passed = 0;
+		_metadata->exit_code = KSFT_FAIL;
 }
 
 TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index b95c249f81c2..c6eda21cefb6 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -1927,7 +1927,7 @@ TEST_F(tls_err, poll_partial_rec_async)
 		pfd.events = POLLIN;
 		EXPECT_EQ(poll(&pfd, 1, 20), 1);
 
-		exit(!_metadata->passed);
+		exit(!__test_passed(_metadata));
 	}
 }
 
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 38f651469968..1027e6170186 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -1576,7 +1576,7 @@ void start_tracer(struct __test_metadata *_metadata, int fd, pid_t tracee,
 		ASSERT_EQ(0, ret);
 	}
 	/* Directly report the status of our test harness results. */
-	syscall(__NR_exit, _metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
+	syscall(__NR_exit, _metadata->exit_code);
 }
 
 /* Common tracer setup/teardown functions. */
@@ -1623,7 +1623,7 @@ void teardown_trace_fixture(struct __test_metadata *_metadata,
 		ASSERT_EQ(0, kill(tracer, SIGUSR1));
 		ASSERT_EQ(tracer, waitpid(tracer, &status, 0));
 		if (WEXITSTATUS(status))
-			_metadata->passed = 0;
+			_metadata->exit_code = KSFT_FAIL;
 	}
 }
 
@@ -3088,8 +3088,7 @@ TEST(syscall_restart)
 		}
 
 		/* Directly report the status of our test harness results. */
-		syscall(__NR_exit, _metadata->passed ? EXIT_SUCCESS
-						     : EXIT_FAILURE);
+		syscall(__NR_exit, _metadata->exit_code);
 	}
 	EXPECT_EQ(0, close(pipefd[0]));
 
@@ -3174,7 +3173,7 @@ TEST(syscall_restart)
 
 	ASSERT_EQ(child_pid, waitpid(child_pid, &status, 0));
 	if (WIFSIGNALED(status) || WEXITSTATUS(status))
-		_metadata->passed = 0;
+		_metadata->exit_code = KSFT_FAIL;
 }
 
 TEST_SIGNAL(filter_flag_log, SIGSYS)
-- 
2.43.2

[PATCH v4 06/12] selftests: kselftest_harness: use exit code to store skip

[Jakub Kicinski]

We always use skip in combination with exit_code being 0
(KSFT_PASS). This are basic KSFT / KTAP semantics.
Store the right KSFT_* code in exit_code directly.

This makes it easier to support tests reporting other
extended KSFT_* codes like XFAIL / XPASS.

Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/kselftest_harness.h | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index d90d4a9039ee..5a48177c8c00 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -128,8 +128,7 @@
 		fprintf(TH_LOG_STREAM, "#      SKIP      %s\n", \
 			_metadata->results->reason); \
 	} \
-	_metadata->exit_code = KSFT_PASS; \
-	_metadata->skip = 1; \
+	_metadata->exit_code = KSFT_SKIP; \
 	_metadata->trigger = 0; \
 	statement; \
 } while (0)
@@ -387,7 +386,7 @@
 		if (setjmp(_metadata->env) == 0) { \
 			fixture_name##_setup(_metadata, &self, variant->data); \
 			/* Let setup failure terminate early. */ \
-			if (!__test_passed(_metadata) || _metadata->skip) \
+			if (_metadata->exit_code) \
 				return; \
 			_metadata->setup_completed = true; \
 			/* Use the same _metadata. */ \
@@ -837,7 +836,6 @@ struct __test_metadata {
 	struct __fixture_metadata *fixture;
 	int termsig;
 	int exit_code;
-	int skip;	/* did SKIP get used? */
 	int trigger; /* extra handler after the evaluation */
 	int timeout;	/* seconds to wait for test timeout */
 	bool timed_out;	/* did this test timeout instead of exiting? */
@@ -944,9 +942,7 @@ void __wait_for_test(struct __test_metadata *t)
 			"# %s: Test terminated by timeout\n", t->name);
 	} else if (WIFEXITED(status)) {
 		if (WEXITSTATUS(status) == KSFT_SKIP) {
-			/* SKIP */
-			t->exit_code = KSFT_PASS;
-			t->skip = 1;
+			t->exit_code = WEXITSTATUS(status);
 		} else if (t->termsig != -1) {
 			t->exit_code = KSFT_FAIL;
 			fprintf(TH_LOG_STREAM,
@@ -1118,7 +1114,6 @@ void __run_test(struct __fixture_metadata *f,
 
 	/* reset test struct */
 	t->exit_code = KSFT_PASS;
-	t->skip = 0;
 	t->trigger = 0;
 	memset(t->results->reason, 0, sizeof(t->results->reason));
 
@@ -1138,18 +1133,14 @@ void __run_test(struct __fixture_metadata *f,
 	} else if (t->pid == 0) {
 		setpgrp();
 		t->fn(t, variant);
-		if (t->skip)
-			_exit(KSFT_SKIP);
-		if (__test_passed(t))
-			_exit(KSFT_PASS);
-		_exit(KSFT_FAIL);
+		_exit(t->exit_code);
 	} else {
 		__wait_for_test(t);
 	}
 	ksft_print_msg("         %4s  %s\n",
 		       __test_passed(t) ? "OK" : "FAIL", test_name);
 
-	if (t->skip)
+	if (t->exit_code == KSFT_SKIP)
 		ksft_test_result_skip("%s\n", t->results->reason[0] ?
 					t->results->reason : "unknown");
 	else
-- 
2.43.2

[PATCH v4 07/12] selftests: kselftest: add ksft_test_result_code(), handling all exit codes

[Jakub Kicinski]

For generic test harness code it's more useful to deal with exit
codes directly, rather than having to switch on them and call
the right ksft_test_result_*() helper. Add such function to kselftest.h.

Note that "directive" and "diagnostic" are what ktap docs call
those parts of the message.

Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/kselftest.h         | 39 +++++++++++++++++++++
 tools/testing/selftests/kselftest_harness.h |  9 +++--
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index a781e6311810..12ad7f8dfe3a 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -25,6 +25,7 @@
  *     ksft_test_result_skip(fmt, ...);
  *     ksft_test_result_xfail(fmt, ...);
  *     ksft_test_result_error(fmt, ...);
+ *     ksft_test_result_code(exit_code, test_name, fmt, ...);
  *
  * When all tests are finished, clean up and exit the program with one of:
  *
@@ -254,6 +255,44 @@ static inline __printf(1, 2) void ksft_test_result_error(const char *msg, ...)
 	va_end(args);
 }
 
+static inline __printf(2, 3)
+void ksft_test_result_code(int exit_code, const char *msg, ...)
+{
+	const char *tap_code = "ok";
+	const char *directive = "";
+	int saved_errno = errno;
+	va_list args;
+
+	switch (exit_code) {
+	case KSFT_PASS:
+		ksft_cnt.ksft_pass++;
+		break;
+	case KSFT_XFAIL:
+		directive = " # XFAIL ";
+		ksft_cnt.ksft_xfail++;
+		break;
+	case KSFT_XPASS:
+		directive = " # XPASS ";
+		ksft_cnt.ksft_xpass++;
+		break;
+	case KSFT_SKIP:
+		directive = " # SKIP ";
+		ksft_cnt.ksft_xskip++;
+		break;
+	case KSFT_FAIL:
+	default:
+		tap_code = "not ok";
+		ksft_cnt.ksft_fail++;
+		break;
+	}
+
+	va_start(args, msg);
+	printf("%s %u%s", tap_code, ksft_test_num(), directive);
+	errno = saved_errno;
+	vprintf(msg, args);
+	va_end(args);
+}
+
 static inline int ksft_exit_pass(void)
 {
 	ksft_print_cnts();
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 5a48177c8c00..4fb30fcc7774 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -1111,6 +1111,7 @@ void __run_test(struct __fixture_metadata *f,
 		struct __test_metadata *t)
 {
 	char test_name[LINE_MAX];
+	const char *diagnostic;
 
 	/* reset test struct */
 	t->exit_code = KSFT_PASS;
@@ -1140,9 +1141,13 @@ void __run_test(struct __fixture_metadata *f,
 	ksft_print_msg("         %4s  %s\n",
 		       __test_passed(t) ? "OK" : "FAIL", test_name);
 
+	if (t->results->reason[0])
+		diagnostic = t->results->reason;
+	else
+		diagnostic = "unknown";
+
 	if (t->exit_code == KSFT_SKIP)
-		ksft_test_result_skip("%s\n", t->results->reason[0] ?
-					t->results->reason : "unknown");
+		ksft_test_result_code(t->exit_code, "%s\n", diagnostic);
 	else
 		ksft_test_result(__test_passed(t), "%s\n", test_name);
 }
-- 
2.43.2

[PATCH v4 08/12] selftests: kselftest_harness: print test name for SKIP

[Jakub Kicinski]

Jakub points out that for parsers it's rather useful to always
have the test name on the result line. Currently if we SKIP
(or soon XFAIL or XPASS), we will print:

ok 17 # SKIP SCTP doesn't support IP_BIND_ADDRESS_NO_PORT

     ^
     no test name

Always print the test name.
KTAP format seems to allow or even call for it, per:
https://docs.kernel.org/dev-tools/ktap.html

Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Link: https://lore.kernel.org/all/87jzn6lnou.fsf@cloudflare.com/
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/kselftest.h         | 7 ++++---
 tools/testing/selftests/kselftest_harness.h | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 12ad7f8dfe3a..25e29626566e 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -255,8 +255,9 @@ static inline __printf(1, 2) void ksft_test_result_error(const char *msg, ...)
 	va_end(args);
 }
 
-static inline __printf(2, 3)
-void ksft_test_result_code(int exit_code, const char *msg, ...)
+static inline __printf(3, 4)
+void ksft_test_result_code(int exit_code, const char *test_name,
+			   const char *msg, ...)
 {
 	const char *tap_code = "ok";
 	const char *directive = "";
@@ -287,7 +288,7 @@ void ksft_test_result_code(int exit_code, const char *msg, ...)
 	}
 
 	va_start(args, msg);
-	printf("%s %u%s", tap_code, ksft_test_num(), directive);
+	printf("%s %u %s%s", tap_code, ksft_test_num(), test_name, directive);
 	errno = saved_errno;
 	vprintf(msg, args);
 	va_end(args);
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 4fb30fcc7774..82377051aa54 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -1147,7 +1147,8 @@ void __run_test(struct __fixture_metadata *f,
 		diagnostic = "unknown";
 
 	if (t->exit_code == KSFT_SKIP)
-		ksft_test_result_code(t->exit_code, "%s\n", diagnostic);
+		ksft_test_result_code(t->exit_code, test_name,
+				      "%s\n", diagnostic);
 	else
 		ksft_test_result(__test_passed(t), "%s\n", test_name);
 }
-- 
2.43.2

[PATCH v4 09/12] selftests: kselftest_harness: separate diagnostic message with # in ksft_test_result_code()

[Jakub Kicinski]

According to the spec we should always print a # if we add
a diagnostic message. Having the caller pass in the new line
as part of diagnostic message makes handling this a bit
counter-intuitive, so append the new line in the helper.

Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/kselftest.h         | 5 +++++
 tools/testing/selftests/kselftest_harness.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 25e29626566e..541bf192e30e 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -287,10 +287,15 @@ void ksft_test_result_code(int exit_code, const char *test_name,
 		break;
 	}
 
+	/* Docs seem to call for double space if directive is absent */
+	if (!directive[0] && msg[0])
+		directive = " #  ";
+
 	va_start(args, msg);
 	printf("%s %u %s%s", tap_code, ksft_test_num(), test_name, directive);
 	errno = saved_errno;
 	vprintf(msg, args);
+	printf("\n");
 	va_end(args);
 }
 
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 82377051aa54..5b0592e4b7a4 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -1148,7 +1148,7 @@ void __run_test(struct __fixture_metadata *f,
 
 	if (t->exit_code == KSFT_SKIP)
 		ksft_test_result_code(t->exit_code, test_name,
-				      "%s\n", diagnostic);
+				      "%s", diagnostic);
 	else
 		ksft_test_result(__test_passed(t), "%s\n", test_name);
 }
-- 
2.43.2

[PATCH v4 10/12] selftests: kselftest_harness: let PASS / FAIL provide diagnostic

[Jakub Kicinski]

Switch to printing KTAP line for PASS / FAIL with ksft_test_result_code(),
this gives us the ability to report diagnostic messages.

Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/kselftest_harness.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 5b0592e4b7a4..b643a577f9e1 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -1143,14 +1143,13 @@ void __run_test(struct __fixture_metadata *f,
 
 	if (t->results->reason[0])
 		diagnostic = t->results->reason;
+	else if (t->exit_code == KSFT_PASS || t->exit_code == KSFT_FAIL)
+		diagnostic = NULL;
 	else
 		diagnostic = "unknown";
 
-	if (t->exit_code == KSFT_SKIP)
-		ksft_test_result_code(t->exit_code, test_name,
-				      "%s", diagnostic);
-	else
-		ksft_test_result(__test_passed(t), "%s\n", test_name);
+	ksft_test_result_code(t->exit_code, test_name,
+			      diagnostic ? "%s" : "", diagnostic);
 }
 
 static int test_harness_run(int argc, char **argv)
-- 
2.43.2

[PATCH v4 11/12] selftests: kselftest_harness: support using xfail

[Jakub Kicinski]

Currently some tests report skip for things they expect to fail
e.g. when given combination of parameters is known to be unsupported.
This is confusing because in an ideal test environment and fully
featured kernel no tests should be skipped.

Selftest summary line already includes xfail and xpass counters,
e.g.:

  Totals: pass:725 fail:0 xfail:0 xpass:0 skip:0 error:0

but there's no way to use it from within the harness.

Add a new per-fixture+variant combination list of test cases
we expect to fail.

Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v3: change the paradigm to declarative
---
 tools/testing/selftests/kselftest_harness.h | 49 ++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index b643a577f9e1..634be793ad58 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -803,6 +803,37 @@ struct __fixture_metadata {
 	.prev = &_fixture_global,
 };
 
+struct __test_xfail {
+	struct __fixture_metadata *fixture;
+	struct __fixture_variant_metadata *variant;
+	struct __test_metadata *test;
+	struct __test_xfail *prev, *next;
+};
+
+/**
+ * XFAIL_ADD() - mark variant + test case combination as expected to fail
+ * @fixture_name: name of the fixture
+ * @variant_name: name of the variant
+ * @test_name: name of the test case
+ *
+ * Mark a combination of variant + test case for a given fixture as expected
+ * to fail. Tests marked this way will report XPASS / XFAIL return codes,
+ * instead of PASS / FAIL,and use respective counters.
+ */
+#define XFAIL_ADD(fixture_name, variant_name, test_name) \
+	static struct __test_xfail \
+		_##fixture_name##_##variant_name##_##test_name##_xfail = \
+	{ \
+		.fixture = &_##fixture_name##_fixture_object, \
+		.variant = &_##fixture_name##_##variant_name##_object, \
+		.test = &_##fixture_name##_##test_name##_object, \
+	}; \
+	static void __attribute__((constructor)) \
+		_register_##fixture_name##_##variant_name##_##test_name##_xfail(void) \
+	{ \
+		__register_xfail(&_##fixture_name##_##variant_name##_##test_name##_xfail); \
+	}
+
 static struct __fixture_metadata *__fixture_list = &_fixture_global;
 static int __constructor_order;
 
@@ -817,6 +848,7 @@ static inline void __register_fixture(struct __fixture_metadata *f)
 struct __fixture_variant_metadata {
 	const char *name;
 	const void *data;
+	struct __test_xfail *xfails;
 	struct __fixture_variant_metadata *prev, *next;
 };
 
@@ -866,6 +898,11 @@ static inline void __register_test(struct __test_metadata *t)
 	__LIST_APPEND(t->fixture->tests, t);
 }
 
+static inline void __register_xfail(struct __test_xfail *xf)
+{
+	__LIST_APPEND(xf->variant->xfails, xf);
+}
+
 static inline int __bail(int for_realz, struct __test_metadata *t)
 {
 	/* if this is ASSERT, return immediately. */
@@ -941,7 +978,9 @@ void __wait_for_test(struct __test_metadata *t)
 		fprintf(TH_LOG_STREAM,
 			"# %s: Test terminated by timeout\n", t->name);
 	} else if (WIFEXITED(status)) {
-		if (WEXITSTATUS(status) == KSFT_SKIP) {
+		if (WEXITSTATUS(status) == KSFT_SKIP ||
+		    WEXITSTATUS(status) == KSFT_XPASS ||
+		    WEXITSTATUS(status) == KSFT_XFAIL) {
 			t->exit_code = WEXITSTATUS(status);
 		} else if (t->termsig != -1) {
 			t->exit_code = KSFT_FAIL;
@@ -1110,6 +1149,7 @@ void __run_test(struct __fixture_metadata *f,
 		struct __fixture_variant_metadata *variant,
 		struct __test_metadata *t)
 {
+	struct __test_xfail *xfail;
 	char test_name[LINE_MAX];
 	const char *diagnostic;
 
@@ -1141,6 +1181,13 @@ void __run_test(struct __fixture_metadata *f,
 	ksft_print_msg("         %4s  %s\n",
 		       __test_passed(t) ? "OK" : "FAIL", test_name);
 
+	/* Check if we're expecting this test to fail */
+	for (xfail = variant->xfails; xfail; xfail = xfail->next)
+		if (xfail->test == t)
+			break;
+	if (xfail)
+		t->exit_code = __test_passed(t) ? KSFT_XPASS : KSFT_XFAIL;
+
 	if (t->results->reason[0])
 		diagnostic = t->results->reason;
 	else if (t->exit_code == KSFT_PASS || t->exit_code == KSFT_FAIL)
-- 
2.43.2

[PATCH v4 12/12] selftests: ip_local_port_range: use XFAIL instead of SKIP

[Jakub Kicinski]

SCTP does not support IP_LOCAL_PORT_RANGE and we know it,
so use XFAIL instead of SKIP.

Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/ip_local_port_range.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c
index 6ebd58869a63..193b82745fd8 100644
--- a/tools/testing/selftests/net/ip_local_port_range.c
+++ b/tools/testing/selftests/net/ip_local_port_range.c
@@ -365,9 +365,6 @@ TEST_F(ip_local_port_range, late_bind)
 	__u32 range;
 	__u16 port;
 
-	if (variant->so_protocol == IPPROTO_SCTP)
-		SKIP(return, "SCTP doesn't support IP_BIND_ADDRESS_NO_PORT");
-
 	fd = socket(variant->so_domain, variant->so_type, 0);
 	ASSERT_GE(fd, 0) TH_LOG("socket failed");
 
@@ -414,6 +411,9 @@ TEST_F(ip_local_port_range, late_bind)
 	ASSERT_TRUE(!err) TH_LOG("close failed");
 }
 
+XFAIL_ADD(ip_local_port_range, ip4_stcp, late_bind);
+XFAIL_ADD(ip_local_port_range, ip6_stcp, late_bind);
+
 TEST_F(ip_local_port_range, get_port_range)
 {
 	__u16 lo, hi;
-- 
2.43.2

Re: [PATCH v4 12/12] selftests: ip_local_port_range: use XFAIL instead of SKIP

[Jakub Sitnicki]

On Wed, Feb 28, 2024 at 04:59 PM -08, Jakub Kicinski wrote:
> SCTP does not support IP_LOCAL_PORT_RANGE and we know it,
> so use XFAIL instead of SKIP.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/net/ip_local_port_range.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c
> index 6ebd58869a63..193b82745fd8 100644
> --- a/tools/testing/selftests/net/ip_local_port_range.c
> +++ b/tools/testing/selftests/net/ip_local_port_range.c
> @@ -365,9 +365,6 @@ TEST_F(ip_local_port_range, late_bind)
>  	__u32 range;
>  	__u16 port;
>  
> -	if (variant->so_protocol == IPPROTO_SCTP)
> -		SKIP(return, "SCTP doesn't support IP_BIND_ADDRESS_NO_PORT");
> -
>  	fd = socket(variant->so_domain, variant->so_type, 0);
>  	ASSERT_GE(fd, 0) TH_LOG("socket failed");
>  
> @@ -414,6 +411,9 @@ TEST_F(ip_local_port_range, late_bind)
>  	ASSERT_TRUE(!err) TH_LOG("close failed");
>  }
>  
> +XFAIL_ADD(ip_local_port_range, ip4_stcp, late_bind);
> +XFAIL_ADD(ip_local_port_range, ip6_stcp, late_bind);
> +
>  TEST_F(ip_local_port_range, get_port_range)
>  {
>  	__u16 lo, hi;

[wrt our earlier discussion off-list]

You were right, this test succeeds if I delete SKIP for SCTP.
Turns out IP_LOCAL_PORT_RANGE works for SCTP out of the box after all.

What I didn't notice earlier is that sctp_setsockopt() delegates to
ip_setsockopt() when level != SOL_SCTP.

CC'ing Marcelo & Xin, to confirm that this isn't a problem.

Re: [PATCH v4 12/12] selftests: ip_local_port_range: use XFAIL instead of SKIP

[Xin Long]

On Thu, Feb 29, 2024 at 3:27 PM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> On Wed, Feb 28, 2024 at 04:59 PM -08, Jakub Kicinski wrote:
> > SCTP does not support IP_LOCAL_PORT_RANGE and we know it,
> > so use XFAIL instead of SKIP.
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> >  tools/testing/selftests/net/ip_local_port_range.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c
> > index 6ebd58869a63..193b82745fd8 100644
> > --- a/tools/testing/selftests/net/ip_local_port_range.c
> > +++ b/tools/testing/selftests/net/ip_local_port_range.c
> > @@ -365,9 +365,6 @@ TEST_F(ip_local_port_range, late_bind)
> >       __u32 range;
> >       __u16 port;
> >
> > -     if (variant->so_protocol == IPPROTO_SCTP)
> > -             SKIP(return, "SCTP doesn't support IP_BIND_ADDRESS_NO_PORT");
> > -
> >       fd = socket(variant->so_domain, variant->so_type, 0);
> >       ASSERT_GE(fd, 0) TH_LOG("socket failed");
> >
> > @@ -414,6 +411,9 @@ TEST_F(ip_local_port_range, late_bind)
> >       ASSERT_TRUE(!err) TH_LOG("close failed");
> >  }
> >
> > +XFAIL_ADD(ip_local_port_range, ip4_stcp, late_bind);
> > +XFAIL_ADD(ip_local_port_range, ip6_stcp, late_bind);
> > +
> >  TEST_F(ip_local_port_range, get_port_range)
> >  {
> >       __u16 lo, hi;
>
> [wrt our earlier discussion off-list]
>
> You were right, this test succeeds if I delete SKIP for SCTP.
> Turns out IP_LOCAL_PORT_RANGE works for SCTP out of the box after all.
>
> What I didn't notice earlier is that sctp_setsockopt() delegates to
> ip_setsockopt() when level != SOL_SCTP.
>
> CC'ing Marcelo & Xin, to confirm that this isn't a problem.
Yes, SCTP supports ip_local_port_range by calling
inet_sk_get_local_port_range() in sctp_get_port(), similar to TCP/UDP.

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 28 Feb 2024 16:59:07 -0800 you wrote:
> Hi!
> 
> When running selftests for our subsystem in our CI we'd like all
> tests to pass. Currently some tests use SKIP for cases they
> expect to fail, because the kselftest_harness limits the return
> codes to pass/fail/skip. XFAIL which would be a great match
> here cannot be used.
> 
> [...]

Here is the summary with links:
  - [v4,01/12] selftests/landlock: Redefine TEST_F() as TEST_F_FORK()
    https://git.kernel.org/netdev/net-next/c/e74048650eaf
  - [v4,02/12] selftests/harness: Merge TEST_F_FORK() into TEST_F()
    https://git.kernel.org/netdev/net-next/c/0710a1a73fb4
  - [v4,03/12] selftests: kselftest_harness: use KSFT_* exit codes
    https://git.kernel.org/netdev/net-next/c/a724707976b0
  - [v4,04/12] selftests: kselftest_harness: generate test name once
    https://git.kernel.org/netdev/net-next/c/38c957f07038
  - [v4,05/12] selftests: kselftest_harness: save full exit code in metadata
    https://git.kernel.org/netdev/net-next/c/69fe8ec4f673
  - [v4,06/12] selftests: kselftest_harness: use exit code to store skip
    https://git.kernel.org/netdev/net-next/c/796a344fa431
  - [v4,07/12] selftests: kselftest: add ksft_test_result_code(), handling all exit codes
    https://git.kernel.org/netdev/net-next/c/fa1a53d83674
  - [v4,08/12] selftests: kselftest_harness: print test name for SKIP
    https://git.kernel.org/netdev/net-next/c/732e2035280b
  - [v4,09/12] selftests: kselftest_harness: separate diagnostic message with # in ksft_test_result_code()
    https://git.kernel.org/netdev/net-next/c/42ab727eb95f
  - [v4,10/12] selftests: kselftest_harness: let PASS / FAIL provide diagnostic
    https://git.kernel.org/netdev/net-next/c/378193eff339
  - [v4,11/12] selftests: kselftest_harness: support using xfail
    https://git.kernel.org/netdev/net-next/c/2709473c9386
  - [v4,12/12] selftests: ip_local_port_range: use XFAIL instead of SKIP
    https://git.kernel.org/netdev/net-next/c/c05bf0e93312

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v4 12/12] selftests: ip_local_port_range: use XFAIL instead of SKIP

[Jakub Sitnicki]

On Thu, Feb 29, 2024 at 06:25 PM -05, Xin Long wrote:
> On Thu, Feb 29, 2024 at 3:27 PM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> On Wed, Feb 28, 2024 at 04:59 PM -08, Jakub Kicinski wrote:
>> > SCTP does not support IP_LOCAL_PORT_RANGE and we know it,
>> > so use XFAIL instead of SKIP.
>> >
>> > Reviewed-by: Kees Cook <keescook@chromium.org>
>> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>> > ---
>> >  tools/testing/selftests/net/ip_local_port_range.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c
>> > index 6ebd58869a63..193b82745fd8 100644
>> > --- a/tools/testing/selftests/net/ip_local_port_range.c
>> > +++ b/tools/testing/selftests/net/ip_local_port_range.c
>> > @@ -365,9 +365,6 @@ TEST_F(ip_local_port_range, late_bind)
>> >       __u32 range;
>> >       __u16 port;
>> >
>> > -     if (variant->so_protocol == IPPROTO_SCTP)
>> > -             SKIP(return, "SCTP doesn't support IP_BIND_ADDRESS_NO_PORT");
>> > -
>> >       fd = socket(variant->so_domain, variant->so_type, 0);
>> >       ASSERT_GE(fd, 0) TH_LOG("socket failed");
>> >
>> > @@ -414,6 +411,9 @@ TEST_F(ip_local_port_range, late_bind)
>> >       ASSERT_TRUE(!err) TH_LOG("close failed");
>> >  }
>> >
>> > +XFAIL_ADD(ip_local_port_range, ip4_stcp, late_bind);
>> > +XFAIL_ADD(ip_local_port_range, ip6_stcp, late_bind);
>> > +
>> >  TEST_F(ip_local_port_range, get_port_range)
>> >  {
>> >       __u16 lo, hi;
>>
>> [wrt our earlier discussion off-list]
>>
>> You were right, this test succeeds if I delete SKIP for SCTP.
>> Turns out IP_LOCAL_PORT_RANGE works for SCTP out of the box after all.
>>
>> What I didn't notice earlier is that sctp_setsockopt() delegates to
>> ip_setsockopt() when level != SOL_SCTP.
>>
>> CC'ing Marcelo & Xin, to confirm that this isn't a problem.
> Yes, SCTP supports ip_local_port_range by calling
> inet_sk_get_local_port_range() in sctp_get_port(), similar to TCP/UDP.

Well, that's embarassing.

I see that I've updated sctp stack to use inet_sk_get_local_port_range()
in 91d0b78c5177 ("inet: Add IP_LOCAL_PORT_RANGE socket option").

Thanks for confirming, Xin.

It's clearly an overside on my side. That SKIP in tests should have
never been there. I will send a fixup.

Re: [PATCH v4 02/12] selftests/harness: Merge TEST_F_FORK() into TEST_F()

[Mickaël Salaün]

Testing the whole series, I found that some Landlock tests are flaky
starting with this patch.  I tried to not use the longjmp in the
grandchild but it didn't change.  I suspect missing volatiles but I
didn't find the faulty one(s) yet. :/
I'll continue investigating tomorrow but help would be much appreciated!


On Wed, Feb 28, 2024 at 04:59:09PM -0800, Jakub Kicinski wrote:
> From: Mickaël Salaün <mic@digikod.net>
> 
> Replace Landlock-specific TEST_F_FORK() with an improved TEST_F() which
> brings four related changes:
> 
> Run TEST_F()'s tests in a grandchild process to make it possible to
> drop privileges and delegate teardown to the parent.
> 
> Compared to TEST_F_FORK(), simplify handling of the test grandchild
> process thanks to vfork(2), and makes it generic (e.g. no explicit
> conversion between exit code and _metadata).
> 
> Compared to TEST_F_FORK(), run teardown even when tests failed with an
> assert thanks to commit 63e6b2a42342 ("selftests/harness: Run TEARDOWN
> for ASSERT failures").
> 
> Simplify the test harness code by removing the no_print and step fields
> which are not used.  I added this feature just after I made
> kselftest_harness.h more broadly available but this step counter
> remained even though it wasn't needed after all. See commit 369130b63178
> ("selftests: Enhance kselftest_harness.h to print which assert failed").
> 
> Replace spaces with tabs in one line of __TEST_F_IMPL().
> 
> Cc: Günther Noack <gnoack@google.com>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Will Drewry <wad@chromium.org>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> --
> v4:
>  - GAND -> GRAND
>  - init child to 1, otherwise assert in setup triggers a longjmp
>    which in turn reads child without it ever getting initialized
>    (or being 0, i.e. we mistakenly assume we're in the grandchild)

Good catch!

Re: [PATCH v4 02/12] selftests/harness: Merge TEST_F_FORK() into TEST_F()

[Mickaël Salaün]

On Mon, Mar 04, 2024 at 08:27:50PM +0100, Mickaël Salaün wrote:
> Testing the whole series, I found that some Landlock tests are flaky
> starting with this patch.  I tried to not use the longjmp in the
> grandchild but it didn't change.  I suspect missing volatiles but I
> didn't find the faulty one(s) yet. :/
> I'll continue investigating tomorrow but help would be much appreciated!

The issue is with the fs_test.c, often starting with this one:

#  RUN           layout1.relative_chroot_only ...
# fs_test.c:294:relative_chroot_only:Expected 0 (0) == umount(TMP_DIR) (-1)
# fs_test.c:296:relative_chroot_only:Expected 0 (0) == remove_path(TMP_DIR) (16)
# relative_chroot_only: Test failed
#          FAIL  layout1.relative_chroot_only

...or this one:

#  RUN           layout3_fs.hostfs.tag_inode_dir_child ...
# fs_test.c:4707:tag_inode_dir_child:Expected 0 (0) == mkdir(self->dir_path, 0700) (-1)
# fs_test.c:4709:tag_inode_dir_child:Failed to create directory "tmp/dir": No such file or directory
# fs_test.c:4724:tag_inode_dir_child:Expected 0 (0) <= fd (-1)
# fs_test.c:4726:tag_inode_dir_child:Failed to create file "tmp/dir/file": No such file or directory
# fs_test.c:4729:tag_inode_dir_child:Expected 0 (0) == close(fd) (-1)
# tag_inode_dir_child: Test failed
#          FAIL  layout3_fs.hostfs.tag_inode_dir_child


> 
> 
> On Wed, Feb 28, 2024 at 04:59:09PM -0800, Jakub Kicinski wrote:
> > From: Mickaël Salaün <mic@digikod.net>
> > 
> > Replace Landlock-specific TEST_F_FORK() with an improved TEST_F() which
> > brings four related changes:
> > 
> > Run TEST_F()'s tests in a grandchild process to make it possible to
> > drop privileges and delegate teardown to the parent.
> > 
> > Compared to TEST_F_FORK(), simplify handling of the test grandchild
> > process thanks to vfork(2), and makes it generic (e.g. no explicit
> > conversion between exit code and _metadata).
> > 
> > Compared to TEST_F_FORK(), run teardown even when tests failed with an
> > assert thanks to commit 63e6b2a42342 ("selftests/harness: Run TEARDOWN
> > for ASSERT failures").
> > 
> > Simplify the test harness code by removing the no_print and step fields
> > which are not used.  I added this feature just after I made
> > kselftest_harness.h more broadly available but this step counter
> > remained even though it wasn't needed after all. See commit 369130b63178
> > ("selftests: Enhance kselftest_harness.h to print which assert failed").
> > 
> > Replace spaces with tabs in one line of __TEST_F_IMPL().
> > 
> > Cc: Günther Noack <gnoack@google.com>
> > Cc: Shuah Khan <shuah@kernel.org>
> > Cc: Will Drewry <wad@chromium.org>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > --
> > v4:
> >  - GAND -> GRAND
> >  - init child to 1, otherwise assert in setup triggers a longjmp
> >    which in turn reads child without it ever getting initialized
> >    (or being 0, i.e. we mistakenly assume we're in the grandchild)
> 
> Good catch!

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3448 bytes --]

On Wed, Feb 28, 2024 at 04:59:07PM -0800, Jakub Kicinski wrote:

> When running selftests for our subsystem in our CI we'd like all
> tests to pass. Currently some tests use SKIP for cases they
> expect to fail, because the kselftest_harness limits the return
> codes to pass/fail/skip. XFAIL which would be a great match
> here cannot be used.
> 
> Remove the no_print handling and use vfork() to run the test in
> a different process than the setup. This way we don't need to
> pass "failing step" via the exit code. Further clean up the exit
> codes so that we can use all KSFT_* values. Rewrite the result
> printing to make handling XFAIL/XPASS easier. Support tests
> declaring combinations of fixture + variant they expect to fail.

This series landed in -next today and has caused breakage on all
platforms in the ALSA pcmtest-driver test.  When run on systems that
don't have the driver it needs loaded the test skip but since this
series was merged skipped tests are logged but then reported back as
failures:

# selftests: alsa: test-pcmtest-driver
# TAP version 13
# 1..5
# # Starting 5 tests from 1 test cases.
# #  RUN           pcmtest.playback ...
# #      SKIP      Can't read patterns. Probably, module isn't loaded
# # playback: Test failed
# #          FAIL  pcmtest.playback
# not ok 1 pcmtest.playback #  Can't read patterns. Probably, module isn't loaded
# #  RUN           pcmtest.capture ...
# #      SKIP      Can't read patterns. Probably, module isn't loaded
# # capture: Test failed
# #          FAIL  pcmtest.capture
# not ok 2 pcmtest.capture #  Can't read patterns. Probably, module isn't loaded
# #  RUN           pcmtest.ni_capture ...
# #      SKIP      Can't read patterns. Probably, module isn't loaded
# # ni_capture: Test failed
# #          FAIL  pcmtest.ni_capture
# not ok 3 pcmtest.ni_capture #  Can't read patterns. Probably, module isn't loaded
# #  RUN           pcmtest.ni_playback ...
# #      SKIP      Can't read patterns. Probably, module isn't loaded
# # ni_playback: Test failed
# #          FAIL  pcmtest.ni_playback
# not ok 4 pcmtest.ni_playback #  Can't read patterns. Probably, module isn't loaded
# #  RUN           pcmtest.reset_ioctl ...
# #      SKIP      Can't read patterns. Probably, module isn't loaded
# # reset_ioctl: Test failed
# #          FAIL  pcmtest.reset_ioctl
# not ok 5 pcmtest.reset_ioctl #  Can't read patterns. Probably, module isn't loaded
# # FAILED: 0 / 5 tests passed.
# # Totals: pass:0 fail:5 xfail:0 xpass:0 skip:0 error:0

I haven't completely isolated the issue due to some other breakage
that's making it harder that it should be to test.  

A sample full log can be seen at:

   https://lava.sirena.org.uk/scheduler/job/659576#L1349

but there's no more context.  I'm also seeing some breakage in the
seccomp selftests which also use kselftest-harness:

# #  RUN           TRAP.dfl ...
# # dfl: Test exited normally instead of by signal (code: 0)
# #          FAIL  TRAP.dfl
# not ok 56 TRAP.dfl
# #  RUN           TRAP.ign ...
# # ign: Test exited normally instead of by signal (code: 0)
# #          FAIL  TRAP.ign
# not ok 57 TRAP.ign

and looks suspiciously like it might also be related.  Again, not
drilled down fully yet or looked at the code but this series jumps out
as most likely relevant.

Full log at:

   https://validation.linaro.org/scheduler/job/4046205#L6066

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Jakub Kicinski]

On Mon, 4 Mar 2024 22:20:03 +0000 Mark Brown wrote:
> On Wed, Feb 28, 2024 at 04:59:07PM -0800, Jakub Kicinski wrote:
> 
> > When running selftests for our subsystem in our CI we'd like all
> > tests to pass. Currently some tests use SKIP for cases they
> > expect to fail, because the kselftest_harness limits the return
> > codes to pass/fail/skip. XFAIL which would be a great match
> > here cannot be used.
> > 
> > Remove the no_print handling and use vfork() to run the test in
> > a different process than the setup. This way we don't need to
> > pass "failing step" via the exit code. Further clean up the exit
> > codes so that we can use all KSFT_* values. Rewrite the result
> > printing to make handling XFAIL/XPASS easier. Support tests
> > declaring combinations of fixture + variant they expect to fail.  
> 
> This series landed in -next today and has caused breakage on all
> platforms in the ALSA pcmtest-driver test.  When run on systems that
> don't have the driver it needs loaded the test skip but since this
> series was merged skipped tests are logged but then reported back as
> failures:
> 
> # selftests: alsa: test-pcmtest-driver
> # TAP version 13
> # 1..5
> # # Starting 5 tests from 1 test cases.
> # #  RUN           pcmtest.playback ...
> # #      SKIP      Can't read patterns. Probably, module isn't loaded
> # # playback: Test failed
> # #          FAIL  pcmtest.playback
> # not ok 1 pcmtest.playback #  Can't read patterns. Probably, module isn't loaded
> # #  RUN           pcmtest.capture ...
> # #      SKIP      Can't read patterns. Probably, module isn't loaded
> # # capture: Test failed
> # #          FAIL  pcmtest.capture
> # not ok 2 pcmtest.capture #  Can't read patterns. Probably, module isn't loaded
> # #  RUN           pcmtest.ni_capture ...
> # #      SKIP      Can't read patterns. Probably, module isn't loaded
> # # ni_capture: Test failed
> # #          FAIL  pcmtest.ni_capture
> # not ok 3 pcmtest.ni_capture #  Can't read patterns. Probably, module isn't loaded
> # #  RUN           pcmtest.ni_playback ...
> # #      SKIP      Can't read patterns. Probably, module isn't loaded
> # # ni_playback: Test failed
> # #          FAIL  pcmtest.ni_playback
> # not ok 4 pcmtest.ni_playback #  Can't read patterns. Probably, module isn't loaded
> # #  RUN           pcmtest.reset_ioctl ...
> # #      SKIP      Can't read patterns. Probably, module isn't loaded
> # # reset_ioctl: Test failed
> # #          FAIL  pcmtest.reset_ioctl
> # not ok 5 pcmtest.reset_ioctl #  Can't read patterns. Probably, module isn't loaded
> # # FAILED: 0 / 5 tests passed.
> # # Totals: pass:0 fail:5 xfail:0 xpass:0 skip:0 error:0
> 
> I haven't completely isolated the issue due to some other breakage
> that's making it harder that it should be to test.  
> 
> A sample full log can be seen at:
> 
>    https://lava.sirena.org.uk/scheduler/job/659576#L1349

Thanks! the exit() inside the skip evaded my grep, I'm testing this:

diff --git a/tools/testing/selftests/alsa/test-pcmtest-driver.c b/tools/testing/selftests/alsa/test-pcmtest-driver.c
index a52ecd43dbe3..7ab81d6f9e05 100644
--- a/tools/testing/selftests/alsa/test-pcmtest-driver.c
+++ b/tools/testing/selftests/alsa/test-pcmtest-driver.c
@@ -127,11 +127,11 @@ FIXTURE_SETUP(pcmtest) {
 	int err;
 
 	if (geteuid())
-		SKIP(exit(-1), "This test needs root to run!");
+		SKIP(exit(KSFT_SKIP), "This test needs root to run!");
 
 	err = read_patterns();
 	if (err)
-		SKIP(exit(-1), "Can't read patterns. Probably, module isn't loaded");
+		SKIP(exit(KSFT_SKIP), "Can't read patterns. Probably, module isn't loaded");
 
 	card_name = malloc(127);
 	ASSERT_NE(card_name, NULL);
diff --git a/tools/testing/selftests/mm/hmm-tests.c b/tools/testing/selftests/mm/hmm-tests.c
index 20294553a5dd..356ba5f3b68c 100644
--- a/tools/testing/selftests/mm/hmm-tests.c
+++ b/tools/testing/selftests/mm/hmm-tests.c
@@ -138,7 +138,7 @@ FIXTURE_SETUP(hmm)
 
 	self->fd = hmm_open(variant->device_number);
 	if (self->fd < 0 && hmm_is_coherent_type(variant->device_number))
-		SKIP(exit(0), "DEVICE_COHERENT not available");
+		SKIP(exit(KSFT_SKIP), "DEVICE_COHERENT not available");
 	ASSERT_GE(self->fd, 0);
 }
 
@@ -149,7 +149,7 @@ FIXTURE_SETUP(hmm2)
 
 	self->fd0 = hmm_open(variant->device_number0);
 	if (self->fd0 < 0 && hmm_is_coherent_type(variant->device_number0))
-		SKIP(exit(0), "DEVICE_COHERENT not available");
+		SKIP(exit(KSFT_SKIP), "DEVICE_COHERENT not available");
 	ASSERT_GE(self->fd0, 0);
 	self->fd1 = hmm_open(variant->device_number1);
 	ASSERT_GE(self->fd1, 0);

> but there's no more context.  I'm also seeing some breakage in the
> seccomp selftests which also use kselftest-harness:
> 
> # #  RUN           TRAP.dfl ...
> # # dfl: Test exited normally instead of by signal (code: 0)
> # #          FAIL  TRAP.dfl
> # not ok 56 TRAP.dfl
> # #  RUN           TRAP.ign ...
> # # ign: Test exited normally instead of by signal (code: 0)
> # #          FAIL  TRAP.ign
> # not ok 57 TRAP.ign

Ugh, I'm guessing vfork() "eats" the signal, IOW grandchild signals,
child exits? vfork() and signals.. I'd rather leave to Kees || Mickael.

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Kees Cook]

On Mon, Mar 04, 2024 at 03:04:11PM -0800, Jakub Kicinski wrote:
> On Mon, 4 Mar 2024 22:20:03 +0000 Mark Brown wrote:
> > On Wed, Feb 28, 2024 at 04:59:07PM -0800, Jakub Kicinski wrote:
> > 
> > > When running selftests for our subsystem in our CI we'd like all
> > > tests to pass. Currently some tests use SKIP for cases they
> > > expect to fail, because the kselftest_harness limits the return
> > > codes to pass/fail/skip. XFAIL which would be a great match
> > > here cannot be used.
> > > 
> > > Remove the no_print handling and use vfork() to run the test in
> > > a different process than the setup. This way we don't need to
> > > pass "failing step" via the exit code. Further clean up the exit
> > > codes so that we can use all KSFT_* values. Rewrite the result
> > > printing to make handling XFAIL/XPASS easier. Support tests
> > > declaring combinations of fixture + variant they expect to fail.  
> > 
> > This series landed in -next today and has caused breakage on all
> > platforms in the ALSA pcmtest-driver test.  When run on systems that
> > don't have the driver it needs loaded the test skip but since this
> > series was merged skipped tests are logged but then reported back as
> > failures:
> > 
> > # selftests: alsa: test-pcmtest-driver
> > # TAP version 13
> > # 1..5
> > # # Starting 5 tests from 1 test cases.
> > # #  RUN           pcmtest.playback ...
> > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > # # playback: Test failed
> > # #          FAIL  pcmtest.playback
> > # not ok 1 pcmtest.playback #  Can't read patterns. Probably, module isn't loaded
> > # #  RUN           pcmtest.capture ...
> > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > # # capture: Test failed
> > # #          FAIL  pcmtest.capture
> > # not ok 2 pcmtest.capture #  Can't read patterns. Probably, module isn't loaded
> > # #  RUN           pcmtest.ni_capture ...
> > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > # # ni_capture: Test failed
> > # #          FAIL  pcmtest.ni_capture
> > # not ok 3 pcmtest.ni_capture #  Can't read patterns. Probably, module isn't loaded
> > # #  RUN           pcmtest.ni_playback ...
> > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > # # ni_playback: Test failed
> > # #          FAIL  pcmtest.ni_playback
> > # not ok 4 pcmtest.ni_playback #  Can't read patterns. Probably, module isn't loaded
> > # #  RUN           pcmtest.reset_ioctl ...
> > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > # # reset_ioctl: Test failed
> > # #          FAIL  pcmtest.reset_ioctl
> > # not ok 5 pcmtest.reset_ioctl #  Can't read patterns. Probably, module isn't loaded
> > # # FAILED: 0 / 5 tests passed.
> > # # Totals: pass:0 fail:5 xfail:0 xpass:0 skip:0 error:0
> > 
> > I haven't completely isolated the issue due to some other breakage
> > that's making it harder that it should be to test.  
> > 
> > A sample full log can be seen at:
> > 
> >    https://lava.sirena.org.uk/scheduler/job/659576#L1349
> 
> Thanks! the exit() inside the skip evaded my grep, I'm testing this:
> 
> diff --git a/tools/testing/selftests/alsa/test-pcmtest-driver.c b/tools/testing/selftests/alsa/test-pcmtest-driver.c
> index a52ecd43dbe3..7ab81d6f9e05 100644
> --- a/tools/testing/selftests/alsa/test-pcmtest-driver.c
> +++ b/tools/testing/selftests/alsa/test-pcmtest-driver.c
> @@ -127,11 +127,11 @@ FIXTURE_SETUP(pcmtest) {
>  	int err;
>  
>  	if (geteuid())
> -		SKIP(exit(-1), "This test needs root to run!");
> +		SKIP(exit(KSFT_SKIP), "This test needs root to run!");
>  
>  	err = read_patterns();
>  	if (err)
> -		SKIP(exit(-1), "Can't read patterns. Probably, module isn't loaded");
> +		SKIP(exit(KSFT_SKIP), "Can't read patterns. Probably, module isn't loaded");
>  
>  	card_name = malloc(127);
>  	ASSERT_NE(card_name, NULL);
> diff --git a/tools/testing/selftests/mm/hmm-tests.c b/tools/testing/selftests/mm/hmm-tests.c
> index 20294553a5dd..356ba5f3b68c 100644
> --- a/tools/testing/selftests/mm/hmm-tests.c
> +++ b/tools/testing/selftests/mm/hmm-tests.c
> @@ -138,7 +138,7 @@ FIXTURE_SETUP(hmm)
>  
>  	self->fd = hmm_open(variant->device_number);
>  	if (self->fd < 0 && hmm_is_coherent_type(variant->device_number))
> -		SKIP(exit(0), "DEVICE_COHERENT not available");
> +		SKIP(exit(KSFT_SKIP), "DEVICE_COHERENT not available");
>  	ASSERT_GE(self->fd, 0);
>  }
>  
> @@ -149,7 +149,7 @@ FIXTURE_SETUP(hmm2)
>  
>  	self->fd0 = hmm_open(variant->device_number0);
>  	if (self->fd0 < 0 && hmm_is_coherent_type(variant->device_number0))
> -		SKIP(exit(0), "DEVICE_COHERENT not available");
> +		SKIP(exit(KSFT_SKIP), "DEVICE_COHERENT not available");
>  	ASSERT_GE(self->fd0, 0);
>  	self->fd1 = hmm_open(variant->device_number1);
>  	ASSERT_GE(self->fd1, 0);
> 
> > but there's no more context.  I'm also seeing some breakage in the
> > seccomp selftests which also use kselftest-harness:
> > 
> > # #  RUN           TRAP.dfl ...
> > # # dfl: Test exited normally instead of by signal (code: 0)
> > # #          FAIL  TRAP.dfl
> > # not ok 56 TRAP.dfl
> > # #  RUN           TRAP.ign ...
> > # # ign: Test exited normally instead of by signal (code: 0)
> > # #          FAIL  TRAP.ign
> > # not ok 57 TRAP.ign
> 
> Ugh, I'm guessing vfork() "eats" the signal, IOW grandchild signals,
> child exits? vfork() and signals.. I'd rather leave to Kees || Mickael.

Oh no, that does seem bad. Since Mickaël is also seeing weird issues,
can we drop the vfork changes for now?

-- 
Kees Cook

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Jakub Kicinski]

On Mon, 4 Mar 2024 15:14:04 -0800 Kees Cook wrote:
> > Ugh, I'm guessing vfork() "eats" the signal, IOW grandchild signals,
> > child exits? vfork() and signals.. I'd rather leave to Kees || Mickael.  
> 
> Oh no, that does seem bad. Since Mickaël is also seeing weird issues,
> can we drop the vfork changes for now?

Seems doable, but won't be a simple revert. "drop" means we'd need 
to bring ->step back. More or less go back to v3.
No preference here, Mickael?

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Kees Cook]

On Mon, Mar 04, 2024 at 03:39:02PM -0800, Jakub Kicinski wrote:
> On Mon, 4 Mar 2024 15:14:04 -0800 Kees Cook wrote:
> > > Ugh, I'm guessing vfork() "eats" the signal, IOW grandchild signals,
> > > child exits? vfork() and signals.. I'd rather leave to Kees || Mickael.  
> > 
> > Oh no, that does seem bad. Since Mickaël is also seeing weird issues,
> > can we drop the vfork changes for now?
> 
> Seems doable, but won't be a simple revert. "drop" means we'd need 
> to bring ->step back. More or less go back to v3.

I think we have to -- other CIs are now showing the most of seccomp
failing now. (And I can confirm this now -- I had only tested seccomp
on earlier versions of the series.)

-- 
Kees Cook

Re: [PATCH v4 02/12] selftests/harness: Merge TEST_F_FORK() into TEST_F()

[Mickaël Salaün]

I think I fixed all reported issues with the following patch.  It always
execute the fixture setup in the child process and execute the teardown
in the child process by default (e.g. for seccomp tests which have
assumptions about that). Only the Landlock teardown tests are executed
in the parent process thanks to the new _metadata->teardown_parent
boolean.  Child signals are always forwarded to the parent process where
__wait_for_test() check that.  This works with seccomp and Landlock
tests, and I think with all the others.  I'll send a v2 of the vfork
patch.

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index ad49832457af..4f192904dfd6 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -382,29 +382,33 @@
 		/* fixture data is alloced, setup, and torn down per call. */ \
 		FIXTURE_DATA(fixture_name) self; \
 		pid_t child = 1; \
+		int status = 0; \
 		memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
 		if (setjmp(_metadata->env) == 0) { \
-			fixture_name##_setup(_metadata, &self, variant->data); \
-			/* Let setup failure terminate early. */ \
-			if (!_metadata->passed || _metadata->skip) \
-				return; \
-			_metadata->setup_completed = true; \
 			/* Use the same _metadata. */ \
 			child = vfork(); \
 			if (child == 0) { \
+				fixture_name##_setup(_metadata, &self, variant->data); \
+				/* Let setup failure terminate early. */ \
+				if (!_metadata->passed || _metadata->skip) \
+					_exit(0); \
+				_metadata->setup_completed = true; \
 				fixture_name##_##test_name(_metadata, &self, variant->data); \
-				_exit(0); \
-			} \
-			if (child < 0) { \
+			} else if (child < 0 || child != waitpid(child, &status, 0)) { \
 				ksft_print_msg("ERROR SPAWNING TEST GRANDCHILD\n"); \
 				_metadata->passed = 0; \
 			} \
 		} \
-		if (child == 0) \
-			/* Child failed and updated the shared _metadata. */ \
+		if (child == 0) { \
+			if (_metadata->setup_completed && !_metadata->teardown_parent) \
+				fixture_name##_teardown(_metadata, &self, variant->data); \
 			_exit(0); \
-		if (_metadata->setup_completed) \
+		} \
+		if (_metadata->setup_completed && _metadata->teardown_parent) \
 			fixture_name##_teardown(_metadata, &self, variant->data); \
+		if (!WIFEXITED(status) && WIFSIGNALED(status)) \
+			/* Forward signal to __wait_for_test(). */ \
+			kill(getpid(), WTERMSIG(status)); \
 		__test_check_assert(_metadata); \
 	} \
 	static struct __test_metadata \
@@ -414,6 +418,7 @@
 		.fixture = &_##fixture_name##_fixture_object, \
 		.termsig = signal, \
 		.timeout = tmout, \
+		.teardown_parent = false, \
 	 }; \
 	static void __attribute__((constructor)) \
 			_register_##fixture_name##_##test_name(void) \
@@ -842,6 +847,7 @@ struct __test_metadata {
 	bool timed_out;	/* did this test timeout instead of exiting? */
 	bool aborted;	/* stopped test due to failed ASSERT */
 	bool setup_completed; /* did setup finish? */
+	bool teardown_parent; /* run teardown in a parent process */
 	jmp_buf env;	/* for exiting out of test early */
 	struct __test_results *results;
 	struct __test_metadata *prev, *next;
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 2d6d9b43d958..1d5952897e05 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -285,6 +285,8 @@ static void prepare_layout_opt(struct __test_metadata *const _metadata,
 
 static void prepare_layout(struct __test_metadata *const _metadata)
 {
+	_metadata->teardown_parent = true;
+
 	prepare_layout_opt(_metadata, &mnt_tmp);
 }
 
@@ -3861,9 +3863,7 @@ FIXTURE_SETUP(layout1_bind)
 
 FIXTURE_TEARDOWN(layout1_bind)
 {
-	set_cap(_metadata, CAP_SYS_ADMIN);
-	EXPECT_EQ(0, umount(dir_s2d2));
-	clear_cap(_metadata, CAP_SYS_ADMIN);
+	/* umount(dir_s2d2)) is handled by namespace lifetime. */
 
 	remove_layout1(_metadata);
 
@@ -4276,9 +4276,8 @@ FIXTURE_TEARDOWN(layout2_overlay)
 	EXPECT_EQ(0, remove_path(lower_fl1));
 	EXPECT_EQ(0, remove_path(lower_do1_fo2));
 	EXPECT_EQ(0, remove_path(lower_fo1));
-	set_cap(_metadata, CAP_SYS_ADMIN);
-	EXPECT_EQ(0, umount(LOWER_BASE));
-	clear_cap(_metadata, CAP_SYS_ADMIN);
+
+	/* umount(LOWER_BASE)) is handled by namespace lifetime. */
 	EXPECT_EQ(0, remove_path(LOWER_BASE));
 
 	EXPECT_EQ(0, remove_path(upper_do1_fu3));
@@ -4287,14 +4286,11 @@ FIXTURE_TEARDOWN(layout2_overlay)
 	EXPECT_EQ(0, remove_path(upper_do1_fo2));
 	EXPECT_EQ(0, remove_path(upper_fo1));
 	EXPECT_EQ(0, remove_path(UPPER_WORK "/work"));
-	set_cap(_metadata, CAP_SYS_ADMIN);
-	EXPECT_EQ(0, umount(UPPER_BASE));
-	clear_cap(_metadata, CAP_SYS_ADMIN);
+
+	/* umount(UPPER_BASE)) is handled by namespace lifetime. */
 	EXPECT_EQ(0, remove_path(UPPER_BASE));
 
-	set_cap(_metadata, CAP_SYS_ADMIN);
-	EXPECT_EQ(0, umount(MERGE_DATA));
-	clear_cap(_metadata, CAP_SYS_ADMIN);
+	/* umount(MERGE_DATA)) is handled by namespace lifetime. */
 	EXPECT_EQ(0, remove_path(MERGE_DATA));
 
 	cleanup_layout(_metadata);
@@ -4691,6 +4687,8 @@ FIXTURE_SETUP(layout3_fs)
 		SKIP(return, "this filesystem is not supported (setup)");
 	}
 
+	_metadata->teardown_parent = true;
+
 	slash = strrchr(variant->file_path, '/');
 	ASSERT_NE(slash, NULL);
 	dir_len = (size_t)slash - (size_t)variant->file_path;


On Mon, Mar 04, 2024 at 08:31:49PM +0100, Mickaël Salaün wrote:
> On Mon, Mar 04, 2024 at 08:27:50PM +0100, Mickaël Salaün wrote:
> > Testing the whole series, I found that some Landlock tests are flaky
> > starting with this patch.  I tried to not use the longjmp in the
> > grandchild but it didn't change.  I suspect missing volatiles but I
> > didn't find the faulty one(s) yet. :/
> > I'll continue investigating tomorrow but help would be much appreciated!
> 
> The issue is with the fs_test.c, often starting with this one:
> 
> #  RUN           layout1.relative_chroot_only ...
> # fs_test.c:294:relative_chroot_only:Expected 0 (0) == umount(TMP_DIR) (-1)
> # fs_test.c:296:relative_chroot_only:Expected 0 (0) == remove_path(TMP_DIR) (16)
> # relative_chroot_only: Test failed
> #          FAIL  layout1.relative_chroot_only
> 
> ...or this one:
> 
> #  RUN           layout3_fs.hostfs.tag_inode_dir_child ...
> # fs_test.c:4707:tag_inode_dir_child:Expected 0 (0) == mkdir(self->dir_path, 0700) (-1)
> # fs_test.c:4709:tag_inode_dir_child:Failed to create directory "tmp/dir": No such file or directory
> # fs_test.c:4724:tag_inode_dir_child:Expected 0 (0) <= fd (-1)
> # fs_test.c:4726:tag_inode_dir_child:Failed to create file "tmp/dir/file": No such file or directory
> # fs_test.c:4729:tag_inode_dir_child:Expected 0 (0) == close(fd) (-1)
> # tag_inode_dir_child: Test failed
> #          FAIL  layout3_fs.hostfs.tag_inode_dir_child
> 

This was because the vfork() wasn't followed by a wait().

> 
> > 
> > 
> > On Wed, Feb 28, 2024 at 04:59:09PM -0800, Jakub Kicinski wrote:
> > > From: Mickaël Salaün <mic@digikod.net>
> > > 
> > > Replace Landlock-specific TEST_F_FORK() with an improved TEST_F() which
> > > brings four related changes:
> > > 
> > > Run TEST_F()'s tests in a grandchild process to make it possible to
> > > drop privileges and delegate teardown to the parent.
> > > 
> > > Compared to TEST_F_FORK(), simplify handling of the test grandchild
> > > process thanks to vfork(2), and makes it generic (e.g. no explicit
> > > conversion between exit code and _metadata).
> > > 
> > > Compared to TEST_F_FORK(), run teardown even when tests failed with an
> > > assert thanks to commit 63e6b2a42342 ("selftests/harness: Run TEARDOWN
> > > for ASSERT failures").
> > > 
> > > Simplify the test harness code by removing the no_print and step fields
> > > which are not used.  I added this feature just after I made
> > > kselftest_harness.h more broadly available but this step counter
> > > remained even though it wasn't needed after all. See commit 369130b63178
> > > ("selftests: Enhance kselftest_harness.h to print which assert failed").
> > > 
> > > Replace spaces with tabs in one line of __TEST_F_IMPL().
> > > 
> > > Cc: Günther Noack <gnoack@google.com>
> > > Cc: Shuah Khan <shuah@kernel.org>
> > > Cc: Will Drewry <wad@chromium.org>
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > Reviewed-by: Kees Cook <keescook@chromium.org>
> > > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > > --
> > > v4:
> > >  - GAND -> GRAND
> > >  - init child to 1, otherwise assert in setup triggers a longjmp
> > >    which in turn reads child without it ever getting initialized
> > >    (or being 0, i.e. we mistakenly assume we're in the grandchild)
> > 
> > Good catch!

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Przemek Kitszel]

On 3/5/24 00:04, Jakub Kicinski wrote:
> On Mon, 4 Mar 2024 22:20:03 +0000 Mark Brown wrote:
>> On Wed, Feb 28, 2024 at 04:59:07PM -0800, Jakub Kicinski wrote:
>>
>>> When running selftests for our subsystem in our CI we'd like all
>>> tests to pass. Currently some tests use SKIP for cases they
>>> expect to fail, because the kselftest_harness limits the return
>>> codes to pass/fail/skip. XFAIL which would be a great match
>>> here cannot be used.
>>>
>>> Remove the no_print handling and use vfork() to run the test in
>>> a different process than the setup. This way we don't need to
>>> pass "failing step" via the exit code. Further clean up the exit
>>> codes so that we can use all KSFT_* values. Rewrite the result
>>> printing to make handling XFAIL/XPASS easier. Support tests
>>> declaring combinations of fixture + variant they expect to fail.
>>
>> This series landed in -next today and has caused breakage on all
>> platforms in the ALSA pcmtest-driver test.  When run on systems that
>> don't have the driver it needs loaded the test skip but since this
>> series was merged skipped tests are logged but then reported back as
>> failures:
>>
>> # selftests: alsa: test-pcmtest-driver
>> # TAP version 13
>> # 1..5
>> # # Starting 5 tests from 1 test cases.
>> # #  RUN           pcmtest.playback ...
>> # #      SKIP      Can't read patterns. Probably, module isn't loaded
>> # # playback: Test failed
>> # #          FAIL  pcmtest.playback
>> # not ok 1 pcmtest.playback #  Can't read patterns. Probably, module isn't loaded
>> # #  RUN           pcmtest.capture ...
>> # #      SKIP      Can't read patterns. Probably, module isn't loaded
>> # # capture: Test failed
>> # #          FAIL  pcmtest.capture
>> # not ok 2 pcmtest.capture #  Can't read patterns. Probably, module isn't loaded
>> # #  RUN           pcmtest.ni_capture ...
>> # #      SKIP      Can't read patterns. Probably, module isn't loaded
>> # # ni_capture: Test failed
>> # #          FAIL  pcmtest.ni_capture
>> # not ok 3 pcmtest.ni_capture #  Can't read patterns. Probably, module isn't loaded
>> # #  RUN           pcmtest.ni_playback ...
>> # #      SKIP      Can't read patterns. Probably, module isn't loaded
>> # # ni_playback: Test failed
>> # #          FAIL  pcmtest.ni_playback
>> # not ok 4 pcmtest.ni_playback #  Can't read patterns. Probably, module isn't loaded
>> # #  RUN           pcmtest.reset_ioctl ...
>> # #      SKIP      Can't read patterns. Probably, module isn't loaded
>> # # reset_ioctl: Test failed
>> # #          FAIL  pcmtest.reset_ioctl
>> # not ok 5 pcmtest.reset_ioctl #  Can't read patterns. Probably, module isn't loaded
>> # # FAILED: 0 / 5 tests passed.
>> # # Totals: pass:0 fail:5 xfail:0 xpass:0 skip:0 error:0
>>
>> I haven't completely isolated the issue due to some other breakage
>> that's making it harder that it should be to test.
>>
>> A sample full log can be seen at:
>>
>>     https://lava.sirena.org.uk/scheduler/job/659576#L1349
> 
> Thanks! the exit() inside the skip evaded my grep, I'm testing this:
> 
> diff --git a/tools/testing/selftests/alsa/test-pcmtest-driver.c b/tools/testing/selftests/alsa/test-pcmtest-driver.c
> index a52ecd43dbe3..7ab81d6f9e05 100644
> --- a/tools/testing/selftests/alsa/test-pcmtest-driver.c
> +++ b/tools/testing/selftests/alsa/test-pcmtest-driver.c
> @@ -127,11 +127,11 @@ FIXTURE_SETUP(pcmtest) {
>   	int err;
>   
>   	if (geteuid())
> -		SKIP(exit(-1), "This test needs root to run!");
> +		SKIP(exit(KSFT_SKIP), "This test needs root to run!");
>   
>   	err = read_patterns();
>   	if (err)
> -		SKIP(exit(-1), "Can't read patterns. Probably, module isn't loaded");
> +		SKIP(exit(KSFT_SKIP), "Can't read patterns. Probably, module isn't loaded");
>   
>   	card_name = malloc(127);
>   	ASSERT_NE(card_name, NULL);
> diff --git a/tools/testing/selftests/mm/hmm-tests.c b/tools/testing/selftests/mm/hmm-tests.c
> index 20294553a5dd..356ba5f3b68c 100644
> --- a/tools/testing/selftests/mm/hmm-tests.c
> +++ b/tools/testing/selftests/mm/hmm-tests.c
> @@ -138,7 +138,7 @@ FIXTURE_SETUP(hmm)
>   
>   	self->fd = hmm_open(variant->device_number);
>   	if (self->fd < 0 && hmm_is_coherent_type(variant->device_number))
> -		SKIP(exit(0), "DEVICE_COHERENT not available");
> +		SKIP(exit(KSFT_SKIP), "DEVICE_COHERENT not available");
>   	ASSERT_GE(self->fd, 0);
>   }
>   
> @@ -149,7 +149,7 @@ FIXTURE_SETUP(hmm2)
>   
>   	self->fd0 = hmm_open(variant->device_number0);
>   	if (self->fd0 < 0 && hmm_is_coherent_type(variant->device_number0))
> -		SKIP(exit(0), "DEVICE_COHERENT not available");
> +		SKIP(exit(KSFT_SKIP), "DEVICE_COHERENT not available");
>   	ASSERT_GE(self->fd0, 0);
>   	self->fd1 = hmm_open(variant->device_number1);
>   	ASSERT_GE(self->fd1, 0);
> 
>> but there's no more context.  I'm also seeing some breakage in the
>> seccomp selftests which also use kselftest-harness:
>>
>> # #  RUN           TRAP.dfl ...
>> # # dfl: Test exited normally instead of by signal (code: 0)
>> # #          FAIL  TRAP.dfl
>> # not ok 56 TRAP.dfl
>> # #  RUN           TRAP.ign ...
>> # # ign: Test exited normally instead of by signal (code: 0)
>> # #          FAIL  TRAP.ign
>> # not ok 57 TRAP.ign
> 
> Ugh, I'm guessing vfork() "eats" the signal, IOW grandchild signals,
> child exits? vfork() and signals.. I'd rather leave to Kees || Mickael.
> 

Hi, sorry for not trying to reproduce it locally and still commenting,
but my vfork() man page says:

| The child must  not  return  from  the current  function  or  call
| exit(3) (which would have the effect of calling exit handlers
| established by the parent process and flushing the parent's stdio(3)
| buffers), but may call _exit(2).

And you still have some exit(3) calls.

[PATCH v1 0/2] Merge TEST_F_FORK() into TEST_F()

[Mickaël Salaün]

Hi,

This is a long due cleanup to merge TEST_F_FORK() into TEST_F().

This second version includes Jakub's "child" default value fix, proper
signal handling (TEST_SIGNAL), and move fixture setup and teardown to
the grandchild process by default.

This also removes the step counter and simplify Landlock's test
teardowns by removing some umount calls.

See https://lore.kernel.org/r/20240229005920.2407409-1-kuba@kernel.org

Regards,

Mickaël Salaün (2):
  selftests/landlock: Redefine TEST_F() as TEST_F_FORK()
  selftests/harness: Merge TEST_F_FORK() into TEST_F()

 tools/testing/selftests/kselftest_harness.h | 72 ++++++++++-----------
 tools/testing/selftests/landlock/common.h   | 58 +----------------
 tools/testing/selftests/landlock/fs_test.c  | 22 +++----
 3 files changed, 48 insertions(+), 104 deletions(-)


base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
-- 
2.44.0

[PATCH v1 1/2] selftests/landlock: Redefine TEST_F() as TEST_F_FORK()

[Mickaël Salaün]

This has the effect of creating a new test process for either TEST_F()
or TEST_F_FORK(), which doesn't change tests but will ease potential
backports.  See next commit for the TEST_F_FORK() merge into TEST_F().

Cc: Günther Noack <gnoack@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Will Drewry <wad@chromium.org>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 tools/testing/selftests/landlock/common.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
index e64bbdf0e86e..f40146d40763 100644
--- a/tools/testing/selftests/landlock/common.h
+++ b/tools/testing/selftests/landlock/common.h
@@ -37,7 +37,7 @@
 		struct __test_metadata *_metadata, \
 		FIXTURE_DATA(fixture_name) *self, \
 		const FIXTURE_VARIANT(fixture_name) *variant); \
-	TEST_F(fixture_name, test_name) \
+	__TEST_F_IMPL(fixture_name, test_name, -1, TEST_TIMEOUT_DEFAULT) \
 	{ \
 		int status; \
 		const pid_t child = fork(); \
@@ -80,6 +80,10 @@
 			__attribute__((unused)) *variant)
 /* clang-format on */
 
+/* Makes backporting easier. */
+#undef TEST_F
+#define TEST_F(fixture_name, test_name) TEST_F_FORK(fixture_name, test_name)
+
 #ifndef landlock_create_ruleset
 static inline int
 landlock_create_ruleset(const struct landlock_ruleset_attr *const attr,
-- 
2.44.0

[PATCH v1 2/2] selftests/harness: Merge TEST_F_FORK() into TEST_F()

[Mickaël Salaün]

Replace Landlock-specific TEST_F_FORK() with an improved TEST_F() which
brings four related changes:

Run TEST_F()'s tests in a grandchild process to make it possible to
drop privileges and delegate teardown to the parent.

Compared to TEST_F_FORK(), simplify handling of the test grandchild
process thanks to vfork(2), and makes it generic (e.g. no explicit
conversion between exit code and _metadata).

Compared to TEST_F_FORK(), run teardown even when tests failed with an
assert thanks to commit 63e6b2a42342 ("selftests/harness: Run TEARDOWN
for ASSERT failures").

By default teardown is run by the grandchild process, but it can be run
by its parent process if _metadata->teardown_parent is set to true.

Update Landlock tests' fixture setup to set _metadata->teardown, and
remove now-invalid umount calls in Landlock test teardowns.

Simplify the test harness code by removing the no_print and step fields
which are not used.  I added this feature just after I made
kselftest_harness.h more broadly available but this step counter
remained even though it wasn't needed after all. See commit 369130b63178
("selftests: Enhance kselftest_harness.h to print which assert failed").

Replace spaces with tabs in one line of __TEST_F_IMPL().

Cc: Günther Noack <gnoack@google.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Will Drewry <wad@chromium.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---

v5:
 - Fix TEST_SIGNAL() by forwarding grandchild signal to parent.
 - By default, run teardown in the grandchild, but allow teardown to be
   run in its parent if _metadata->teardown_parent is set to true.  This
   fixes seccomp tests that have assumptions about the process
   hierarchy.
 - Remove now-invalid umount calls in Landlock test teardowns.

v4:
 - GAND -> GRAND
 - init child to 1, otherwise assert in setup triggers a longjmp
   which in turn reads child without it ever getting initialized
   (or being 0, i.e. we mistakenly assume we're in the grandchild)
---
 tools/testing/selftests/kselftest_harness.h | 72 ++++++++++-----------
 tools/testing/selftests/landlock/common.h   | 62 +-----------------
 tools/testing/selftests/landlock/fs_test.c  | 22 +++----
 3 files changed, 48 insertions(+), 108 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index e05ac8261046..4f192904dfd6 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -95,14 +95,6 @@
  * E.g., #define TH_LOG_ENABLED 1
  *
  * If no definition is provided, logging is enabled by default.
- *
- * If there is no way to print an error message for the process running the
- * test (e.g. not allowed to write to stderr), it is still possible to get the
- * ASSERT_* number for which the test failed.  This behavior can be enabled by
- * writing `_metadata->no_print = true;` before the check sequence that is
- * unable to print.  When an error occur, instead of printing an error message
- * and calling `abort(3)`, the test process call `_exit(2)` with the assert
- * number as argument, which is then printed by the parent process.
  */
 #define TH_LOG(fmt, ...) do { \
 	if (TH_LOG_ENABLED) \
@@ -363,6 +355,11 @@
  * Defines a test that depends on a fixture (e.g., is part of a test case).
  * Very similar to TEST() except that *self* is the setup instance of fixture's
  * datatype exposed for use by the implementation.
+ *
+ * The @test_name code is run in a separate process sharing the same memory
+ * (i.e. vfork), which means that the test process can update its privileges
+ * without impacting the related FIXTURE_TEARDOWN() (e.g. to remove files from
+ * a directory where write access was dropped).
  */
 #define TEST_F(fixture_name, test_name) \
 	__TEST_F_IMPL(fixture_name, test_name, -1, TEST_TIMEOUT_DEFAULT)
@@ -384,17 +381,34 @@
 	{ \
 		/* fixture data is alloced, setup, and torn down per call. */ \
 		FIXTURE_DATA(fixture_name) self; \
+		pid_t child = 1; \
+		int status = 0; \
 		memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
 		if (setjmp(_metadata->env) == 0) { \
-			fixture_name##_setup(_metadata, &self, variant->data); \
-			/* Let setup failure terminate early. */ \
-                       if (!_metadata->passed || _metadata->skip) \
-				return; \
-			_metadata->setup_completed = true; \
-			fixture_name##_##test_name(_metadata, &self, variant->data); \
+			/* Use the same _metadata. */ \
+			child = vfork(); \
+			if (child == 0) { \
+				fixture_name##_setup(_metadata, &self, variant->data); \
+				/* Let setup failure terminate early. */ \
+				if (!_metadata->passed || _metadata->skip) \
+					_exit(0); \
+				_metadata->setup_completed = true; \
+				fixture_name##_##test_name(_metadata, &self, variant->data); \
+			} else if (child < 0 || child != waitpid(child, &status, 0)) { \
+				ksft_print_msg("ERROR SPAWNING TEST GRANDCHILD\n"); \
+				_metadata->passed = 0; \
+			} \
 		} \
-		if (_metadata->setup_completed) \
+		if (child == 0) { \
+			if (_metadata->setup_completed && !_metadata->teardown_parent) \
+				fixture_name##_teardown(_metadata, &self, variant->data); \
+			_exit(0); \
+		} \
+		if (_metadata->setup_completed && _metadata->teardown_parent) \
 			fixture_name##_teardown(_metadata, &self, variant->data); \
+		if (!WIFEXITED(status) && WIFSIGNALED(status)) \
+			/* Forward signal to __wait_for_test(). */ \
+			kill(getpid(), WTERMSIG(status)); \
 		__test_check_assert(_metadata); \
 	} \
 	static struct __test_metadata \
@@ -404,6 +418,7 @@
 		.fixture = &_##fixture_name##_fixture_object, \
 		.termsig = signal, \
 		.timeout = tmout, \
+		.teardown_parent = false, \
 	 }; \
 	static void __attribute__((constructor)) \
 			_register_##fixture_name##_##test_name(void) \
@@ -694,18 +709,12 @@
 	for (; _metadata->trigger; _metadata->trigger = \
 			__bail(_assert, _metadata))
 
-#define __INC_STEP(_metadata) \
-	/* Keep "step" below 255 (which is used for "SKIP" reporting). */	\
-	if (_metadata->passed && _metadata->step < 253) \
-		_metadata->step++;
-
 #define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
 
 #define __EXPECT(_expected, _expected_str, _seen, _seen_str, _t, _assert) do { \
 	/* Avoid multiple evaluation of the cases */ \
 	__typeof__(_expected) __exp = (_expected); \
 	__typeof__(_seen) __seen = (_seen); \
-	if (_assert) __INC_STEP(_metadata); \
 	if (!(__exp _t __seen)) { \
 		/* Report with actual signedness to avoid weird output. */ \
 		switch (is_signed_type(__exp) * 2 + is_signed_type(__seen)) { \
@@ -751,7 +760,6 @@
 #define __EXPECT_STR(_expected, _seen, _t, _assert) do { \
 	const char *__exp = (_expected); \
 	const char *__seen = (_seen); \
-	if (_assert) __INC_STEP(_metadata); \
 	if (!(strcmp(__exp, __seen) _t 0))  { \
 		__TH_LOG("Expected '%s' %s '%s'.", __exp, #_t, __seen); \
 		_metadata->passed = 0; \
@@ -837,10 +845,9 @@ struct __test_metadata {
 	int trigger; /* extra handler after the evaluation */
 	int timeout;	/* seconds to wait for test timeout */
 	bool timed_out;	/* did this test timeout instead of exiting? */
-	__u8 step;
-	bool no_print; /* manual trigger when TH_LOG_STREAM is not available */
 	bool aborted;	/* stopped test due to failed ASSERT */
 	bool setup_completed; /* did setup finish? */
+	bool teardown_parent; /* run teardown in a parent process */
 	jmp_buf env;	/* for exiting out of test early */
 	struct __test_results *results;
 	struct __test_metadata *prev, *next;
@@ -873,11 +880,8 @@ static inline int __bail(int for_realz, struct __test_metadata *t)
 
 static inline void __test_check_assert(struct __test_metadata *t)
 {
-	if (t->aborted) {
-		if (t->no_print)
-			_exit(t->step);
+	if (t->aborted)
 		abort();
-	}
 }
 
 struct __test_metadata *__active_test;
@@ -954,13 +958,12 @@ void __wait_for_test(struct __test_metadata *t)
 			case 0:
 				t->passed = 1;
 				break;
-			/* Other failure, assume step report. */
+			/* Failure */
 			default:
 				t->passed = 0;
 				fprintf(TH_LOG_STREAM,
-					"# %s: Test failed at step #%d\n",
-					t->name,
-					WEXITSTATUS(status));
+					"# %s: Test failed\n",
+					t->name);
 			}
 		}
 	} else if (WIFSIGNALED(status)) {
@@ -1114,8 +1117,6 @@ void __run_test(struct __fixture_metadata *f,
 	t->passed = 1;
 	t->skip = 0;
 	t->trigger = 0;
-	t->step = 1;
-	t->no_print = 0;
 	memset(t->results->reason, 0, sizeof(t->results->reason));
 
 	ksft_print_msg(" RUN           %s%s%s.%s ...\n",
@@ -1137,8 +1138,7 @@ void __run_test(struct __fixture_metadata *f,
 		/* Pass is exit 0 */
 		if (t->passed)
 			_exit(0);
-		/* Something else happened, report the step. */
-		_exit(t->step);
+		_exit(1);
 	} else {
 		__wait_for_test(t);
 	}
diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
index f40146d40763..401e2eb092a3 100644
--- a/tools/testing/selftests/landlock/common.h
+++ b/tools/testing/selftests/landlock/common.h
@@ -23,66 +23,8 @@
 #define __maybe_unused __attribute__((__unused__))
 #endif
 
-/*
- * TEST_F_FORK() is useful when a test drop privileges but the corresponding
- * FIXTURE_TEARDOWN() requires them (e.g. to remove files from a directory
- * where write actions are denied).  For convenience, FIXTURE_TEARDOWN() is
- * also called when the test failed, but not when FIXTURE_SETUP() failed.  For
- * this to be possible, we must not call abort() but instead exit smoothly
- * (hence the step print).
- */
-/* clang-format off */
-#define TEST_F_FORK(fixture_name, test_name) \
-	static void fixture_name##_##test_name##_child( \
-		struct __test_metadata *_metadata, \
-		FIXTURE_DATA(fixture_name) *self, \
-		const FIXTURE_VARIANT(fixture_name) *variant); \
-	__TEST_F_IMPL(fixture_name, test_name, -1, TEST_TIMEOUT_DEFAULT) \
-	{ \
-		int status; \
-		const pid_t child = fork(); \
-		if (child < 0) \
-			abort(); \
-		if (child == 0) { \
-			_metadata->no_print = 1; \
-			fixture_name##_##test_name##_child(_metadata, self, variant); \
-			if (_metadata->skip) \
-				_exit(255); \
-			if (_metadata->passed) \
-				_exit(0); \
-			_exit(_metadata->step); \
-		} \
-		if (child != waitpid(child, &status, 0)) \
-			abort(); \
-		if (WIFSIGNALED(status) || !WIFEXITED(status)) { \
-			_metadata->passed = 0; \
-			_metadata->step = 1; \
-			return; \
-		} \
-		switch (WEXITSTATUS(status)) { \
-		case 0: \
-			_metadata->passed = 1; \
-			break; \
-		case 255: \
-			_metadata->passed = 1; \
-			_metadata->skip = 1; \
-			break; \
-		default: \
-			_metadata->passed = 0; \
-			_metadata->step = WEXITSTATUS(status); \
-			break; \
-		} \
-	} \
-	static void fixture_name##_##test_name##_child( \
-		struct __test_metadata __attribute__((unused)) *_metadata, \
-		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \
-		const FIXTURE_VARIANT(fixture_name) \
-			__attribute__((unused)) *variant)
-/* clang-format on */
-
-/* Makes backporting easier. */
-#undef TEST_F
-#define TEST_F(fixture_name, test_name) TEST_F_FORK(fixture_name, test_name)
+/* TEST_F_FORK() should not be used for new tests. */
+#define TEST_F_FORK(fixture_name, test_name) TEST_F(fixture_name, test_name)
 
 #ifndef landlock_create_ruleset
 static inline int
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 2d6d9b43d958..1d5952897e05 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -285,6 +285,8 @@ static void prepare_layout_opt(struct __test_metadata *const _metadata,
 
 static void prepare_layout(struct __test_metadata *const _metadata)
 {
+	_metadata->teardown_parent = true;
+
 	prepare_layout_opt(_metadata, &mnt_tmp);
 }
 
@@ -3861,9 +3863,7 @@ FIXTURE_SETUP(layout1_bind)
 
 FIXTURE_TEARDOWN(layout1_bind)
 {
-	set_cap(_metadata, CAP_SYS_ADMIN);
-	EXPECT_EQ(0, umount(dir_s2d2));
-	clear_cap(_metadata, CAP_SYS_ADMIN);
+	/* umount(dir_s2d2)) is handled by namespace lifetime. */
 
 	remove_layout1(_metadata);
 
@@ -4276,9 +4276,8 @@ FIXTURE_TEARDOWN(layout2_overlay)
 	EXPECT_EQ(0, remove_path(lower_fl1));
 	EXPECT_EQ(0, remove_path(lower_do1_fo2));
 	EXPECT_EQ(0, remove_path(lower_fo1));
-	set_cap(_metadata, CAP_SYS_ADMIN);
-	EXPECT_EQ(0, umount(LOWER_BASE));
-	clear_cap(_metadata, CAP_SYS_ADMIN);
+
+	/* umount(LOWER_BASE)) is handled by namespace lifetime. */
 	EXPECT_EQ(0, remove_path(LOWER_BASE));
 
 	EXPECT_EQ(0, remove_path(upper_do1_fu3));
@@ -4287,14 +4286,11 @@ FIXTURE_TEARDOWN(layout2_overlay)
 	EXPECT_EQ(0, remove_path(upper_do1_fo2));
 	EXPECT_EQ(0, remove_path(upper_fo1));
 	EXPECT_EQ(0, remove_path(UPPER_WORK "/work"));
-	set_cap(_metadata, CAP_SYS_ADMIN);
-	EXPECT_EQ(0, umount(UPPER_BASE));
-	clear_cap(_metadata, CAP_SYS_ADMIN);
+
+	/* umount(UPPER_BASE)) is handled by namespace lifetime. */
 	EXPECT_EQ(0, remove_path(UPPER_BASE));
 
-	set_cap(_metadata, CAP_SYS_ADMIN);
-	EXPECT_EQ(0, umount(MERGE_DATA));
-	clear_cap(_metadata, CAP_SYS_ADMIN);
+	/* umount(MERGE_DATA)) is handled by namespace lifetime. */
 	EXPECT_EQ(0, remove_path(MERGE_DATA));
 
 	cleanup_layout(_metadata);
@@ -4691,6 +4687,8 @@ FIXTURE_SETUP(layout3_fs)
 		SKIP(return, "this filesystem is not supported (setup)");
 	}
 
+	_metadata->teardown_parent = true;
+
 	slash = strrchr(variant->file_path, '/');
 	ASSERT_NE(slash, NULL);
 	dir_len = (size_t)slash - (size_t)variant->file_path;
-- 
2.44.0

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Mickaël Salaün]

On Tue, Mar 05, 2024 at 04:48:06PM +0100, Przemek Kitszel wrote:
> On 3/5/24 00:04, Jakub Kicinski wrote:
> > On Mon, 4 Mar 2024 22:20:03 +0000 Mark Brown wrote:
> > > On Wed, Feb 28, 2024 at 04:59:07PM -0800, Jakub Kicinski wrote:
> > > 
> > > > When running selftests for our subsystem in our CI we'd like all
> > > > tests to pass. Currently some tests use SKIP for cases they
> > > > expect to fail, because the kselftest_harness limits the return
> > > > codes to pass/fail/skip. XFAIL which would be a great match
> > > > here cannot be used.
> > > > 
> > > > Remove the no_print handling and use vfork() to run the test in
> > > > a different process than the setup. This way we don't need to
> > > > pass "failing step" via the exit code. Further clean up the exit
> > > > codes so that we can use all KSFT_* values. Rewrite the result
> > > > printing to make handling XFAIL/XPASS easier. Support tests
> > > > declaring combinations of fixture + variant they expect to fail.
> > > 
> > > This series landed in -next today and has caused breakage on all
> > > platforms in the ALSA pcmtest-driver test.  When run on systems that
> > > don't have the driver it needs loaded the test skip but since this
> > > series was merged skipped tests are logged but then reported back as
> > > failures:
> > > 
> > > # selftests: alsa: test-pcmtest-driver
> > > # TAP version 13
> > > # 1..5
> > > # # Starting 5 tests from 1 test cases.
> > > # #  RUN           pcmtest.playback ...
> > > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > > # # playback: Test failed
> > > # #          FAIL  pcmtest.playback
> > > # not ok 1 pcmtest.playback #  Can't read patterns. Probably, module isn't loaded
> > > # #  RUN           pcmtest.capture ...
> > > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > > # # capture: Test failed
> > > # #          FAIL  pcmtest.capture
> > > # not ok 2 pcmtest.capture #  Can't read patterns. Probably, module isn't loaded
> > > # #  RUN           pcmtest.ni_capture ...
> > > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > > # # ni_capture: Test failed
> > > # #          FAIL  pcmtest.ni_capture
> > > # not ok 3 pcmtest.ni_capture #  Can't read patterns. Probably, module isn't loaded
> > > # #  RUN           pcmtest.ni_playback ...
> > > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > > # # ni_playback: Test failed
> > > # #          FAIL  pcmtest.ni_playback
> > > # not ok 4 pcmtest.ni_playback #  Can't read patterns. Probably, module isn't loaded
> > > # #  RUN           pcmtest.reset_ioctl ...
> > > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > > # # reset_ioctl: Test failed
> > > # #          FAIL  pcmtest.reset_ioctl
> > > # not ok 5 pcmtest.reset_ioctl #  Can't read patterns. Probably, module isn't loaded
> > > # # FAILED: 0 / 5 tests passed.
> > > # # Totals: pass:0 fail:5 xfail:0 xpass:0 skip:0 error:0
> > > 
> > > I haven't completely isolated the issue due to some other breakage
> > > that's making it harder that it should be to test.
> > > 
> > > A sample full log can be seen at:
> > > 
> > >     https://lava.sirena.org.uk/scheduler/job/659576#L1349
> > 
> > Thanks! the exit() inside the skip evaded my grep, I'm testing this:
> > 
> > diff --git a/tools/testing/selftests/alsa/test-pcmtest-driver.c b/tools/testing/selftests/alsa/test-pcmtest-driver.c
> > index a52ecd43dbe3..7ab81d6f9e05 100644
> > --- a/tools/testing/selftests/alsa/test-pcmtest-driver.c
> > +++ b/tools/testing/selftests/alsa/test-pcmtest-driver.c
> > @@ -127,11 +127,11 @@ FIXTURE_SETUP(pcmtest) {
> >   	int err;
> >   	if (geteuid())
> > -		SKIP(exit(-1), "This test needs root to run!");
> > +		SKIP(exit(KSFT_SKIP), "This test needs root to run!");
> >   	err = read_patterns();
> >   	if (err)
> > -		SKIP(exit(-1), "Can't read patterns. Probably, module isn't loaded");
> > +		SKIP(exit(KSFT_SKIP), "Can't read patterns. Probably, module isn't loaded");
> >   	card_name = malloc(127);
> >   	ASSERT_NE(card_name, NULL);
> > diff --git a/tools/testing/selftests/mm/hmm-tests.c b/tools/testing/selftests/mm/hmm-tests.c
> > index 20294553a5dd..356ba5f3b68c 100644
> > --- a/tools/testing/selftests/mm/hmm-tests.c
> > +++ b/tools/testing/selftests/mm/hmm-tests.c
> > @@ -138,7 +138,7 @@ FIXTURE_SETUP(hmm)
> >   	self->fd = hmm_open(variant->device_number);
> >   	if (self->fd < 0 && hmm_is_coherent_type(variant->device_number))
> > -		SKIP(exit(0), "DEVICE_COHERENT not available");
> > +		SKIP(exit(KSFT_SKIP), "DEVICE_COHERENT not available");
> >   	ASSERT_GE(self->fd, 0);
> >   }
> > @@ -149,7 +149,7 @@ FIXTURE_SETUP(hmm2)
> >   	self->fd0 = hmm_open(variant->device_number0);
> >   	if (self->fd0 < 0 && hmm_is_coherent_type(variant->device_number0))
> > -		SKIP(exit(0), "DEVICE_COHERENT not available");
> > +		SKIP(exit(KSFT_SKIP), "DEVICE_COHERENT not available");
> >   	ASSERT_GE(self->fd0, 0);
> >   	self->fd1 = hmm_open(variant->device_number1);
> >   	ASSERT_GE(self->fd1, 0);
> > 
> > > but there's no more context.  I'm also seeing some breakage in the
> > > seccomp selftests which also use kselftest-harness:
> > > 
> > > # #  RUN           TRAP.dfl ...
> > > # # dfl: Test exited normally instead of by signal (code: 0)
> > > # #          FAIL  TRAP.dfl
> > > # not ok 56 TRAP.dfl
> > > # #  RUN           TRAP.ign ...
> > > # # ign: Test exited normally instead of by signal (code: 0)
> > > # #          FAIL  TRAP.ign
> > > # not ok 57 TRAP.ign
> > 
> > Ugh, I'm guessing vfork() "eats" the signal, IOW grandchild signals,
> > child exits? vfork() and signals.. I'd rather leave to Kees || Mickael.
> > 
> 
> Hi, sorry for not trying to reproduce it locally and still commenting,
> but my vfork() man page says:
> 
> | The child must  not  return  from  the current  function  or  call
> | exit(3) (which would have the effect of calling exit handlers
> | established by the parent process and flushing the parent's stdio(3)
> | buffers), but may call _exit(2).
> 
> And you still have some exit(3) calls.

Correct, exit(3) should be replaced with _exit(2).

> 
> 
> 

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Mickaël Salaün]

On Tue, Mar 05, 2024 at 01:43:14AM -0800, Kees Cook wrote:
> On Mon, Mar 04, 2024 at 03:39:02PM -0800, Jakub Kicinski wrote:
> > On Mon, 4 Mar 2024 15:14:04 -0800 Kees Cook wrote:
> > > > Ugh, I'm guessing vfork() "eats" the signal, IOW grandchild signals,
> > > > child exits? vfork() and signals.. I'd rather leave to Kees || Mickael.  
> > > 
> > > Oh no, that does seem bad. Since Mickaël is also seeing weird issues,
> > > can we drop the vfork changes for now?
> > 
> > Seems doable, but won't be a simple revert. "drop" means we'd need 
> > to bring ->step back. More or less go back to v3.
> 
> I think we have to -- other CIs are now showing the most of seccomp
> failing now. (And I can confirm this now -- I had only tested seccomp
> on earlier versions of the series.)

Sorry for the trouble, I found and fixed the vfork issues.  I tested
with seccomp and Landlock.  You can find a dedicated branch here (with
some Reviewed-by and Acked-by removed because of the changes):
https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=kselftest-xfail-fix

Jakub, please send a v5 series with this updated patch and your
exit/_exit fixes.

> 
> -- 
> Kees Cook
> 

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Mickaël Salaün]

On Tue, Mar 05, 2024 at 05:00:13PM +0100, Mickaël Salaün wrote:
> On Tue, Mar 05, 2024 at 04:48:06PM +0100, Przemek Kitszel wrote:
> > On 3/5/24 00:04, Jakub Kicinski wrote:
> > > On Mon, 4 Mar 2024 22:20:03 +0000 Mark Brown wrote:
> > > > On Wed, Feb 28, 2024 at 04:59:07PM -0800, Jakub Kicinski wrote:
> > > > 
> > > > > When running selftests for our subsystem in our CI we'd like all
> > > > > tests to pass. Currently some tests use SKIP for cases they
> > > > > expect to fail, because the kselftest_harness limits the return
> > > > > codes to pass/fail/skip. XFAIL which would be a great match
> > > > > here cannot be used.
> > > > > 
> > > > > Remove the no_print handling and use vfork() to run the test in
> > > > > a different process than the setup. This way we don't need to
> > > > > pass "failing step" via the exit code. Further clean up the exit
> > > > > codes so that we can use all KSFT_* values. Rewrite the result
> > > > > printing to make handling XFAIL/XPASS easier. Support tests
> > > > > declaring combinations of fixture + variant they expect to fail.
> > > > 
> > > > This series landed in -next today and has caused breakage on all
> > > > platforms in the ALSA pcmtest-driver test.  When run on systems that
> > > > don't have the driver it needs loaded the test skip but since this
> > > > series was merged skipped tests are logged but then reported back as
> > > > failures:
> > > > 
> > > > # selftests: alsa: test-pcmtest-driver
> > > > # TAP version 13
> > > > # 1..5
> > > > # # Starting 5 tests from 1 test cases.
> > > > # #  RUN           pcmtest.playback ...
> > > > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > > > # # playback: Test failed
> > > > # #          FAIL  pcmtest.playback
> > > > # not ok 1 pcmtest.playback #  Can't read patterns. Probably, module isn't loaded
> > > > # #  RUN           pcmtest.capture ...
> > > > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > > > # # capture: Test failed
> > > > # #          FAIL  pcmtest.capture
> > > > # not ok 2 pcmtest.capture #  Can't read patterns. Probably, module isn't loaded
> > > > # #  RUN           pcmtest.ni_capture ...
> > > > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > > > # # ni_capture: Test failed
> > > > # #          FAIL  pcmtest.ni_capture
> > > > # not ok 3 pcmtest.ni_capture #  Can't read patterns. Probably, module isn't loaded
> > > > # #  RUN           pcmtest.ni_playback ...
> > > > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > > > # # ni_playback: Test failed
> > > > # #          FAIL  pcmtest.ni_playback
> > > > # not ok 4 pcmtest.ni_playback #  Can't read patterns. Probably, module isn't loaded
> > > > # #  RUN           pcmtest.reset_ioctl ...
> > > > # #      SKIP      Can't read patterns. Probably, module isn't loaded
> > > > # # reset_ioctl: Test failed
> > > > # #          FAIL  pcmtest.reset_ioctl
> > > > # not ok 5 pcmtest.reset_ioctl #  Can't read patterns. Probably, module isn't loaded
> > > > # # FAILED: 0 / 5 tests passed.
> > > > # # Totals: pass:0 fail:5 xfail:0 xpass:0 skip:0 error:0
> > > > 
> > > > I haven't completely isolated the issue due to some other breakage
> > > > that's making it harder that it should be to test.
> > > > 
> > > > A sample full log can be seen at:
> > > > 
> > > >     https://lava.sirena.org.uk/scheduler/job/659576#L1349
> > > 
> > > Thanks! the exit() inside the skip evaded my grep, I'm testing this:
> > > 
> > > diff --git a/tools/testing/selftests/alsa/test-pcmtest-driver.c b/tools/testing/selftests/alsa/test-pcmtest-driver.c
> > > index a52ecd43dbe3..7ab81d6f9e05 100644
> > > --- a/tools/testing/selftests/alsa/test-pcmtest-driver.c
> > > +++ b/tools/testing/selftests/alsa/test-pcmtest-driver.c
> > > @@ -127,11 +127,11 @@ FIXTURE_SETUP(pcmtest) {
> > >   	int err;
> > >   	if (geteuid())
> > > -		SKIP(exit(-1), "This test needs root to run!");
> > > +		SKIP(exit(KSFT_SKIP), "This test needs root to run!");
> > >   	err = read_patterns();
> > >   	if (err)
> > > -		SKIP(exit(-1), "Can't read patterns. Probably, module isn't loaded");
> > > +		SKIP(exit(KSFT_SKIP), "Can't read patterns. Probably, module isn't loaded");
> > >   	card_name = malloc(127);
> > >   	ASSERT_NE(card_name, NULL);
> > > diff --git a/tools/testing/selftests/mm/hmm-tests.c b/tools/testing/selftests/mm/hmm-tests.c
> > > index 20294553a5dd..356ba5f3b68c 100644
> > > --- a/tools/testing/selftests/mm/hmm-tests.c
> > > +++ b/tools/testing/selftests/mm/hmm-tests.c
> > > @@ -138,7 +138,7 @@ FIXTURE_SETUP(hmm)
> > >   	self->fd = hmm_open(variant->device_number);
> > >   	if (self->fd < 0 && hmm_is_coherent_type(variant->device_number))
> > > -		SKIP(exit(0), "DEVICE_COHERENT not available");
> > > +		SKIP(exit(KSFT_SKIP), "DEVICE_COHERENT not available");
> > >   	ASSERT_GE(self->fd, 0);
> > >   }
> > > @@ -149,7 +149,7 @@ FIXTURE_SETUP(hmm2)
> > >   	self->fd0 = hmm_open(variant->device_number0);
> > >   	if (self->fd0 < 0 && hmm_is_coherent_type(variant->device_number0))
> > > -		SKIP(exit(0), "DEVICE_COHERENT not available");
> > > +		SKIP(exit(KSFT_SKIP), "DEVICE_COHERENT not available");
> > >   	ASSERT_GE(self->fd0, 0);
> > >   	self->fd1 = hmm_open(variant->device_number1);
> > >   	ASSERT_GE(self->fd1, 0);
> > > 
> > > > but there's no more context.  I'm also seeing some breakage in the
> > > > seccomp selftests which also use kselftest-harness:
> > > > 
> > > > # #  RUN           TRAP.dfl ...
> > > > # # dfl: Test exited normally instead of by signal (code: 0)
> > > > # #          FAIL  TRAP.dfl
> > > > # not ok 56 TRAP.dfl
> > > > # #  RUN           TRAP.ign ...
> > > > # # ign: Test exited normally instead of by signal (code: 0)
> > > > # #          FAIL  TRAP.ign
> > > > # not ok 57 TRAP.ign
> > > 
> > > Ugh, I'm guessing vfork() "eats" the signal, IOW grandchild signals,
> > > child exits? vfork() and signals.. I'd rather leave to Kees || Mickael.
> > > 
> > 
> > Hi, sorry for not trying to reproduce it locally and still commenting,
> > but my vfork() man page says:
> > 
> > | The child must  not  return  from  the current  function  or  call
> > | exit(3) (which would have the effect of calling exit handlers
> > | established by the parent process and flushing the parent's stdio(3)
> > | buffers), but may call _exit(2).
> > 
> > And you still have some exit(3) calls.
> 
> Correct, exit(3) should be replaced with _exit(2).

Well, I think we should be good even if some exit(3) calls remain
because the envirenment in which the vfork() call happen is already
dedicated to the running test (with flushed stdio, setpgrp() call), see
__run_test() and the fork() call just before running the
fixture/test/teardown.  Even if the test configures its own exit
handlers, they will not be run by its parent because it never calls
exit(), and the returned function either ends with a call to _exit() or
a signal.

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Jakub Kicinski]

On Tue, 5 Mar 2024 17:05:51 +0100 Mickaël Salaün wrote:
> > I think we have to -- other CIs are now showing the most of seccomp
> > failing now. (And I can confirm this now -- I had only tested seccomp
> > on earlier versions of the series.)  
> 
> Sorry for the trouble, I found and fixed the vfork issues.  I tested
> with seccomp and Landlock.  You can find a dedicated branch here (with
> some Reviewed-by and Acked-by removed because of the changes):
> https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=kselftest-xfail-fix
> 
> Jakub, please send a v5 series with this updated patch and your
> exit/_exit fixes.

DaveM merged this already, unfortunately. Could send your changes
as incremental fixes on top of net-next?

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Mickaël Salaün]

On Tue, Mar 05, 2024 at 10:06:39AM -0800, Jakub Kicinski wrote:
> On Tue, 5 Mar 2024 17:05:51 +0100 Mickaël Salaün wrote:
> > > I think we have to -- other CIs are now showing the most of seccomp
> > > failing now. (And I can confirm this now -- I had only tested seccomp
> > > on earlier versions of the series.)  
> > 
> > Sorry for the trouble, I found and fixed the vfork issues.  I tested
> > with seccomp and Landlock.  You can find a dedicated branch here (with
> > some Reviewed-by and Acked-by removed because of the changes):
> > https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=kselftest-xfail-fix
> > 
> > Jakub, please send a v5 series with this updated patch and your
> > exit/_exit fixes.
> 
> DaveM merged this already, unfortunately. Could send your changes
> as incremental fixes on top of net-next?

Ok, I'll send that today.

[PATCH] selftests/harness: Fix TEST_F()'s vfork handling

[Mickaël Salaün]

Always run fixture setup in the grandchild process, and by default also
run the teardown in the same process.  However, this change makes it
possible to run the teardown in a parent process when
_metadata->teardown_parent is set to true (e.g. in fixture setup).

Fix TEST_SIGNAL() by forwarding grandchild's signal to its parent.  Fix
seccomp tests by running the test setup in the parent of the test
thread, as expected by the related test code.  Fix Landlock tests by
waiting for the grandchild before processing _metadata.

Use of exit(3) in tests should be OK because the environment in which
the vfork(2) call happen is already dedicated to the running test (with
flushed stdio, setpgrp() call), see __run_test() and the call to fork(2)
just before running the setup/test/teardown.  Even if the test
configures its own exit handlers, they will not be run by the parent
because it never calls exit(3), and the test function either ends with a
call to _exit(2) or a signal.

Cc: David S. Miller <davem@davemloft.net>
Cc: Günther Noack <gnoack@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Will Drewry <wad@chromium.org>
Fixes: 0710a1a73fb4 ("selftests/harness: Merge TEST_F_FORK() into TEST_F()")
Link: https://lore.kernel.org/r/20240305201029.1331333-1-mic@digikod.net
---
 tools/testing/selftests/kselftest_harness.h | 28 +++++++++++++--------
 tools/testing/selftests/landlock/fs_test.c  | 22 ++++++++--------
 2 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 634be793ad58..4fd735e48ee7 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -382,29 +382,33 @@
 		/* fixture data is alloced, setup, and torn down per call. */ \
 		FIXTURE_DATA(fixture_name) self; \
 		pid_t child = 1; \
+		int status = 0; \
 		memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
 		if (setjmp(_metadata->env) == 0) { \
-			fixture_name##_setup(_metadata, &self, variant->data); \
-			/* Let setup failure terminate early. */ \
-			if (_metadata->exit_code) \
-				return; \
-			_metadata->setup_completed = true; \
 			/* Use the same _metadata. */ \
 			child = vfork(); \
 			if (child == 0) { \
+				fixture_name##_setup(_metadata, &self, variant->data); \
+				/* Let setup failure terminate early. */ \
+				if (_metadata->exit_code) \
+					_exit(0); \
+				_metadata->setup_completed = true; \
 				fixture_name##_##test_name(_metadata, &self, variant->data); \
-				_exit(0); \
-			} \
-			if (child < 0) { \
+			} else if (child < 0 || child != waitpid(child, &status, 0)) { \
 				ksft_print_msg("ERROR SPAWNING TEST GRANDCHILD\n"); \
 				_metadata->exit_code = KSFT_FAIL; \
 			} \
 		} \
-		if (child == 0) \
-			/* Child failed and updated the shared _metadata. */ \
+		if (child == 0) { \
+			if (_metadata->setup_completed && !_metadata->teardown_parent) \
+				fixture_name##_teardown(_metadata, &self, variant->data); \
 			_exit(0); \
-		if (_metadata->setup_completed) \
+		} \
+		if (_metadata->setup_completed && _metadata->teardown_parent) \
 			fixture_name##_teardown(_metadata, &self, variant->data); \
+		if (!WIFEXITED(status) && WIFSIGNALED(status)) \
+			/* Forward signal to __wait_for_test(). */ \
+			kill(getpid(), WTERMSIG(status)); \
 		__test_check_assert(_metadata); \
 	} \
 	static struct __test_metadata \
@@ -414,6 +418,7 @@
 		.fixture = &_##fixture_name##_fixture_object, \
 		.termsig = signal, \
 		.timeout = tmout, \
+		.teardown_parent = false, \
 	 }; \
 	static void __attribute__((constructor)) \
 			_register_##fixture_name##_##test_name(void) \
@@ -873,6 +878,7 @@ struct __test_metadata {
 	bool timed_out;	/* did this test timeout instead of exiting? */
 	bool aborted;	/* stopped test due to failed ASSERT */
 	bool setup_completed; /* did setup finish? */
+	bool teardown_parent; /* run teardown in a parent process */
 	jmp_buf env;	/* for exiting out of test early */
 	struct __test_results *results;
 	struct __test_metadata *prev, *next;
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 98817a14c91b..9a6036fbf289 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -285,6 +285,8 @@ static void prepare_layout_opt(struct __test_metadata *const _metadata,
 
 static void prepare_layout(struct __test_metadata *const _metadata)
 {
+	_metadata->teardown_parent = true;
+
 	prepare_layout_opt(_metadata, &mnt_tmp);
 }
 
@@ -3861,9 +3863,7 @@ FIXTURE_SETUP(layout1_bind)
 
 FIXTURE_TEARDOWN(layout1_bind)
 {
-	set_cap(_metadata, CAP_SYS_ADMIN);
-	EXPECT_EQ(0, umount(dir_s2d2));
-	clear_cap(_metadata, CAP_SYS_ADMIN);
+	/* umount(dir_s2d2)) is handled by namespace lifetime. */
 
 	remove_layout1(_metadata);
 
@@ -4276,9 +4276,8 @@ FIXTURE_TEARDOWN(layout2_overlay)
 	EXPECT_EQ(0, remove_path(lower_fl1));
 	EXPECT_EQ(0, remove_path(lower_do1_fo2));
 	EXPECT_EQ(0, remove_path(lower_fo1));
-	set_cap(_metadata, CAP_SYS_ADMIN);
-	EXPECT_EQ(0, umount(LOWER_BASE));
-	clear_cap(_metadata, CAP_SYS_ADMIN);
+
+	/* umount(LOWER_BASE)) is handled by namespace lifetime. */
 	EXPECT_EQ(0, remove_path(LOWER_BASE));
 
 	EXPECT_EQ(0, remove_path(upper_do1_fu3));
@@ -4287,14 +4286,11 @@ FIXTURE_TEARDOWN(layout2_overlay)
 	EXPECT_EQ(0, remove_path(upper_do1_fo2));
 	EXPECT_EQ(0, remove_path(upper_fo1));
 	EXPECT_EQ(0, remove_path(UPPER_WORK "/work"));
-	set_cap(_metadata, CAP_SYS_ADMIN);
-	EXPECT_EQ(0, umount(UPPER_BASE));
-	clear_cap(_metadata, CAP_SYS_ADMIN);
+
+	/* umount(UPPER_BASE)) is handled by namespace lifetime. */
 	EXPECT_EQ(0, remove_path(UPPER_BASE));
 
-	set_cap(_metadata, CAP_SYS_ADMIN);
-	EXPECT_EQ(0, umount(MERGE_DATA));
-	clear_cap(_metadata, CAP_SYS_ADMIN);
+	/* umount(MERGE_DATA)) is handled by namespace lifetime. */
 	EXPECT_EQ(0, remove_path(MERGE_DATA));
 
 	cleanup_layout(_metadata);
@@ -4691,6 +4687,8 @@ FIXTURE_SETUP(layout3_fs)
 		SKIP(return, "this filesystem is not supported (setup)");
 	}
 
+	_metadata->teardown_parent = true;
+
 	slash = strrchr(variant->file_path, '/');
 	ASSERT_NE(slash, NULL);
 	dir_len = (size_t)slash - (size_t)variant->file_path;
-- 
2.44.0

Re: [PATCH] selftests/harness: Fix TEST_F()'s vfork handling

[Jakub Kicinski]

On Tue,  5 Mar 2024 21:10:29 +0100 Mickaël Salaün wrote:
> Always run fixture setup in the grandchild process, and by default also
> run the teardown in the same process.  However, this change makes it
> possible to run the teardown in a parent process when
> _metadata->teardown_parent is set to true (e.g. in fixture setup).
> 
> Fix TEST_SIGNAL() by forwarding grandchild's signal to its parent.  Fix
> seccomp tests by running the test setup in the parent of the test
> thread, as expected by the related test code.  Fix Landlock tests by
> waiting for the grandchild before processing _metadata.
> 
> Use of exit(3) in tests should be OK because the environment in which
> the vfork(2) call happen is already dedicated to the running test (with
> flushed stdio, setpgrp() call), see __run_test() and the call to fork(2)
> just before running the setup/test/teardown.  Even if the test
> configures its own exit handlers, they will not be run by the parent
> because it never calls exit(3), and the test function either ends with a
> call to _exit(2) or a signal.
> 
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Günther Noack <gnoack@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Will Drewry <wad@chromium.org>
> Fixes: 0710a1a73fb4 ("selftests/harness: Merge TEST_F_FORK() into TEST_F()")
> Link: https://lore.kernel.org/r/20240305201029.1331333-1-mic@digikod.net

Your S-o-b is missing. Should be enough if you responded with it.

Code LGTM, thanks!

Re: [PATCH] selftests/harness: Fix TEST_F()'s vfork handling

[Kees Cook]

On Tue, Mar 05, 2024 at 09:10:29PM +0100, Mickaël Salaün wrote:
> Always run fixture setup in the grandchild process, and by default also
> run the teardown in the same process.  However, this change makes it
> possible to run the teardown in a parent process when
> _metadata->teardown_parent is set to true (e.g. in fixture setup).
> 
> Fix TEST_SIGNAL() by forwarding grandchild's signal to its parent.  Fix
> seccomp tests by running the test setup in the parent of the test
> thread, as expected by the related test code.  Fix Landlock tests by
> waiting for the grandchild before processing _metadata.
> 
> Use of exit(3) in tests should be OK because the environment in which
> the vfork(2) call happen is already dedicated to the running test (with
> flushed stdio, setpgrp() call), see __run_test() and the call to fork(2)
> just before running the setup/test/teardown.  Even if the test
> configures its own exit handlers, they will not be run by the parent
> because it never calls exit(3), and the test function either ends with a
> call to _exit(2) or a signal.
> 
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Günther Noack <gnoack@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Will Drewry <wad@chromium.org>
> Fixes: 0710a1a73fb4 ("selftests/harness: Merge TEST_F_FORK() into TEST_F()")
> Link: https://lore.kernel.org/r/20240305201029.1331333-1-mic@digikod.net

Sanity-check run of seccomp tests before:

# # Totals: pass:70 fail:21 xfail:0 xpass:0 skip:5 error:0

After:

# # Totals: pass:91 fail:0 xfail:0 xpass:0 skip:5 error:0

Reviewed-by: Kees Cook <keescook@chromium.org>
Tested-by: Kees Cook <keescook@chromium.org>

Thanks for a quick fix!

-Kees

-- 
Kees Cook

Re: [PATCH] selftests/harness: Fix TEST_F()'s vfork handling

[Mickaël Salaün]

On Tue, Mar 05, 2024 at 12:25:54PM -0800, Jakub Kicinski wrote:
> On Tue,  5 Mar 2024 21:10:29 +0100 Mickaël Salaün wrote:
> > Always run fixture setup in the grandchild process, and by default also
> > run the teardown in the same process.  However, this change makes it
> > possible to run the teardown in a parent process when
> > _metadata->teardown_parent is set to true (e.g. in fixture setup).
> > 
> > Fix TEST_SIGNAL() by forwarding grandchild's signal to its parent.  Fix
> > seccomp tests by running the test setup in the parent of the test
> > thread, as expected by the related test code.  Fix Landlock tests by
> > waiting for the grandchild before processing _metadata.
> > 
> > Use of exit(3) in tests should be OK because the environment in which
> > the vfork(2) call happen is already dedicated to the running test (with
> > flushed stdio, setpgrp() call), see __run_test() and the call to fork(2)
> > just before running the setup/test/teardown.  Even if the test
> > configures its own exit handlers, they will not be run by the parent
> > because it never calls exit(3), and the test function either ends with a
> > call to _exit(2) or a signal.
> > 
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Günther Noack <gnoack@google.com>
> > Cc: Jakub Kicinski <kuba@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Shuah Khan <shuah@kernel.org>
> > Cc: Will Drewry <wad@chromium.org>
> > Fixes: 0710a1a73fb4 ("selftests/harness: Merge TEST_F_FORK() into TEST_F()")
> > Link: https://lore.kernel.org/r/20240305201029.1331333-1-mic@digikod.net

Signed-off-by: Mickaël Salaün <mic@digikod.net>

> 
> Your S-o-b is missing. Should be enough if you responded with it.
> 
> Code LGTM, thanks!
> 

Re: [PATCH] selftests/harness: Fix TEST_F()'s vfork handling

[Mickaël Salaün]

On Wed, Mar 06, 2024 at 08:25:45AM +0100, Mickaël Salaün wrote:
> On Tue, Mar 05, 2024 at 12:25:54PM -0800, Jakub Kicinski wrote:
> > On Tue,  5 Mar 2024 21:10:29 +0100 Mickaël Salaün wrote:
> > > Always run fixture setup in the grandchild process, and by default also
> > > run the teardown in the same process.  However, this change makes it
> > > possible to run the teardown in a parent process when
> > > _metadata->teardown_parent is set to true (e.g. in fixture setup).
> > > 
> > > Fix TEST_SIGNAL() by forwarding grandchild's signal to its parent.  Fix
> > > seccomp tests by running the test setup in the parent of the test
> > > thread, as expected by the related test code.  Fix Landlock tests by
> > > waiting for the grandchild before processing _metadata.
> > > 
> > > Use of exit(3) in tests should be OK because the environment in which
> > > the vfork(2) call happen is already dedicated to the running test (with
> > > flushed stdio, setpgrp() call), see __run_test() and the call to fork(2)
> > > just before running the setup/test/teardown.  Even if the test
> > > configures its own exit handlers, they will not be run by the parent
> > > because it never calls exit(3), and the test function either ends with a
> > > call to _exit(2) or a signal.
> > > 
> > > Cc: David S. Miller <davem@davemloft.net>
> > > Cc: Günther Noack <gnoack@google.com>
> > > Cc: Jakub Kicinski <kuba@kernel.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Mark Brown <broonie@kernel.org>
> > > Cc: Shuah Khan <shuah@kernel.org>
> > > Cc: Will Drewry <wad@chromium.org>
> > > Fixes: 0710a1a73fb4 ("selftests/harness: Merge TEST_F_FORK() into TEST_F()")
> > > Link: https://lore.kernel.org/r/20240305201029.1331333-1-mic@digikod.net
> 
> Signed-off-by: Mickaël Salaün <mic@digikod.net>

Reported-by: Mark Brown <broonie@kernel.org>

> 
> > 
> > Your S-o-b is missing. Should be enough if you responded with it.
> > 
> > Code LGTM, thanks!
> > 

Re: [PATCH] selftests/harness: Fix TEST_F()'s vfork handling

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]

On Tue, Mar 05, 2024 at 09:10:29PM +0100, Mickaël Salaün wrote:
> Always run fixture setup in the grandchild process, and by default also
> run the teardown in the same process.  However, this change makes it
> possible to run the teardown in a parent process when
> _metadata->teardown_parent is set to true (e.g. in fixture setup).

This fixes the seccomp isues for me, thanks:

Tested-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] selftests/harness: Fix TEST_F()'s vfork handling

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  5 Mar 2024 21:10:29 +0100 you wrote:
> Always run fixture setup in the grandchild process, and by default also
> run the teardown in the same process.  However, this change makes it
> possible to run the teardown in a parent process when
> _metadata->teardown_parent is set to true (e.g. in fixture setup).
> 
> Fix TEST_SIGNAL() by forwarding grandchild's signal to its parent.  Fix
> seccomp tests by running the test setup in the parent of the test
> thread, as expected by the related test code.  Fix Landlock tests by
> waiting for the grandchild before processing _metadata.
> 
> [...]

Here is the summary with links:
  - selftests/harness: Fix TEST_F()'s vfork handling
    https://git.kernel.org/netdev/net-next/c/41cca0542d7c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v4 10/12] selftests: kselftest_harness: let PASS / FAIL provide diagnostic

[Muhammad Usama Anjum]

On 2/29/24 5:59 AM, Jakub Kicinski wrote:
> Switch to printing KTAP line for PASS / FAIL with ksft_test_result_code(),
> this gives us the ability to report diagnostic messages.
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/kselftest_harness.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 5b0592e4b7a4..b643a577f9e1 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -1143,14 +1143,13 @@ void __run_test(struct __fixture_metadata *f,
>  
>  	if (t->results->reason[0])
>  		diagnostic = t->results->reason;
> +	else if (t->exit_code == KSFT_PASS || t->exit_code == KSFT_FAIL)
> +		diagnostic = NULL;
>  	else
>  		diagnostic = "unknown";
>  
> -	if (t->exit_code == KSFT_SKIP)
> -		ksft_test_result_code(t->exit_code, test_name,
> -				      "%s", diagnostic);
> -	else
> -		ksft_test_result(__test_passed(t), "%s\n", test_name);
> +	ksft_test_result_code(t->exit_code, test_name,
> +			      diagnostic ? "%s" : "", diagnostic);
We are getting the following annoying warning while using clang:

./../kselftest_harness.h:1207:30: warning: format string is empty
[-Wformat-zero-length]
 1207 |            diagnostic ? "%s" : "", diagnostic);
      |                                 ^~
1 warning generated.

I've tried to look for solution. In my opinion, the best solution would be
to just revert this patch. Any thoughts?

>  }
>  
>  static int test_harness_run(int argc, char **argv)

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Sean Christopherson]

+kvm

On Tue, Mar 05, 2024, Mickaël Salaün wrote:
> On Tue, Mar 05, 2024 at 01:43:14AM -0800, Kees Cook wrote:
> > On Mon, Mar 04, 2024 at 03:39:02PM -0800, Jakub Kicinski wrote:
> > > On Mon, 4 Mar 2024 15:14:04 -0800 Kees Cook wrote:
> > > > > Ugh, I'm guessing vfork() "eats" the signal, IOW grandchild signals,
> > > > > child exits? vfork() and signals.. I'd rather leave to Kees || Mickael.  
> > > > 
> > > > Oh no, that does seem bad. Since Mickaël is also seeing weird issues,
> > > > can we drop the vfork changes for now?
> > > 
> > > Seems doable, but won't be a simple revert. "drop" means we'd need 
> > > to bring ->step back. More or less go back to v3.
> > 
> > I think we have to -- other CIs are now showing the most of seccomp
> > failing now. (And I can confirm this now -- I had only tested seccomp
> > on earlier versions of the series.)
> 
> Sorry for the trouble, I found and fixed the vfork issues.

Heh, you found and fixed _some of_ the vfork issues.  This whole mess completely
breaks existing tests that use TEST_F() and exit() with non-zero values to
indicate failure, including failures that occur during FIXTURE_SETUP().

E.g. all of the KVM selftests that use KVM_ONE_VCPU_TEST() are broken and will
always show all tests as passing.

The below gets things working for KVM selftests again, but (a) I have no idea if
it's a complete fix, (b) I don't know if it will break other users of the harness,
and (c) I don't understand why spawning a grandchild is the default behavior, i.e.
why usage that has zero need of separating teardown from setup+run is subjected to
the complexity of the handful of tests that do.

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 4fd735e48ee7..24e95828976f 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -391,7 +391,7 @@
                                fixture_name##_setup(_metadata, &self, variant->data); \
                                /* Let setup failure terminate early. */ \
                                if (_metadata->exit_code) \
-                                       _exit(0); \
+                                       _exit(_metadata->exit_code); \
                                _metadata->setup_completed = true; \
                                fixture_name##_##test_name(_metadata, &self, variant->data); \
                        } else if (child < 0 || child != waitpid(child, &status, 0)) { \
@@ -406,8 +406,10 @@
                } \
                if (_metadata->setup_completed && _metadata->teardown_parent) \
                        fixture_name##_teardown(_metadata, &self, variant->data); \
-               if (!WIFEXITED(status) && WIFSIGNALED(status)) \
-                       /* Forward signal to __wait_for_test(). */ \
+               /* Forward exit codes and signals to __wait_for_test(). */ \
+               if (WIFEXITED(status)) \
+                       _exit(WEXITSTATUS(status)); \
+               else if (WIFSIGNALED(status)) \
                        kill(getpid(), WTERMSIG(status)); \
                __test_check_assert(_metadata); \
        } \

Re: [PATCH v4 00/12] selftests: kselftest_harness: support using xfail

[Mickaël Salaün]

On Thu, May 02, 2024 at 11:42:29AM GMT, Sean Christopherson wrote:
> +kvm
> 
> On Tue, Mar 05, 2024, Mickaël Salaün wrote:
> > On Tue, Mar 05, 2024 at 01:43:14AM -0800, Kees Cook wrote:
> > > On Mon, Mar 04, 2024 at 03:39:02PM -0800, Jakub Kicinski wrote:
> > > > On Mon, 4 Mar 2024 15:14:04 -0800 Kees Cook wrote:
> > > > > > Ugh, I'm guessing vfork() "eats" the signal, IOW grandchild signals,
> > > > > > child exits? vfork() and signals.. I'd rather leave to Kees || Mickael.  
> > > > > 
> > > > > Oh no, that does seem bad. Since Mickaël is also seeing weird issues,
> > > > > can we drop the vfork changes for now?
> > > > 
> > > > Seems doable, but won't be a simple revert. "drop" means we'd need 
> > > > to bring ->step back. More or less go back to v3.
> > > 
> > > I think we have to -- other CIs are now showing the most of seccomp
> > > failing now. (And I can confirm this now -- I had only tested seccomp
> > > on earlier versions of the series.)
> > 
> > Sorry for the trouble, I found and fixed the vfork issues.
> 
> Heh, you found and fixed _some of_ the vfork issues.  This whole mess completely
> breaks existing tests that use TEST_F() and exit() with non-zero values to
> indicate failure, including failures that occur during FIXTURE_SETUP().
> 
> E.g. all of the KVM selftests that use KVM_ONE_VCPU_TEST() are broken and will
> always show all tests as passing.
> 
> The below gets things working for KVM selftests again, but (a) I have no idea if
> it's a complete fix, (b) I don't know if it will break other users of the harness,
> and (c) I don't understand why spawning a grandchild is the default behavior, i.e.
> why usage that has zero need of separating teardown from setup+run is subjected to
> the complexity of the handful of tests that do.

Thanks for the fix.  I think it covers almost all cases.  I'd handle the
same way the remaining _exit() though.  The grandchild changes was a
long due patch from the time I added kselftest_harness.h and forked the
TEST_F() macro.  I'll send a new patch series with this fix.

> 
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 4fd735e48ee7..24e95828976f 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -391,7 +391,7 @@
>                                 fixture_name##_setup(_metadata, &self, variant->data); \
>                                 /* Let setup failure terminate early. */ \
>                                 if (_metadata->exit_code) \
> -                                       _exit(0); \
> +                                       _exit(_metadata->exit_code); \
>                                 _metadata->setup_completed = true; \
>                                 fixture_name##_##test_name(_metadata, &self, variant->data); \
>                         } else if (child < 0 || child != waitpid(child, &status, 0)) { \
> @@ -406,8 +406,10 @@
>                 } \
>                 if (_metadata->setup_completed && _metadata->teardown_parent) \
>                         fixture_name##_teardown(_metadata, &self, variant->data); \
> -               if (!WIFEXITED(status) && WIFSIGNALED(status)) \
> -                       /* Forward signal to __wait_for_test(). */ \
> +               /* Forward exit codes and signals to __wait_for_test(). */ \
> +               if (WIFEXITED(status)) \
> +                       _exit(WEXITSTATUS(status)); \
> +               else if (WIFSIGNALED(status)) \
>                         kill(getpid(), WTERMSIG(status)); \
>                 __test_check_assert(_metadata); \
>         } \
>