Hi,
As reported by Kernel Test Robot [1], some pidfd tests fail. This is
due to the use of vfork() which introduced some side effects.
Similarly, while making it more generic, a previous commit made some
Landlock file system tests flaky, and subject to the host's file system
mount configuration.
This series fixes all these side effects by replacing vfork() with
clone3() and CLONE_VFORK, which is cleaner (no arbitrary shared memory)
and makes the Kselftest framework more robust.
I tried different approaches and I found this one to be the cleaner and
less invasive for current test cases.
This second series split the latest patch into 5 patches.
I successfully ran the following tests (using TEST_F and
fork/clone/clone3) with this series:
- landlock:fs_test
- landlock:net_test
- landlock:ptrace_test
- move_mount_set_group:move_mount_set_group_test
- net/af_unix:scm_pidfd
- perf_events:remove_on_exec
- pidfd:pidfd_getfd_test
- pidfd:pidfd_setns_test
- seccomp:seccomp_bpf
- user_events:abi_test
[1] https://lore.kernel.org/oe-lkp/[email protected]
Previous version:
v1: https://lore.kernel.org/r/[email protected]
Regards,
Mickaël Salaün (9):
selftests/pidfd: Fix config for pidfd_setns_test
selftests/landlock: Fix FS tests when run on a private mount point
selftests/harness: Fix fixture teardown
selftests/harness: Fix interleaved scheduling leading to race
conditions
selftests/landlock: Do not allocate memory in fixture data
selftests/harness: Constify fixture variants
selftests/pidfd: Fix wrong expectation
selftests/harness: Share _metadata between forked processes
selftests/harness: Fix vfork() side effects
tools/testing/selftests/kselftest_harness.h | 113 +++++++++++++-----
tools/testing/selftests/landlock/fs_test.c | 83 ++++++++-----
tools/testing/selftests/pidfd/config | 2 +
.../selftests/pidfd/pidfd_setns_test.c | 2 +-
4 files changed, 135 insertions(+), 65 deletions(-)
base-commit: e67572cd2204894179d89bd7b984072f19313b03
--
2.44.0
Fix a race condition when running several FIXTURE_TEARDOWN() managing
the same resource. This fixes a race condition in the Landlock file
system tests when creating or unmounting the same directory.
Using clone3() with CLONE_VFORK guarantees that the child and grandchild
test processes are sequentially scheduled. This is implemented with a
new clone3_vfork() helper replacing the fork() call.
This avoids triggering this error in __wait_for_test():
Test ended in some other way [127]
Cc: Christian Brauner <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Günther Noack <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Shuah Khan <[email protected]>
Cc: Will Drewry <[email protected]>
Fixes: 41cca0542d7c ("selftests/harness: Fix TEST_F()'s vfork handling")
Signed-off-by: Mickaël Salaün <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
tools/testing/selftests/kselftest_harness.h | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 55699a762c45..9f04638707ae 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -66,6 +66,8 @@
#include <sys/wait.h>
#include <unistd.h>
#include <setjmp.h>
+#include <syscall.h>
+#include <linux/sched.h>
#include "kselftest.h"
@@ -80,6 +82,17 @@
# define TH_LOG_ENABLED 1
#endif
+/* Wait for the child process to end but without sharing memory mapping. */
+static pid_t __attribute__((__unused__)) clone3_vfork(void)
+{
+ struct clone_args args = {
+ .flags = CLONE_VFORK,
+ .exit_signal = SIGCHLD,
+ };
+
+ return syscall(__NR_clone3, &args, sizeof(args));
+}
+
/**
* TH_LOG()
*
@@ -1183,7 +1196,7 @@ void __run_test(struct __fixture_metadata *f,
fflush(stdout);
fflush(stderr);
- t->pid = fork();
+ t->pid = clone3_vfork();
if (t->pid < 0) {
ksft_print_msg("ERROR SPAWNING TEST CHILD\n");
t->exit_code = KSFT_FAIL;
--
2.44.0
According to the test environment, the mount point of the test's working
directory may be shared or not, which changes the visibility of the
nested "tmp" mount point for the test's parent process calling
umount("tmp").
This was spotted while running tests in containers [1], where mount
points are private.
Cc: Günther Noack <[email protected]>
Cc: Shuah Khan <[email protected]>
Link: https://github.com/landlock-lsm/landlock-test-tools/pull/4 [1]
Fixes: 41cca0542d7c ("selftests/harness: Fix TEST_F()'s vfork handling")
Reviewed-by: Kees Cook <[email protected]>
Signed-off-by: Mickaël Salaün <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
Changes since v1:
* Update commit description.
---
tools/testing/selftests/landlock/fs_test.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 9a6036fbf289..46b9effd53e4 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -293,7 +293,15 @@ static void prepare_layout(struct __test_metadata *const _metadata)
static void cleanup_layout(struct __test_metadata *const _metadata)
{
set_cap(_metadata, CAP_SYS_ADMIN);
- EXPECT_EQ(0, umount(TMP_DIR));
+ if (umount(TMP_DIR)) {
+ /*
+ * According to the test environment, the mount point of the
+ * current directory may be shared or not, which changes the
+ * visibility of the nested TMP_DIR mount point for the test's
+ * parent process doing this cleanup.
+ */
+ ASSERT_EQ(EINVAL, errno);
+ }
clear_cap(_metadata, CAP_SYS_ADMIN);
EXPECT_EQ(0, remove_path(TMP_DIR));
}
--
2.44.0
On Mon, Apr 29, 2024 at 03:09:26PM +0200, Micka?l Sala?n wrote:
> Fix a race condition when running several FIXTURE_TEARDOWN() managing
> the same resource. This fixes a race condition in the Landlock file
> system tests when creating or unmounting the same directory.
>
> Using clone3() with CLONE_VFORK guarantees that the child and grandchild
> test processes are sequentially scheduled. This is implemented with a
> new clone3_vfork() helper replacing the fork() call.
>
> This avoids triggering this error in __wait_for_test():
> Test ended in some other way [127]
>
> Cc: Christian Brauner <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: G?nther Noack <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Shuah Khan <[email protected]>
> Cc: Will Drewry <[email protected]>
> Fixes: 41cca0542d7c ("selftests/harness: Fix TEST_F()'s vfork handling")
> Signed-off-by: Micka?l Sala?n <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> ---
> tools/testing/selftests/kselftest_harness.h | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 55699a762c45..9f04638707ae 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -66,6 +66,8 @@
> #include <sys/wait.h>
> #include <unistd.h>
> #include <setjmp.h>
> +#include <syscall.h>
> +#include <linux/sched.h>
>
> #include "kselftest.h"
>
> @@ -80,6 +82,17 @@
> # define TH_LOG_ENABLED 1
> #endif
>
> +/* Wait for the child process to end but without sharing memory mapping. */
> +static pid_t __attribute__((__unused__)) clone3_vfork(void)
Why "unused"?
> +{
> + struct clone_args args = {
> + .flags = CLONE_VFORK,
> + .exit_signal = SIGCHLD,
> + };
> +
> + return syscall(__NR_clone3, &args, sizeof(args));
> +}
> +
> /**
> * TH_LOG()
> *
> @@ -1183,7 +1196,7 @@ void __run_test(struct __fixture_metadata *f,
> fflush(stdout);
> fflush(stderr);
>
> - t->pid = fork();
> + t->pid = clone3_vfork();
> if (t->pid < 0) {
> ksft_print_msg("ERROR SPAWNING TEST CHILD\n");
> t->exit_code = KSFT_FAIL;
Regardless, yup, looks good.
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
On Mon, 29 Apr 2024 08:52:36 -0700 Kees Cook wrote:
> > +/* Wait for the child process to end but without sharing memory mapping. */
> > +static pid_t __attribute__((__unused__)) clone3_vfork(void)
>
> Why "unused"?
Right, static inline is enough
On Mon, Apr 29, 2024 at 10:16:47AM -0700, Jakub Kicinski wrote:
> On Mon, 29 Apr 2024 08:52:36 -0700 Kees Cook wrote:
> > > +/* Wait for the child process to end but without sharing memory mapping. */
> > > +static pid_t __attribute__((__unused__)) clone3_vfork(void)
> >
> > Why "unused"?
>
> Right, static inline is enough
Indeed, I'll fix that.