Message-ID: <202404261245.DC9A268FF@keescook>
Date: Fri, 26 Apr 2024 12:47:16 -0700
From: Kees Cook <keescook@...omium.org>
To: Mickaël Salaün <mic@...ikod.net>
Cc: Christian Brauner <brauner@...nel.org>,
	Jakub Kicinski <kuba@...nel.org>, Mark Brown <broonie@...nel.org>,
	Shengyu Li <shengyu.li.evgeny@...il.com>,
	Shuah Khan <shuah@...nel.org>,
	"David S . Miller" <davem@...emloft.net>,
	Günther Noack <gnoack@...gle.com>,
	Will Drewry <wad@...omium.org>,
	kernel test robot <oliver.sang@...el.com>,
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v1 5/5] selftests/harness: Fix vfork() side effects and
 uncaught errors

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@...nel.org>
> Cc: David S. Miller <davem@...emloft.net>
> Cc: Günther Noack <gnoack@...gle.com>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Mark Brown <broonie@...nel.org>
> Cc: Shuah Khan <shuah@...nel.org>
> Cc: Will Drewry <wad@...omium.org>
> Reported-by: kernel test robot <oliver.sang@...el.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@...ikod.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

Powered by blists - more mailing lists