From: "Mickaël Salaün" <mic@digikod.net>
To: Jakub Kicinski <kuba@kernel.org>, Mark Brown <broonie@kernel.org>
Cc: davem@davemloft.net, netdev@vger.kernel.org, edumazet@google.com,
pabeni@redhat.com, shuah@kernel.org,
linux-kselftest@vger.kernel.org,
linux-security-module@vger.kernel.org, keescook@chromium.org,
jakub@cloudflare.com, "Günther Noack" <gnoack@google.com>,
"Will Drewry" <wad@chromium.org>
Subject: Re: [PATCH v4 02/12] selftests/harness: Merge TEST_F_FORK() into TEST_F()
Date: Tue, 5 Mar 2024 16:47:04 +0100 [thread overview]
Message-ID: <20240305.eth2Ohcawa7u@digikod.net> (raw)
In-Reply-To: <20240304.ceje1phaiFei@digikod.net>
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!
next prev parent reply other threads:[~2024-03-05 15:47 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-29 0:59 [PATCH v4 00/12] selftests: kselftest_harness: support using xfail Jakub Kicinski
2024-02-29 0:59 ` [PATCH v4 01/12] selftests/landlock: Redefine TEST_F() as TEST_F_FORK() Jakub Kicinski
2024-02-29 0:59 ` [PATCH v4 02/12] selftests/harness: Merge TEST_F_FORK() into TEST_F() Jakub Kicinski
2024-03-04 19:27 ` Mickaël Salaün
2024-03-04 19:31 ` Mickaël Salaün
2024-03-05 15:47 ` Mickaël Salaün [this message]
2024-03-05 15:56 ` [PATCH v1 0/2] " Mickaël Salaün
2024-03-05 15:56 ` [PATCH v1 1/2] selftests/landlock: Redefine TEST_F() as TEST_F_FORK() Mickaël Salaün
2024-03-05 15:56 ` [PATCH v1 2/2] selftests/harness: Merge TEST_F_FORK() into TEST_F() Mickaël Salaün
2024-02-29 0:59 ` [PATCH v4 03/12] selftests: kselftest_harness: use KSFT_* exit codes Jakub Kicinski
2024-02-29 0:59 ` [PATCH v4 04/12] selftests: kselftest_harness: generate test name once Jakub Kicinski
2024-02-29 0:59 ` [PATCH v4 05/12] selftests: kselftest_harness: save full exit code in metadata Jakub Kicinski
2024-02-29 0:59 ` [PATCH v4 06/12] selftests: kselftest_harness: use exit code to store skip Jakub Kicinski
2024-02-29 0:59 ` [PATCH v4 07/12] selftests: kselftest: add ksft_test_result_code(), handling all exit codes Jakub Kicinski
2024-02-29 0:59 ` [PATCH v4 08/12] selftests: kselftest_harness: print test name for SKIP Jakub Kicinski
2024-02-29 0:59 ` [PATCH v4 09/12] selftests: kselftest_harness: separate diagnostic message with # in ksft_test_result_code() Jakub Kicinski
2024-02-29 0:59 ` [PATCH v4 10/12] selftests: kselftest_harness: let PASS / FAIL provide diagnostic Jakub Kicinski
2024-04-16 14:11 ` Muhammad Usama Anjum
2024-02-29 0:59 ` [PATCH v4 11/12] selftests: kselftest_harness: support using xfail Jakub Kicinski
2024-02-29 0:59 ` [PATCH v4 12/12] selftests: ip_local_port_range: use XFAIL instead of SKIP Jakub Kicinski
2024-02-29 20:19 ` Jakub Sitnicki
2024-02-29 23:25 ` Xin Long
2024-03-01 10:40 ` Jakub Sitnicki
2024-03-01 10:40 ` [PATCH v4 00/12] selftests: kselftest_harness: support using xfail patchwork-bot+netdevbpf
2024-03-04 22:20 ` Mark Brown
2024-03-04 23:04 ` Jakub Kicinski
2024-03-04 23:14 ` Kees Cook
2024-03-04 23:39 ` Jakub Kicinski
2024-03-05 9:43 ` Kees Cook
2024-03-05 16:05 ` Mickaël Salaün
2024-03-05 18:06 ` Jakub Kicinski
2024-03-05 19:14 ` Mickaël Salaün
2024-03-05 20:10 ` [PATCH] selftests/harness: Fix TEST_F()'s vfork handling Mickaël Salaün
2024-03-05 20:25 ` Jakub Kicinski
2024-03-06 7:25 ` Mickaël Salaün
2024-03-06 7:32 ` Mickaël Salaün
2024-03-05 20:31 ` Kees Cook
2024-03-06 13:25 ` Mark Brown
2024-03-07 4:40 ` patchwork-bot+netdevbpf
2024-05-02 18:42 ` [PATCH v4 00/12] selftests: kselftest_harness: support using xfail Sean Christopherson
2024-05-02 21:07 ` Mickaël Salaün
2024-03-05 15:48 ` Przemek Kitszel
2024-03-05 16:00 ` Mickaël Salaün
2024-03-05 16:39 ` Mickaël Salaün
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240305.eth2Ohcawa7u@digikod.net \
--to=mic@digikod.net \
--cc=broonie@kernel.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=gnoack@google.com \
--cc=jakub@cloudflare.com \
--cc=keescook@chromium.org \
--cc=kuba@kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=shuah@kernel.org \
--cc=wad@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).