Re: [PATCH v1 5/5] selftests/harness: Fix vfork() side effects and uncaught errors

Kees Cook
2024-04-26T19:47:17Z

On Fri, Apr 26, 2024 at 07:22:52PM +0200, Mickaël Salaün wrote:
> 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
> __test_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).
>
> In the Landlock filesystem tests, don't allocate self->dir_path in the
> test process because this would not be visible in the
> FIXTURE_TEARDOWN_PARENT() process when not sharing the memory mapping.
>
> Unconditionally share _metadata between all forked processes, which
> enables to actually catch errors (which were previously ignored).
>
> Replace a wrong EXPECT_GT(self->child_pid_exited, 0) with EXPECT_GE(),
> which is now actually tested on the parent and child sides.
>
> FIXTURE_VARIANT_ADD() doesn't need to be MAP_SHARED because it should
> not be modified: it is already passed as const pointers to
> FIXTURE_TEARDOWN(). Make that explicit by constifying the variants
> declarations.

This patch makes at least(?) 3 different logical changes. Can you split
these up a bit; I think it would make review a bit easier.

I don't quite understand why the need for the explicit shared memory
setup for the fixture metadata? Is this to deal with the vfork?

-Kees

>
> 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: Kees Cook <keescook@chromium.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()")
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20240426172252.1862930-6-mic@digikod.net
> ---
> tools/testing/selftests/kselftest_harness.h | 88 +++++++++++++------
> tools/testing/selftests/landlock/fs_test.c | 73 ++++++++-------
> .../selftests/pidfd/pidfd_setns_test.c | 2 +-
> 3 files changed, 103 insertions(+), 60 deletions(-)
>
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index d2dd246a3843..a19d01c0b7a7 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -295,6 +295,32 @@ static pid_t __attribute__((__unused__)) 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 = false; \
> + __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, \
> @@ -339,7 +365,7 @@ static pid_t __attribute__((__unused__)) clone3_vfork(void)
> * variant.
> */
> #define FIXTURE_VARIANT_ADD(fixture_name, variant_name) \
> - extern FIXTURE_VARIANT(fixture_name) \
> + extern const FIXTURE_VARIANT(fixture_name) \
> _##fixture_name##_##variant_name##_variant; \
> static struct __fixture_variant_metadata \
> _##fixture_name##_##variant_name##_object = \
> @@ -351,7 +377,7 @@ static pid_t __attribute__((__unused__)) clone3_vfork(void)
> __register_fixture_variant(&_##fixture_name##_fixture_object, \
> &_##fixture_name##_##variant_name##_object); \
> } \
> - FIXTURE_VARIANT(fixture_name) \
> + const FIXTURE_VARIANT(fixture_name) \
> _##fixture_name##_##variant_name##_variant =
>
> /**
> @@ -369,10 +395,11 @@ static pid_t __attribute__((__unused__)) 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 __test_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)
> @@ -393,57 +420,65 @@ static pid_t __attribute__((__unused__)) 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)); \
> __test_check_assert(_metadata); \
> } \
> - static struct __test_metadata \
> - _##fixture_name##_##test_name##_object = { \
> - .name = #test_name, \
> - .fn = &wrapper_##fixture_name##_##test_name, \
> - .fixture = &_##fixture_name##_fixture_object, \
> - .termsig = signal, \
> - .timeout = tmout, \
> - .teardown_parent = false, \
> - }; \
> static void __attribute__((constructor)) \
> _register_##fixture_name##_##test_name(void) \
> { \
> - __register_test(&_##fixture_name##_##test_name##_object); \
> + struct __test_metadata *object = mmap(NULL, sizeof(*object), \
> + PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); \
> + object->name = #test_name; \
> + object->fn = &wrapper_##fixture_name##_##test_name; \
> + object->fixture = &_##fixture_name##_fixture_object; \
> + object->termsig = signal; \
> + object->timeout = tmout; \
> + __register_test(object); \
> } \
> static void fixture_name##_##test_name( \
> struct __test_metadata __attribute__((unused)) *_metadata, \
> @@ -898,7 +933,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 46b9effd53e4..27744524df51 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -9,6 +9,7 @@
>
> #define _GNU_SOURCE
> #include <fcntl.h>
> +#include <libgen.h>
> #include <linux/landlock.h>
> #include <linux/magic.h>
> #include <sched.h>
> @@ -285,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);
> }
>
> @@ -315,7 +314,7 @@ FIXTURE_SETUP(layout0)
> prepare_layout(_metadata);
> }
>
> -FIXTURE_TEARDOWN(layout0)
> +FIXTURE_TEARDOWN_PARENT(layout0)
> {
> cleanup_layout(_metadata);
> }
> @@ -378,7 +377,7 @@ FIXTURE_SETUP(layout1)
> create_layout1(_metadata);
> }
>
> -FIXTURE_TEARDOWN(layout1)
> +FIXTURE_TEARDOWN_PARENT(layout1)
> {
> remove_layout1(_metadata);
>
> @@ -3691,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);
> @@ -3869,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. */
>
> @@ -4274,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)");
> @@ -4624,7 +4623,6 @@ FIXTURE(layout3_fs)
> {
> bool has_created_dir;
> bool has_created_file;
> - char *dir_path;
> bool skip_test;
> };
>
> @@ -4683,11 +4681,24 @@ FIXTURE_VARIANT_ADD(layout3_fs, hostfs) {
> .cwd_fs_magic = HOSTFS_SUPER_MAGIC,
> };
>
> +static char *dirname_alloc(const char *path)
> +{
> + char *dup;
> +
> + if (!path)
> + return NULL;
> +
> + dup = strdup(path);
> + if (!dup)
> + return NULL;
> +
> + return dirname(dup);
> +}
> +
> FIXTURE_SETUP(layout3_fs)
> {
> struct stat statbuf;
> - const char *slash;
> - size_t dir_len;
> + char *dir_path = dirname_alloc(variant->file_path);
>
> if (!supports_filesystem(variant->mnt.type) ||
> !cwd_matches_fs(variant->cwd_fs_magic)) {
> @@ -4695,27 +4706,15 @@ 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;
> - ASSERT_LT(0, dir_len);
> - self->dir_path = malloc(dir_len + 1);
> - self->dir_path[dir_len] = '\0';
> - strncpy(self->dir_path, variant->file_path, dir_len);
> -
> prepare_layout_opt(_metadata, &variant->mnt);
>
> /* Creates directory when required. */
> - if (stat(self->dir_path, &statbuf)) {
> + if (stat(dir_path, &statbuf)) {
> set_cap(_metadata, CAP_DAC_OVERRIDE);
> - EXPECT_EQ(0, mkdir(self->dir_path, 0700))
> + EXPECT_EQ(0, mkdir(dir_path, 0700))
> {
> TH_LOG("Failed to create directory \"%s\": %s",
> - self->dir_path, strerror(errno));
> - free(self->dir_path);
> - self->dir_path = NULL;
> + dir_path, strerror(errno));
> }
> self->has_created_dir = true;
> clear_cap(_metadata, CAP_DAC_OVERRIDE);
> @@ -4736,9 +4735,11 @@ FIXTURE_SETUP(layout3_fs)
> self->has_created_file = true;
> clear_cap(_metadata, CAP_DAC_OVERRIDE);
> }
> +
> + free(dir_path);
> }
>
> -FIXTURE_TEARDOWN(layout3_fs)
> +FIXTURE_TEARDOWN_PARENT(layout3_fs)
> {
> if (self->skip_test)
> SKIP(return, "this filesystem is not supported (teardown)");
> @@ -4754,16 +4755,17 @@ FIXTURE_TEARDOWN(layout3_fs)
> }
>
> if (self->has_created_dir) {
> + char *dir_path = dirname_alloc(variant->file_path);
> +
> set_cap(_metadata, CAP_DAC_OVERRIDE);
> /*
> * Don't check for error because the directory might already
> * have been removed (cf. release_inode test).
> */
> - rmdir(self->dir_path);
> + rmdir(dir_path);
> clear_cap(_metadata, CAP_DAC_OVERRIDE);
> + free(dir_path);
> }
> - free(self->dir_path);
> - self->dir_path = NULL;
>
> cleanup_layout(_metadata);
> }
> @@ -4830,7 +4832,10 @@ TEST_F_FORK(layout3_fs, tag_inode_dir_mnt)
>
> TEST_F_FORK(layout3_fs, tag_inode_dir_child)
> {
> - layer3_fs_tag_inode(_metadata, self, variant, self->dir_path);
> + char *dir_path = dirname_alloc(variant->file_path);
> +
> + layer3_fs_tag_inode(_metadata, self, variant, dir_path);
> + free(dir_path);
> }
>
> TEST_F_FORK(layout3_fs, tag_inode_file)
> @@ -4857,9 +4862,13 @@ TEST_F_FORK(layout3_fs, release_inodes)
> if (self->has_created_file)
> EXPECT_EQ(0, remove_path(variant->file_path));
>
> - if (self->has_created_dir)
> + if (self->has_created_dir) {
> + char *dir_path = dirname_alloc(variant->file_path);
> +
> /* Don't check for error because of cgroup specificities. */
> - remove_path(self->dir_path);
> + remove_path(dir_path);
> + free(dir_path);
> + }
>
> ruleset_fd =
> create_ruleset(_metadata, LANDLOCK_ACCESS_FS_READ_DIR, layer1);
> diff --git a/tools/testing/selftests/pidfd/pidfd_setns_test.c b/tools/testing/selftests/pidfd/pidfd_setns_test.c
> index 6e2f2cd400ca..47746b0c6acd 100644
> --- a/tools/testing/selftests/pidfd/pidfd_setns_test.c
> +++ b/tools/testing/selftests/pidfd/pidfd_setns_test.c
> @@ -158,7 +158,7 @@ FIXTURE_SETUP(current_nsset)
> /* Create task that exits right away. */
> self->child_pid_exited = create_child(&self->child_pidfd_exited,
> CLONE_NEWUSER | CLONE_NEWNET);
> - EXPECT_GT(self->child_pid_exited, 0);
> + EXPECT_GE(self->child_pid_exited, 0);
>
> if (self->child_pid_exited == 0)
> _exit(EXIT_SUCCESS);
> --
> 2.44.0
>

--
Kees Cook