* [PATCH v4] selftests/landlock: skip overlayfs test when kernel not support it
@ 2022-08-23 1:02 jeffxu
2022-08-23 15:28 ` Mickaël Salaün
0 siblings, 1 reply; 4+ messages in thread
From: jeffxu @ 2022-08-23 1:02 UTC (permalink / raw)
To: mic; +Cc: jorgelo, keescook, linux-security-module, groeck, Jeff Xu,
Jeff Xu
From: Jeff Xu <jeffxu@google.com>
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.
Signed-off-by: Jeff Xu <jeffxu@chromium.org>
Reviewed-by: Guenter Roeck <groeck@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";
/*
* layout1 hierarchy:
*
@@ -169,6 +171,42 @@ static int remove_path(const char *const path)
return err;
}
+static bool fgrep(FILE *inf, const char *str)
+{
+ char line[32];
+ int slen = strlen(str);
+
+ 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;
+ FILE *inf = fopen(proc_filesystems, "r");
+
+ /*
+ * If fopen fails, return supported.
+ * This helps to detect missing file (shall not
+ * happen).
+ */
+ if (!inf)
+ return true;
+
+ ret = fgrep(inf, "nodev\toverlay\n");
+ fclose(inf);
+
+ 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;
+
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);
clear_cap(_metadata, CAP_DAC_OVERRIDE);
clear_cap(_metadata, CAP_SYS_ADMIN);
+
+ if (rc < 0) {
+ ASSERT_EQ(ENODEV, errno);
+ ASSERT_FALSE(supports_overlayfs());
+ SKIP(return, "overlayfs is not supported");
+ }
}
FIXTURE_TEARDOWN(layout2_overlay)
base-commit: 50cd95ac46548429e5bba7ca75cc97d11a697947
--
2.37.1.595.g718a3a8f04-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v4] selftests/landlock: skip overlayfs test when kernel not support it
2022-08-23 1:02 [PATCH v4] selftests/landlock: skip overlayfs test when kernel not support it jeffxu
@ 2022-08-23 15:28 ` Mickaël Salaün
2022-08-23 23:47 ` Jeff Xu
0 siblings, 1 reply; 4+ messages in thread
From: Mickaël Salaün @ 2022-08-23 15:28 UTC (permalink / raw)
To: jeffxu; +Cc: jorgelo, keescook, linux-security-module, groeck, Jeff Xu,
Kees Cook
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@google.com wrote:
> From: Jeff Xu <jeffxu@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@chromium.org>
> Reviewed-by: Guenter Roeck <groeck@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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v4] selftests/landlock: skip overlayfs test when kernel not support it
2022-08-23 15:28 ` Mickaël Salaün
@ 2022-08-23 23:47 ` Jeff Xu
2022-08-24 9:08 ` Mickaël Salaün
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Xu @ 2022-08-23 23:47 UTC (permalink / raw)
To: Mickaël Salaün
Cc: jorgelo, linux-security-module, groeck, Jeff Xu, Kees Cook
> 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.
OK. That can be after I worked on pt_test.
Would you like me to move supports_overlayfs() to the beginning of
layout2_overlay in this Patch ?
then there is nothing to cleanup.
> On 23/08/2022 03:02, jeffxu@google.com wrote:
> > From: Jeff Xu <jeffxu@google.com>
>
> This is not consistent with your Signed-off-by email.
Sure, will try to switch to send patch via jeffxu@chromium.org
On Tue, Aug 23, 2022 at 8:28 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> 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@google.com wrote:
> > From: Jeff Xu <jeffxu@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@chromium.org>
> > Reviewed-by: Guenter Roeck <groeck@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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v4] selftests/landlock: skip overlayfs test when kernel not support it
2022-08-23 23:47 ` Jeff Xu
@ 2022-08-24 9:08 ` Mickaël Salaün
0 siblings, 0 replies; 4+ messages in thread
From: Mickaël Salaün @ 2022-08-24 9:08 UTC (permalink / raw)
To: Jeff Xu; +Cc: jorgelo, linux-security-module, groeck, Jeff Xu, Kees Cook
On 24/08/2022 01:47, Jeff Xu wrote:
>> 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.
>
> OK. That can be after I worked on pt_test.
Right, but that is independent from the TEST_F_FORK() fix which should
be easier to do and make backport possible.
>
> Would you like me to move supports_overlayfs() to the beginning of
> layout2_overlay in this Patch ?
> then there is nothing to cleanup.
I'm not sure this would be enough, and the current approach is good.
TEST_F_FORK() needs some investigation.
>
>> On 23/08/2022 03:02, jeffxu@google.com wrote:
>>> From: Jeff Xu <jeffxu@google.com>
>>
>> This is not consistent with your Signed-off-by email.
>
> Sure, will try to switch to send patch via jeffxu@chromium.org
>
>
> On Tue, Aug 23, 2022 at 8:28 AM Mickaël Salaün <mic@digikod.net> wrote:
>>
>> 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@google.com wrote:
>>> From: Jeff Xu <jeffxu@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@chromium.org>
>>> Reviewed-by: Guenter Roeck <groeck@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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-24 9:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-23 1:02 [PATCH v4] selftests/landlock: skip overlayfs test when kernel not support it jeffxu
2022-08-23 15:28 ` Mickaël Salaün
2022-08-23 23:47 ` Jeff Xu
2022-08-24 9:08 ` Mickaël Salaün
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).