[PATCH 0/3] selftests/landlock: UX improvements

[PATCH 0/3] selftests/landlock: UX improvements

[Tingmao Wang]

Hi,

A while ago I tried running the landlock selftests and it took me a while
to get it completely working.  One of the main thing which confused me a
bit was the fact that I was missing the right permissions on the test
directory, and this matters even though I ran the test with root.

The other slightly annoying thing was that the mkdir would fail with
EEXIST if a previous run fails to setup.

This patch series tries to improve the experience should someone else runs
into the same issues.

Tingmao Wang (3):
  selftests/landlock: Clean up tmp directory even when mount fails
  selftests/landlock: Print a warning about directory permissions
  selftests/landlock: Clean up TMP_DIR and retry if dir already exists

 tools/testing/selftests/landlock/fs_test.c | 91 +++++++++++++++++++---
 1 file changed, 81 insertions(+), 10 deletions(-)


base-commit: a5806cd506af5a7c19bcd596e4708b5c464bfd21
--
2.49.0

[PATCH 1/3] selftests/landlock: Clean up tmp directory even when mount fails

[Tingmao Wang]

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 <m@maowtm.org>
---
 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 73729382d40f..e65e6cc80e22 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 <mic@digikod.net>
  * Copyright © 2020 ANSSI
  * Copyright © 2020-2022 Microsoft Corporation
+ * Copyright © 2025 Tingmao Wang <m@maowtm.org>
  */
 
 #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);
 }
-- 
2.49.0

[PATCH 2/3] selftests/landlock: Print a warning about directory permissions

[Tingmao Wang]

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 <m@maowtm.org>
---
 tools/testing/selftests/landlock/fs_test.c | 35 +++++++++++++++++++---
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index e65e6cc80e22..21ed8afcc060 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -216,14 +216,37 @@ 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 +2008,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,
-- 
2.49.0

[PATCH 3/3] selftests/landlock: Clean up TMP_DIR and retry if dir already exists

[Tingmao Wang]

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 <m@maowtm.org>
---
 tools/testing/selftests/landlock/fs_test.c | 44 ++++++++++++++++++++--
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 21ed8afcc060..e6891f59803a 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -233,12 +233,41 @@ 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,
@@ -246,7 +275,14 @@ static void create_directory(struct __test_metadata *const _metadata,
 
 		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);
 	}
 }
 
@@ -320,13 +356,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",
-- 
2.49.0

Re: [PATCH 2/3] selftests/landlock: Print a warning about directory permissions

[Tingmao Wang]

On 5/24/25 18:56, Tingmao Wang wrote:
> [...]
> +		TH_LOG("Hint: fs_tests requires permissions for uid %u on test directory %s and files under it (even when running as root).",
> [...]
> +		if (strcmp(path, TMP_DIR) == 0) {
> +			maybe_warn_about_permission_on_cwd(_metadata, err);
> +		}
I immediately realized that I forgot to run checkpatch.pl after sending 
the patch - fixed in my own branch [1] and the next version will have 
the fix for these style problems.

[1]: https://github.com/micromaomao/linux-dev/pull/7

Re: [PATCH 2/3] selftests/landlock: Print a warning about directory permissions

[Tingmao Wang]

On 5/24/25 18:56, Tingmao Wang wrote:
> 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 <m@maowtm.org>
> ---
>   tools/testing/selftests/landlock/fs_test.c | 35 +++++++++++++++++++---
>   1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index e65e6cc80e22..21ed8afcc060 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -216,14 +216,37 @@ 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);

Actually, just having rwx on the test directory itself is not enough. 
For audit tests, in order to set the executable itself as AUDIT_EXE, we 
pass in an absolute path (which is required), which then means that we 
need path walk permission from root to the executable (otherwise 
audit_alloc_mark -> kern_path_locked fails), so in fact if the user has 
a setup where the home directory, containing the Linux source code, is 
not world-readable (or owned by root), fs_test::audit_layout1 etc will 
fail too...

I wonder if we should in fact drop capabilities only after fixture 
setup?  Alternatively we should have an appropriate message explaining 
that the test dir needs to be walkable and writable by root without 
CAP_DAC_OVERRIDE.