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;

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.

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.

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?

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;

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); \
    } \
  
  

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