From: "Mickaël Salaün" <mic@digikod.net>
To: Christian Brauner <brauner@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jakub Kicinski <kuba@kernel.org>,
Kees Cook <keescook@chromium.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
Mark Brown <broonie@kernel.org>, Sasha Levin <sashal@kernel.org>,
Sean Christopherson <seanjc@google.com>,
Shengyu Li <shengyu.li.evgeny@gmail.com>,
Shuah Khan <shuah@kernel.org>,
Shuah Khan <skhan@linuxfoundation.org>
Cc: "Mickaël Salaün" <mic@digikod.net>,
"Bagas Sanjaya" <bagasdotme@gmail.com>,
"Brendan Higgins" <brendanhiggins@google.com>,
"David Gow" <davidgow@google.com>,
"David S . Miller" <davem@davemloft.net>,
"Florian Fainelli" <florian.fainelli@broadcom.com>,
"Günther Noack" <gnoack@google.com>,
"Jon Hunter" <jonathanh@nvidia.com>, "Ron Economos" <re@w6rz.net>,
"Ronald Warsow" <rwarsow@gmx.de>,
"Stephen Rothwell" <sfr@canb.auug.org.au>,
"Will Drewry" <wad@chromium.org>,
"kernel test robot" <oliver.sang@intel.com>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest@vger.kernel.org, netdev@vger.kernel.org,
stable@vger.kernel.org
Subject: [PATCH v6 09/10] selftests/harness: Fix vfork() side effects
Date: Mon, 6 May 2024 18:55:17 +0200 [thread overview]
Message-ID: <20240506165518.474504-10-mic@digikod.net> (raw)
In-Reply-To: <20240506165518.474504-1-mic@digikod.net>
Setting the time namespace with CLONE_NEWTIME returns -EUSERS if the
calling thread shares memory with another thread (because of the shared
vDSO), which is the case when it is created with vfork().
Fix pidfd_setns_test by replacing test harness's vfork() call with a
clone3() call with CLONE_VFORK, and an explicit sharing of the
_metadata and self objects.
Replace _metadata->teardown_parent with a new FIXTURE_TEARDOWN_PARENT()
helper that can replace FIXTURE_TEARDOWN(). This is a cleaner approach
and it enables to selectively share the fixture data between the child
process running tests and the parent process running the fixture
teardown. This also avoids updating several tests to not rely on the
self object's copy-on-write property (e.g. storing the returned value of
a fork() call).
Cc: Christian Brauner <brauner@kernel.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: Günther Noack <gnoack@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Will Drewry <wad@chromium.org>
Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202403291015.1fcfa957-oliver.sang@intel.com
Fixes: 0710a1a73fb4 ("selftests/harness: Merge TEST_F_FORK() into TEST_F()")
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20240506165518.474504-10-mic@digikod.net
---
Changes since v1:
* Split changes (suggested by Kees).
* Improve documentation.
* Remove the static fixture_name##_teardown_parent initialisation to
false (as suggested by checkpatch.pl).
---
tools/testing/selftests/kselftest_harness.h | 66 ++++++++++++++++-----
tools/testing/selftests/landlock/fs_test.c | 16 ++---
2 files changed, 57 insertions(+), 25 deletions(-)
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index ea78bec5856f..eb25f7c11949 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -294,6 +294,32 @@ static inline pid_t clone3_vfork(void)
* A bare "return;" statement may be used to return early.
*/
#define FIXTURE_TEARDOWN(fixture_name) \
+ static const bool fixture_name##_teardown_parent; \
+ __FIXTURE_TEARDOWN(fixture_name)
+
+/**
+ * FIXTURE_TEARDOWN_PARENT()
+ * *_metadata* is included so that EXPECT_*, ASSERT_* etc. work correctly.
+ *
+ * @fixture_name: fixture name
+ *
+ * .. code-block:: c
+ *
+ * FIXTURE_TEARDOWN_PARENT(fixture_name) { implementation }
+ *
+ * Same as FIXTURE_TEARDOWN() but run this code in a parent process. This
+ * enables the test process to drop its privileges without impacting the
+ * related FIXTURE_TEARDOWN_PARENT() (e.g. to remove files from a directory
+ * where write access was dropped).
+ *
+ * To make it possible for the parent process to use *self*, share (MAP_SHARED)
+ * the fixture data between all forked processes.
+ */
+#define FIXTURE_TEARDOWN_PARENT(fixture_name) \
+ static const bool fixture_name##_teardown_parent = true; \
+ __FIXTURE_TEARDOWN(fixture_name)
+
+#define __FIXTURE_TEARDOWN(fixture_name) \
void fixture_name##_teardown( \
struct __test_metadata __attribute__((unused)) *_metadata, \
FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \
@@ -368,10 +394,11 @@ static inline pid_t clone3_vfork(void)
* 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).
+ * The _metadata object is shared (MAP_SHARED) with all the potential forked
+ * processes, which enables them to use EXCEPT_*() and ASSERT_*().
+ *
+ * The *self* object is only shared with the potential forked processes if
+ * FIXTURE_TEARDOWN_PARENT() is used instead of FIXTURE_TEARDOWN().
*/
#define TEST_F(fixture_name, test_name) \
__TEST_F_IMPL(fixture_name, test_name, -1, TEST_TIMEOUT_DEFAULT)
@@ -392,39 +419,49 @@ static inline pid_t clone3_vfork(void)
struct __fixture_variant_metadata *variant) \
{ \
/* fixture data is alloced, setup, and torn down per call. */ \
- FIXTURE_DATA(fixture_name) self; \
+ FIXTURE_DATA(fixture_name) self_private, *self = NULL; \
pid_t child = 1; \
int status = 0; \
/* Makes sure there is only one teardown, even when child forks again. */ \
bool *teardown = mmap(NULL, sizeof(*teardown), \
PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); \
*teardown = false; \
- memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
+ if (sizeof(*self) > 0) { \
+ if (fixture_name##_teardown_parent) { \
+ self = mmap(NULL, sizeof(*self), PROT_READ | PROT_WRITE, \
+ MAP_SHARED | MAP_ANONYMOUS, -1, 0); \
+ } else { \
+ memset(&self_private, 0, sizeof(self_private)); \
+ self = &self_private; \
+ } \
+ } \
if (setjmp(_metadata->env) == 0) { \
- /* Use the same _metadata. */ \
- child = vfork(); \
+ /* _metadata and potentially self are shared with all forks. */ \
+ child = clone3_vfork(); \
if (child == 0) { \
- fixture_name##_setup(_metadata, &self, variant->data); \
+ 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); \
+ 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->exit_code = KSFT_FAIL; \
} \
} \
if (child == 0) { \
- if (_metadata->setup_completed && !_metadata->teardown_parent && \
+ if (_metadata->setup_completed && !fixture_name##_teardown_parent && \
__sync_bool_compare_and_swap(teardown, false, true)) \
- fixture_name##_teardown(_metadata, &self, variant->data); \
+ fixture_name##_teardown(_metadata, self, variant->data); \
_exit(0); \
} \
- if (_metadata->setup_completed && _metadata->teardown_parent && \
+ if (_metadata->setup_completed && fixture_name##_teardown_parent && \
__sync_bool_compare_and_swap(teardown, false, true)) \
- fixture_name##_teardown(_metadata, &self, variant->data); \
+ fixture_name##_teardown(_metadata, self, variant->data); \
munmap(teardown, sizeof(*teardown)); \
+ if (self && fixture_name##_teardown_parent) \
+ munmap(self, sizeof(*self)); \
if (!WIFEXITED(status) && WIFSIGNALED(status)) \
/* Forward signal to __wait_for_test(). */ \
kill(getpid(), WTERMSIG(status)); \
@@ -895,7 +932,6 @@ 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 1e2cffde02b5..27744524df51 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -286,8 +286,6 @@ 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);
}
@@ -316,7 +314,7 @@ FIXTURE_SETUP(layout0)
prepare_layout(_metadata);
}
-FIXTURE_TEARDOWN(layout0)
+FIXTURE_TEARDOWN_PARENT(layout0)
{
cleanup_layout(_metadata);
}
@@ -379,7 +377,7 @@ FIXTURE_SETUP(layout1)
create_layout1(_metadata);
}
-FIXTURE_TEARDOWN(layout1)
+FIXTURE_TEARDOWN_PARENT(layout1)
{
remove_layout1(_metadata);
@@ -3692,7 +3690,7 @@ FIXTURE_SETUP(ftruncate)
create_file(_metadata, file1_s1d1);
}
-FIXTURE_TEARDOWN(ftruncate)
+FIXTURE_TEARDOWN_PARENT(ftruncate)
{
EXPECT_EQ(0, remove_path(file1_s1d1));
cleanup_layout(_metadata);
@@ -3870,7 +3868,7 @@ FIXTURE_SETUP(layout1_bind)
clear_cap(_metadata, CAP_SYS_ADMIN);
}
-FIXTURE_TEARDOWN(layout1_bind)
+FIXTURE_TEARDOWN_PARENT(layout1_bind)
{
/* umount(dir_s2d2)) is handled by namespace lifetime. */
@@ -4275,7 +4273,7 @@ FIXTURE_SETUP(layout2_overlay)
clear_cap(_metadata, CAP_SYS_ADMIN);
}
-FIXTURE_TEARDOWN(layout2_overlay)
+FIXTURE_TEARDOWN_PARENT(layout2_overlay)
{
if (self->skip_test)
SKIP(return, "overlayfs is not supported (teardown)");
@@ -4708,8 +4706,6 @@ FIXTURE_SETUP(layout3_fs)
SKIP(return, "this filesystem is not supported (setup)");
}
- _metadata->teardown_parent = true;
-
prepare_layout_opt(_metadata, &variant->mnt);
/* Creates directory when required. */
@@ -4743,7 +4739,7 @@ FIXTURE_SETUP(layout3_fs)
free(dir_path);
}
-FIXTURE_TEARDOWN(layout3_fs)
+FIXTURE_TEARDOWN_PARENT(layout3_fs)
{
if (self->skip_test)
SKIP(return, "this filesystem is not supported (teardown)");
--
2.45.0
next prev parent reply other threads:[~2024-05-06 16:55 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-06 16:55 [PATCH v6 00/10] Fix Kselftest's vfork() side effects Mickaël Salaün
2024-05-06 16:55 ` [PATCH v6 01/10] selftests/pidfd: Fix config for pidfd_setns_test Mickaël Salaün
2024-05-06 16:55 ` [PATCH v6 02/10] selftests/landlock: Fix FS tests when run on a private mount point Mickaël Salaün
2024-05-06 16:55 ` [PATCH v6 03/10] selftests/harness: Fix fixture teardown Mickaël Salaün
2024-05-06 16:55 ` [PATCH v6 04/10] selftests/harness: Fix interleaved scheduling leading to race conditions Mickaël Salaün
2024-05-06 16:55 ` [PATCH v6 05/10] selftests/landlock: Do not allocate memory in fixture data Mickaël Salaün
2024-05-06 16:55 ` [PATCH v6 06/10] selftests/harness: Constify fixture variants Mickaël Salaün
2024-05-06 16:55 ` [PATCH v6 07/10] selftests/pidfd: Fix wrong expectation Mickaël Salaün
2024-05-06 16:55 ` [PATCH v6 08/10] selftests/harness: Share _metadata between forked processes Mickaël Salaün
2024-05-06 16:55 ` Mickaël Salaün [this message]
2024-05-06 16:55 ` [PATCH v6 10/10] selftests/harness: Handle TEST_F()'s explicit exit codes Mickaël Salaün
2024-05-06 17:04 ` [PATCH v6 00/10] Fix Kselftest's vfork() side effects Kees Cook
2024-05-07 14:12 ` Sean Christopherson
2024-05-07 15:42 ` 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=20240506165518.474504-10-mic@digikod.net \
--to=mic@digikod.net \
--cc=bagasdotme@gmail.com \
--cc=brauner@kernel.org \
--cc=brendanhiggins@google.com \
--cc=broonie@kernel.org \
--cc=davem@davemloft.net \
--cc=davidgow@google.com \
--cc=florian.fainelli@broadcom.com \
--cc=gnoack@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=jonathanh@nvidia.com \
--cc=keescook@chromium.org \
--cc=kuba@kernel.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=oliver.sang@intel.com \
--cc=re@w6rz.net \
--cc=rwarsow@gmx.de \
--cc=sashal@kernel.org \
--cc=seanjc@google.com \
--cc=sfr@canb.auug.org.au \
--cc=shengyu.li.evgeny@gmail.com \
--cc=shuah@kernel.org \
--cc=skhan@linuxfoundation.org \
--cc=stable@vger.kernel.org \
--cc=torvalds@linux-foundation.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).