Unverified Commit 130e4280 authored by Mickaël Salaün's avatar Mickaël Salaün
Browse files

selftests/harness: Fix tests timeout and race condition 

We cannot use CLONE_VFORK because we also need to wait for the timeout
signal.

Restore tests timeout by using the original fork() call in __run_test()
but also in __TEST_F_IMPL().  Also fix a race condition when waiting for
the test child process.

Because test metadata are shared between test processes, only the
parent process must set the test PID (child).  Otherwise, t->pid may be
set to zero, leading to inconsistent error cases:

  #  RUN           layout1.rule_on_mountpoint ...
  # rule_on_mountpoint: Test ended in some other way [127]
  #            OK  layout1.rule_on_mountpoint
  ok 20 layout1.rule_on_mountpoint

As safeguards, initialize the "status" variable with a valid exit code,
and handle unknown test exits as errors.

The use of fork() introduces a new race condition in landlock/fs_test.c
which seems to be specific to hostfs bind mounts, but I haven't found
the root cause and it's difficult to trigger.  I'll try to fix it with
another patch.

Cc: Christian Brauner <brauner@kernel.org>
Cc: Günther Noack <gnoack@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Will Drewry <wad@chromium.org>
Cc: stable@vger.kernel.org
Closes: https://lore.kernel.org/r/9341d4db-5e21-418c-bf9e-9ae2da7877e1@sirena.org.uk
Fixes: a86f1890 ("selftests/harness: Fix interleaved scheduling leading to race conditions")
Fixes: 24cf65a6 ("selftests/harness: Share _metadata between forked processes")
Link: https://lore.kernel.org/r/20240621180605.834676-1-mic@digikod.net


Tested-by: default avatarMark Brown <broonie@kernel.org>
Signed-off-by: default avatarMickaël Salaün <mic@digikod.net>
parent f2661062

tools/testing/selftests/kselftest_harness.h

+24 −19
Original line number Diff line number Diff line
@@ -66,8 +66,6 @@ 
#include <sys/wait.h>

#include <unistd.h>

#include <setjmp.h>

#include <syscall.h>

#include <linux/sched.h>



#include "kselftest.h"
@@ -82,17 +80,6 @@ 
#  define TH_LOG_ENABLED 1

#endif



/* Wait for the child process to end but without sharing memory mapping. */

static inline pid_t clone3_vfork(void)

{

	struct clone_args args = {

		.flags = CLONE_VFORK,

		.exit_signal = SIGCHLD,

	};



	return syscall(__NR_clone3, &args, sizeof(args));

}



/**

 * TH_LOG()

 *
@@ -437,7 +424,7 @@ static inline pid_t clone3_vfork(void) 
		} \

		if (setjmp(_metadata->env) == 0) { \

			/* _metadata and potentially self are shared with all forks. */ \

			child = clone3_vfork(); \

			child = fork(); \

			if (child == 0) { \

				fixture_name##_setup(_metadata, self, variant->data); \

				/* Let setup failure terminate early. */ \
@@ -1016,7 +1003,14 @@ void __wait_for_test(struct __test_metadata *t) 
		.sa_flags = SA_SIGINFO,

	};

	struct sigaction saved_action;

	int status;

	/*

	 * Sets status so that WIFEXITED(status) returns true and

	 * WEXITSTATUS(status) returns KSFT_FAIL.  This safe default value

	 * should never be evaluated because of the waitpid(2) check and

	 * SIGALRM handling.

	 */

	int status = KSFT_FAIL << 8;

	int child;



	if (sigaction(SIGALRM, &action, &saved_action)) {

		t->exit_code = KSFT_FAIL;
@@ -1028,7 +1022,15 @@ void __wait_for_test(struct __test_metadata *t) 
	__active_test = t;

	t->timed_out = false;

	alarm(t->timeout);

	waitpid(t->pid, &status, 0);

	child = waitpid(t->pid, &status, 0);

	if (child == -1 && errno != EINTR) {

		t->exit_code = KSFT_FAIL;

		fprintf(TH_LOG_STREAM,

			"# %s: Failed to wait for PID %d (errno: %d)\n",

			t->name, t->pid, errno);

		return;

	}



	alarm(0);

	if (sigaction(SIGALRM, &saved_action, NULL)) {

		t->exit_code = KSFT_FAIL;
@@ -1083,6 +1085,7 @@ void __wait_for_test(struct __test_metadata *t) 
				WTERMSIG(status));

		}

	} else {

		t->exit_code = KSFT_FAIL;

		fprintf(TH_LOG_STREAM,

			"# %s: Test ended in some other way [%u]\n",

			t->name,
@@ -1218,6 +1221,7 @@ void __run_test(struct __fixture_metadata *f, 
	struct __test_xfail *xfail;

	char test_name[1024];

	const char *diagnostic;

	int child;



	/* reset test struct */

	t->exit_code = KSFT_PASS;
@@ -1236,15 +1240,16 @@ void __run_test(struct __fixture_metadata *f, 
	fflush(stdout);

	fflush(stderr);



	t->pid = clone3_vfork();

	if (t->pid < 0) {

	child = fork();

	if (child < 0) {

		ksft_print_msg("ERROR SPAWNING TEST CHILD\n");

		t->exit_code = KSFT_FAIL;

	} else if (t->pid == 0) {

	} else if (child == 0) {

		setpgrp();

		t->fn(t, variant);

		_exit(t->exit_code);

	} else {

		t->pid = child;

		__wait_for_test(t);

	}

	ksft_print_msg("         %4s  %s\n",