[PATCH v4] selftests/landlock: skip overlayfs test when kernel not support it
Mickaël Salaün
mic at digikod.net
Wed Aug 24 09:08:57 UTC 2022
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 at google.com wrote:
>>> From: Jeff Xu <jeffxu at google.com>
>>
>> This is not consistent with your Signed-off-by email.
>
> Sure, will try to switch to send patch via jeffxu at chromium.org
>
>
> On Tue, Aug 23, 2022 at 8:28 AM Mickaël Salaün <mic at 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 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
More information about the Linux-security-module-archive
mailing list