| Message ID | 20240305201029.1331333-1-mic@digikod.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 41cca0542d7c79b6be3920a917f643c075ad1561 |
| Headers | show |
| Series | selftests/harness: Fix TEST_F()'s vfork handling | expand |
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!
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
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! >
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! > >
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>
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!
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;
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(-)