[PATCH v4] selftests/landlock: skip overlayfs test when kernel not support it

[Mickaël Salaün]

This looks good overall. You'll find some nitpicking review below.

I found that there is an issue with the skipped test, and especially the 
teardown parts:

> #  RUN           layout2_overlay.no_restriction ...
> #      SKIP      overlayfs is not supported
> # fs_test.c:3537:no_restriction:Expected 0 (0) == test_open(merge_fl1, O_RDONLY) (2)
> # fs_test.c:3512:no_restriction:Expected 0 (0) == umount(MERGE_DATA) (-1)

These messages seem OK…

> # fs_test.c:3496:no_restriction:Expected 0 (0) == umount(LOWER_BASE) (-1)
> # fs_test.c:3507:no_restriction:Expected 0 (0) == umount(UPPER_BASE) (-1)

…but these two should not happen because the tmpfs should still be mounted.


> #            OK  layout2_overlay.no_restriction
> ok 58 # SKIP overlayfs is not supported
> #  RUN           layout2_overlay.same_content_different_file ...
> # fs_test.c:3512:no_restriction:Expected 0 (0) == umount(MERGE_DATA) (-1)

These is some inconsistencies here too.

> # fs_test.c:230:no_restriction:Expected 0 (0) == umount(TMP_DIR) (-1)
> #      SKIP      overlayfs is not supported
> # fs_test.c:3723:same_content_different_file:Expected 0 (0) == test_open(path_entry, O_RDWR) (2)
> # fs_test.c:3496:same_content_different_file:Expected 0 (0) == umount(LOWER_BASE) (-1)
> # fs_test.c:3498:same_content_different_file:Expected 0 (0) == remove_path(LOWER_BASE) (16)
> # fs_test.c:3507:same_content_different_file:Expected 0 (0) == umount(UPPER_BASE) (-1)
> # fs_test.c:3509:same_content_different_file:Expected 0 (0) == remove_path(UPPER_BASE) (16)
> # fs_test.c:3512:same_content_different_file:Expected 0 (0) == umount(MERGE_DATA) (-1)
> # fs_test.c:230:same_content_different_file:Expected 0 (0) == umount(TMP_DIR) (-1)
> # fs_test.c:232:same_content_different_file:Expected 0 (0) == remove_path(TMP_DIR) (16)
> # fs_test.c:3512:same_content_different_file:Expected 0 (0) == umount(MERGE_DATA) (-1)

I guess this is because of the TEST_F_FORK() helper which doesn't handle 
well the skipped with error tests, hence the parent test process trying 
to execute the teardown and unmounting something which is not mounted it 
its namespace, and the duplicated messages.

This may be related to commit 63e6b2a42342 ("selftests/harness: Run 
TEARDOWN for ASSERT failures").

Can you fix TEST_F_FORK() for skipped tests please?

We should merge TEST_F_FORK() within kselftest_harness.h with a follow 
up patch though.


On 23/08/2022 03:02, jeffxu at google.com wrote:
> From: Jeff Xu <jeffxu at google.com>

This is not consistent with your Signed-off-by email.

> 
> Overlayfs can be disabled in kernel config, causing related tests to fail.
> Add check for overlayfs’s supportability at runtime, so we can call SKIP()
> when needed.

selftests/landlock: Skip overlayfs tests when not supported

overlayfs can be disabled in the kernel configuration (which is the case
for chromeOS), causing related tests to fail.  Skip such tests when an
overlayfs mount operation failed because the running kernel doesn't
support this file system.


> 
> Signed-off-by: Jeff Xu <jeffxu at chromium.org>
> Reviewed-by: Guenter Roeck <groeck at chromium.org>
> ---
>   tools/testing/selftests/landlock/fs_test.c | 53 ++++++++++++++++++++--
>   1 file changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 21a2ce8fa739..aaece185579d 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -11,6 +11,7 @@
>   #include <fcntl.h>
>   #include <linux/landlock.h>
>   #include <sched.h>
> +#include <stdio.h>
>   #include <string.h>
>   #include <sys/capability.h>
>   #include <sys/mount.h>
> @@ -62,6 +63,7 @@ static const char dir_s3d1[] = TMP_DIR "/s3d1";
>   static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2";
>   static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
>   
> +static const char proc_filesystems[] = "/proc/filesystems";

No need for this global variable, just use the string in the fopen() call.


>   /*
>    * layout1 hierarchy:
>    *
> @@ -169,6 +171,42 @@ static int remove_path(const char *const path)
>   	return err;
>   }
>   
> +static bool fgrep(FILE *inf, const char *str)

const char *const str

s/inf/file/


> +{
> +	char line[32];
> +	int slen = strlen(str);

s/slen/str_len/

> +
> +	while (!feof(inf)) {
> +		if (!fgets(line, sizeof(line), inf))
> +			break;
> +		if (strncmp(line, str, slen))
> +			continue;
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool supports_overlayfs(void)
> +{
> +	bool ret = false;

No need to set it to "false" (which should be "true" BTW).


> +	FILE *inf = fopen(proc_filesystems, "r");

s/inf/filename/

> +
> +	/*
> +	 * If fopen fails, return supported.
> +	 * This helps to detect missing file (shall not
> +	 * happen).

"A failed attempt to open /proc/filesystems implies that the file
system is supported (default behavior). This can help detect such
unattended issue (which should not happen)."


> +	 */
> +	if (!inf)
> +		return true;
> +
> +	ret = fgrep(inf, "nodev\toverlay\n");
> +	fclose(inf);
> +

You can remove this newline.

> +	return ret;
> +}
> +
>   static void prepare_layout(struct __test_metadata *const _metadata)
>   {
>   	disable_caps(_metadata);
> @@ -3404,6 +3442,8 @@ FIXTURE(layout2_overlay) {};
>   
>   FIXTURE_SETUP(layout2_overlay)
>   {
> +	int rc;

s/rc/ret/

int ret, err;

> +
>   	prepare_layout(_metadata);
>   
>   	create_directory(_metadata, LOWER_BASE);
> @@ -3431,11 +3471,18 @@ FIXTURE_SETUP(layout2_overlay)
>   	create_directory(_metadata, MERGE_DATA);
>   	set_cap(_metadata, CAP_SYS_ADMIN);
>   	set_cap(_metadata, CAP_DAC_OVERRIDE);
> -	ASSERT_EQ(0, mount("overlay", MERGE_DATA, "overlay", 0,
> -			   "lowerdir=" LOWER_DATA ",upperdir=" UPPER_DATA
> -			   ",workdir=" UPPER_WORK));
> +
> +	rc = mount("overlay", MERGE_DATA, "overlay", 0,
> +		   "lowerdir=" LOWER_DATA ",upperdir=" UPPER_DATA
> +		   ",workdir=" UPPER_WORK);

err = errno;

>   	clear_cap(_metadata, CAP_DAC_OVERRIDE);
>   	clear_cap(_metadata, CAP_SYS_ADMIN);
> +
> +	if (rc < 0) {

if (ret == -1) {

> +		ASSERT_EQ(ENODEV, errno);

ASSERT_EQ(ENODEV, err);

> +		ASSERT_FALSE(supports_overlayfs());
> +		SKIP(return, "overlayfs is not supported");
> +	}

ASSERT_EQ(0, ret);

>   }
>   
>   FIXTURE_TEARDOWN(layout2_overlay)
> 
> base-commit: 50cd95ac46548429e5bba7ca75cc97d11a697947