Re: [PATCH v7 1/1] selftests/landlock: skip overlayfs test when kernel not support it

[Jeff Xu <jeffxu@google.com>]

Hi Mickaël
Please see inline.

On Mon, Jan 9, 2023 at 8:05 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> Please refresh with clang-format-14.
>
My installation has clang-format version 15, but changes are quite big
if I use it,
do you still want me to use it ?

> You might want to update the subject to:
> selftests/landlock: Skip overlayfs tests when not supported
>
OK.

>
> On 29/12/2022 22:41, Guenter Roeck wrote:
> > On Thu, Dec 29, 2022 at 1:02 PM <jeffxu@chromium.org> wrote:
> >>
> >> 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@google.com>
> >
> > Reviewed-by: Guenter Roeck <groeck@chromium.org>
> >
> >> ---
> >>   tools/testing/selftests/landlock/fs_test.c | 51 ++++++++++++++++++++++
> >>   1 file changed, 51 insertions(+)
> >>
> >> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> >> index 21a2ce8fa739..34095fe2419b 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";
>
> You can inline this string in the fopen() call for now.
>
Done.

>
> >>   /*
> >>    * layout1 hierarchy:
> >>    *
> >> @@ -169,6 +171,43 @@ 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)
>
> You can move this two functions just before mkdir_parents().
>
Done.

>
> >> +{
> >> +       bool res;
> >> +       FILE *inf = fopen(proc_filesystems, "r");
> >> +
> >> +       /*
> >> +        * If fopen failed, return supported.
> >> +        * This help detect missing file (shall not
> >> +        * happen).
>
> You can make this comment fit in two lines, with 80 columns.
>
Done.

> >> +        */
> >> +       if (!inf)
> >> +               return true;
> >> +
> >> +       res = fgrep(inf, "nodev\toverlay\n");
> >> +       fclose(inf);
> >> +
> >> +       return res;
> >> +}
> >> +
> >> +
> >>   static void prepare_layout(struct __test_metadata *const _metadata)
> >>   {
> >>          disable_caps(_metadata);
> >> @@ -3404,6 +3443,9 @@ FIXTURE(layout2_overlay) {};
> >>
> >>   FIXTURE_SETUP(layout2_overlay)
> >>   {
> >> +       if (!supports_overlayfs())
> >> +               SKIP(return, "overlayfs is not supported");
> >> +
> >>          prepare_layout(_metadata);
> >>
> >>          create_directory(_metadata, LOWER_BASE);
> >> @@ -3440,6 +3482,9 @@ FIXTURE_SETUP(layout2_overlay)
> >>
> >>   FIXTURE_TEARDOWN(layout2_overlay)
> >>   {
> >> +       if (!supports_overlayfs())
> >> +               SKIP(return, "overlayfs is not supported");
>
> This looks good to me except the multiple supports_overlayfs() calls.
> Only the FIXTURE_SETUP() should be required. I guess some modifications
> of kselftest_harness.h are need to support that. I'd like to avoid
> touching TEST_F_FORK() which should be part of kselftest_harness.h
>
>
In kselftest_harness.h,  SKIP() only applies within the function scope (
FIXTURE_SETUP(), TEST_F_FORK(), FIXTURE_TEARDOWN())

If we want to apply the skip logic to all remaining steps of the testcase,
I think we should do it with a dedicated environment check hook
(FIXTURE_ENV_CHECK),
called before FIXTURE_SETUP(), if the environment check fails, all of
the remaining
test steps will be skipped. In this way, once the env check pass,
the remaining test case should also be passing, or if env check fails,
there is no need to
delete the resource since no setup is called.

However,  this requires change to the kselftest_harness.h, I do think it needs
to be a separate feature and commit (we can adopt fs_test to be the
first user)

Best regards,
Jeff

> >> +
> >>          EXPECT_EQ(0, remove_path(lower_do1_fl3));
> >>          EXPECT_EQ(0, remove_path(lower_dl1_fl2));
> >>          EXPECT_EQ(0, remove_path(lower_fl1));
> >> @@ -3471,6 +3516,9 @@ FIXTURE_TEARDOWN(layout2_overlay)
> >>
> >>   TEST_F_FORK(layout2_overlay, no_restriction)
> >>   {
> >> +       if (!supports_overlayfs())
> >> +               SKIP(return, "overlayfs is not supported");
> >> +
> >>          ASSERT_EQ(0, test_open(lower_fl1, O_RDONLY));
> >>          ASSERT_EQ(0, test_open(lower_dl1, O_RDONLY));
> >>          ASSERT_EQ(0, test_open(lower_dl1_fl2, O_RDONLY));
> >> @@ -3634,6 +3682,9 @@ TEST_F_FORK(layout2_overlay, same_content_different_file)
> >>          size_t i;
> >>          const char *path_entry;
> >>
> >> +       if (!supports_overlayfs())
> >> +               SKIP(return, "overlayfs is not supported");
> >> +
> >>          /* Sets rules on base directories (i.e. outside overlay scope). */
> >>          ruleset_fd = create_ruleset(_metadata, ACCESS_RW, layer1_base);
> >>          ASSERT_LE(0, ruleset_fd);
> >> --
> >> 2.39.0.314.g84b9a713c41-goog
> >>