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

[Mickaël Salaün]

On 09/01/2023 22:59, Jeff Xu wrote:
> Hi Mickaël
> Please see inline.
> 
> On Mon, Jan 9, 2023 at 8:05 AM Mickaël Salaün <mic at 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 ?

That's fine, this patch is almost good, I'll run clang-format-14 on the 
final one.

> 
>> 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 at chromium.org> wrote:
>>>>
>>>> From: Jeff Xu <jeffxu at 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 at google.com>
>>>
>>> Reviewed-by: Guenter Roeck <groeck at 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)

Looks good to me, implementing this FIXTURE_ENV_CHECK (or something 
similar) will be cleaner.

Shuah, what do you think about that?


> 
> 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
>>>>