From 7132fe5077ee8e0138d61068188f6d5828433e3f Mon Sep 17 00:00:00 2001 From: Tingmao Wang Date: Sat, 24 May 2025 16:37:54 +0100 Subject: [PATCH 1/3] selftests/landlock: Clean up tmp directory even when mount fails A typical sequence for someone running this test for the first time might be: make kselftest TARGETS="landlock" (sees a bunch of "Permission denied", realizes that sudo is needed) sudo make kselftest TARGETS="landlock" (sees a bunch of "File exists", scratches head) This ensures that the newly created directory is cleaned up by the first attempt (and also gives a slightly more helpful message explaining the cause). See proposal in patch 3 message for a more generic solution - this might not be necessary. Signed-off-by: Tingmao Wang --- tools/testing/selftests/landlock/fs_test.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 73729382d40f82..e65e6cc80e227a 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -5,6 +5,7 @@ * Copyright © 2017-2020 Mickaël Salaün * Copyright © 2020 ANSSI * Copyright © 2020-2022 Microsoft Corporation + * Copyright © 2025 Tingmao Wang */ #define _GNU_SOURCE @@ -303,10 +304,9 @@ static void prepare_layout_opt(struct __test_metadata *const _metadata, * for tests relying on pivot_root(2) and move_mount(2). */ set_cap(_metadata, CAP_SYS_ADMIN); - ASSERT_EQ(0, unshare(CLONE_NEWNS | CLONE_NEWCGROUP)); - ASSERT_EQ(0, mount_opt(mnt, TMP_DIR)) + ASSERT_EQ(0, unshare(CLONE_NEWNS | CLONE_NEWCGROUP)) { - TH_LOG("Failed to mount the %s filesystem: %s", mnt->type, + TH_LOG("Failed to create new mount namespace: %s", strerror(errno)); /* * FIXTURE_TEARDOWN() is not called when FIXTURE_SETUP() @@ -316,6 +316,12 @@ static void prepare_layout_opt(struct __test_metadata *const _metadata, */ remove_path(TMP_DIR); } + ASSERT_EQ(0, mount_opt(mnt, TMP_DIR)) + { + TH_LOG("Failed to mount the %s filesystem: %s", mnt->type, + strerror(errno)); + remove_path(TMP_DIR); + } ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC, NULL)); clear_cap(_metadata, CAP_SYS_ADMIN); } From 2669038842c2300e6e0b9fcb68059a502c74092a Mon Sep 17 00:00:00 2001 From: Tingmao Wang Date: Sat, 24 May 2025 17:32:27 +0100 Subject: [PATCH 2/3] selftests/landlock: Print a warning about directory permissions Because we drop capabilities (most importantly, CAP_DAC_OVERRIDE), if a user runs the selftests under a Linux source checked out by a non-root user, the test will fail even when ran under sudo, and will print a "Permission denied" error. This creates a confusing situation if they does not realize that the test drops capabilities, and can mislead users to think there's something wrong with the test or landlock. This patch produces output that looks like: # # RUN layout0.ruleset_with_unknown_access ... # # fs_test.c:240:ruleset_with_unknown_access:Expected 0 (0) == mkdir(path, 0700) (-1) # # fs_test.c:244:ruleset_with_unknown_access:Failed to create directory "tmp": Permission denied # # fs_test.c:230:ruleset_with_unknown_access:Hint: fs_tests requires permissions for uid 0 on test directory /home/mao/landlock-selftests/tools/testing/selftests/landlock and files under it (even when running as root). # # fs_test.c:232:ruleset_with_unknown_access: Try chmod a+rwX -R /home/mao/landlock-selftests/tools/testing/selftests/landlock # # ruleset_with_unknown_access: Test terminated by assertion # # FAIL layout0.ruleset_with_unknown_access Signed-off-by: Tingmao Wang --- tools/testing/selftests/landlock/fs_test.c | 36 +++++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index e65e6cc80e227a..328816b08e39c3 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -216,14 +216,38 @@ static void mkdir_parents(struct __test_metadata *const _metadata, free(walker); } +static void +maybe_warn_about_permission_on_cwd(struct __test_metadata *const _metadata, + int err) +{ + char abspath_buf[255]; + + if (err == EACCES) { + const char *realp = realpath(".", abspath_buf); + + if (realp == NULL) + realp = "."; + + TH_LOG("Hint: fs_tests requires permissions for uid %u on test directory %s " + "and files under it (even when running as root).", + getuid(), realp); + TH_LOG(" Try chmod a+rwX -R %s", realp); + } +} + static void create_directory(struct __test_metadata *const _metadata, const char *const path) { mkdir_parents(_metadata, path); ASSERT_EQ(0, mkdir(path, 0700)) { + int err = errno; + TH_LOG("Failed to create directory \"%s\": %s", path, - strerror(errno)); + strerror(err)); + + if (strcmp(path, TMP_DIR) == 0) + maybe_warn_about_permission_on_cwd(_metadata, err); } } @@ -1985,18 +2009,22 @@ TEST_F_FORK(layout1, relative_chroot_chdir) static void copy_file(struct __test_metadata *const _metadata, const char *const src_path, const char *const dst_path) { - int dst_fd, src_fd; + int dst_fd, src_fd, err; struct stat statbuf; dst_fd = open(dst_path, O_WRONLY | O_TRUNC | O_CLOEXEC); ASSERT_LE(0, dst_fd) { - TH_LOG("Failed to open \"%s\": %s", dst_path, strerror(errno)); + err = errno; + TH_LOG("Failed to open \"%s\": %s", dst_path, strerror(err)); + maybe_warn_about_permission_on_cwd(_metadata, err); } src_fd = open(src_path, O_RDONLY | O_CLOEXEC); ASSERT_LE(0, src_fd) { - TH_LOG("Failed to open \"%s\": %s", src_path, strerror(errno)); + err = errno; + TH_LOG("Failed to open \"%s\": %s", src_path, strerror(err)); + maybe_warn_about_permission_on_cwd(_metadata, err); } ASSERT_EQ(0, fstat(src_fd, &statbuf)); ASSERT_EQ(statbuf.st_size, From e8b75613e54ae94673b597ff4aac6857fb96c705 Mon Sep 17 00:00:00 2001 From: Tingmao Wang Date: Sat, 24 May 2025 18:02:57 +0100 Subject: [PATCH 3/3] selftests/landlock: Clean up TMP_DIR and retry if dir already exists This ensures that if for whatever reason FIXTURE_SETUP fails, the next test run will handle this gracefully. I don't actually 100% like this approach - maybe we should consider enhancing the test framework to add a FIXTURE_TEARDOWN_ALWAYS, that will run even if FIXTURE_SETUP fails? Signed-off-by: Tingmao Wang --- tools/testing/selftests/landlock/fs_test.c | 46 ++++++++++++++++++++-- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 328816b08e39c3..eab7b4dba44ce3 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -235,19 +235,55 @@ maybe_warn_about_permission_on_cwd(struct __test_metadata *const _metadata, } } +static int try_teardown_layout(struct __test_metadata *const _metadata) +{ + struct stat stat_buf; + + if (stat(TMP_DIR, &stat_buf) < 0) + return -1; + + TH_LOG("Attempting to cleanup layout and retry..."); + + if (umount(TMP_DIR)) { + if (errno != EINVAL && errno != ENOENT) { + TH_LOG("Failed to unmount directory \"%s\": %s", + TMP_DIR, strerror(errno)); + return -1; + } + } + if (rmdir(TMP_DIR)) { + if (errno != ENOENT) { + TH_LOG("Failed to remove directory \"%s\": %s", TMP_DIR, + strerror(errno)); + return -1; + } + } + return 0; +} + static void create_directory(struct __test_metadata *const _metadata, const char *const path) { + bool retried = false; + +retry: mkdir_parents(_metadata, path); - ASSERT_EQ(0, mkdir(path, 0700)) - { + if (mkdir(path, 0700)) { int err = errno; TH_LOG("Failed to create directory \"%s\": %s", path, strerror(err)); - if (strcmp(path, TMP_DIR) == 0) + if (strcmp(path, TMP_DIR) == 0) { maybe_warn_about_permission_on_cwd(_metadata, err); + if (!retried && errno == EEXIST && + !try_teardown_layout(_metadata)) { + retried = true; + goto retry; + } + } + + ASSERT_TRUE(false); } } @@ -321,13 +357,15 @@ static void prepare_layout_opt(struct __test_metadata *const _metadata, { disable_caps(_metadata); umask(0077); + + /* create_directory may try umounting then rmdir if tmp already mounted */ + set_cap(_metadata, CAP_SYS_ADMIN); create_directory(_metadata, TMP_DIR); /* * Do not pollute the rest of the system: creates a private mount point * for tests relying on pivot_root(2) and move_mount(2). */ - set_cap(_metadata, CAP_SYS_ADMIN); ASSERT_EQ(0, unshare(CLONE_NEWNS | CLONE_NEWCGROUP)) { TH_LOG("Failed to create new mount namespace: %s",