Skip to content

Commit 130e428

Browse files
committed
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 <[email protected]> Cc: Günther Noack <[email protected]> Cc: Jakub Kicinski <[email protected]> Cc: Kees Cook <[email protected]> Cc: Shuah Khan <[email protected]> Cc: Will Drewry <[email protected]> Cc: [email protected] Closes: https://lore.kernel.org/r/[email protected] Fixes: a86f189 ("selftests/harness: Fix interleaved scheduling leading to race conditions") Fixes: 24cf65a ("selftests/harness: Share _metadata between forked processes") Link: https://lore.kernel.org/r/[email protected] Tested-by: Mark Brown <[email protected]> Signed-off-by: Mickaël Salaün <[email protected]>
1 parent f266106 commit 130e428

File tree

1 file changed

+24
-19
lines changed

1 file changed

+24
-19
lines changed

tools/testing/selftests/kselftest_harness.h

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,6 @@
6666
#include <sys/wait.h>
6767
#include <unistd.h>
6868
#include <setjmp.h>
69-
#include <syscall.h>
70-
#include <linux/sched.h>
7169

7270
#include "kselftest.h"
7371

@@ -82,17 +80,6 @@
8280
# define TH_LOG_ENABLED 1
8381
#endif
8482

85-
/* Wait for the child process to end but without sharing memory mapping. */
86-
static inline pid_t clone3_vfork(void)
87-
{
88-
struct clone_args args = {
89-
.flags = CLONE_VFORK,
90-
.exit_signal = SIGCHLD,
91-
};
92-
93-
return syscall(__NR_clone3, &args, sizeof(args));
94-
}
95-
9683
/**
9784
* TH_LOG()
9885
*
@@ -437,7 +424,7 @@ static inline pid_t clone3_vfork(void)
437424
} \
438425
if (setjmp(_metadata->env) == 0) { \
439426
/* _metadata and potentially self are shared with all forks. */ \
440-
child = clone3_vfork(); \
427+
child = fork(); \
441428
if (child == 0) { \
442429
fixture_name##_setup(_metadata, self, variant->data); \
443430
/* Let setup failure terminate early. */ \
@@ -1016,7 +1003,14 @@ void __wait_for_test(struct __test_metadata *t)
10161003
.sa_flags = SA_SIGINFO,
10171004
};
10181005
struct sigaction saved_action;
1019-
int status;
1006+
/*
1007+
* Sets status so that WIFEXITED(status) returns true and
1008+
* WEXITSTATUS(status) returns KSFT_FAIL. This safe default value
1009+
* should never be evaluated because of the waitpid(2) check and
1010+
* SIGALRM handling.
1011+
*/
1012+
int status = KSFT_FAIL << 8;
1013+
int child;
10201014

10211015
if (sigaction(SIGALRM, &action, &saved_action)) {
10221016
t->exit_code = KSFT_FAIL;
@@ -1028,7 +1022,15 @@ void __wait_for_test(struct __test_metadata *t)
10281022
__active_test = t;
10291023
t->timed_out = false;
10301024
alarm(t->timeout);
1031-
waitpid(t->pid, &status, 0);
1025+
child = waitpid(t->pid, &status, 0);
1026+
if (child == -1 && errno != EINTR) {
1027+
t->exit_code = KSFT_FAIL;
1028+
fprintf(TH_LOG_STREAM,
1029+
"# %s: Failed to wait for PID %d (errno: %d)\n",
1030+
t->name, t->pid, errno);
1031+
return;
1032+
}
1033+
10321034
alarm(0);
10331035
if (sigaction(SIGALRM, &saved_action, NULL)) {
10341036
t->exit_code = KSFT_FAIL;
@@ -1083,6 +1085,7 @@ void __wait_for_test(struct __test_metadata *t)
10831085
WTERMSIG(status));
10841086
}
10851087
} else {
1088+
t->exit_code = KSFT_FAIL;
10861089
fprintf(TH_LOG_STREAM,
10871090
"# %s: Test ended in some other way [%u]\n",
10881091
t->name,
@@ -1218,6 +1221,7 @@ void __run_test(struct __fixture_metadata *f,
12181221
struct __test_xfail *xfail;
12191222
char test_name[1024];
12201223
const char *diagnostic;
1224+
int child;
12211225

12221226
/* reset test struct */
12231227
t->exit_code = KSFT_PASS;
@@ -1236,15 +1240,16 @@ void __run_test(struct __fixture_metadata *f,
12361240
fflush(stdout);
12371241
fflush(stderr);
12381242

1239-
t->pid = clone3_vfork();
1240-
if (t->pid < 0) {
1243+
child = fork();
1244+
if (child < 0) {
12411245
ksft_print_msg("ERROR SPAWNING TEST CHILD\n");
12421246
t->exit_code = KSFT_FAIL;
1243-
} else if (t->pid == 0) {
1247+
} else if (child == 0) {
12441248
setpgrp();
12451249
t->fn(t, variant);
12461250
_exit(t->exit_code);
12471251
} else {
1252+
t->pid = child;
12481253
__wait_for_test(t);
12491254
}
12501255
ksft_print_msg(" %4s %s\n",

0 commit comments

Comments
 (0)