Re: [PATCH v2 -next 16/18] selftests/landlock: Add tests for metadata access rights
flat view
From: sashiko-bot@kernel.org
Date: 2026-10-15 12:46:32
Also in:
bpf, linux-trace-kernel, selinux
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] selftests/landlock: test logic and commit message contradiction in fileattr_write_only - [Medium] selftests: hardcoded syscall fallbacks 468/469 are incorrect on Alpha -- commit 14e6c962c9835f4d2f9d99bb2bfea0e277c3117a Author: Cai Xinchen [off-list ref] selftests/landlock: Add tests for metadata access rights This commit adds layout1 and layout2 tests for the newly introduced LANDLOCK_ACCESS_FS_READ_METADATA and LANDLOCK_ACCESS_FS_WRITE_METADATA access rights. It verifies metadata restriction behaviors for various system calls and fileattr interfaces, including overlayfs scenarios.
quoted hunk ↗ jump to hunk
--- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c
[ ... ]
quoted hunk ↗ jump to hunk
+#ifndef __NR_file_getattr +#define __NR_file_getattr 468 +#endif +#ifndef __NR_file_setattr +#define __NR_file_setattr 469 +#endif
[Severity: Medium] Are these hardcoded syscall fallbacks correct for all architectures? The assumption that syscalls post-424 are unified across all architectures is incorrect for Alpha, which uses 578 and 579 for these syscalls. If the selftests are built on an Alpha platform with older host headers, it will invoke incorrect system calls (such as splice and sync_file_range), leading to failures or undefined behavior. [ ... ]
quoted hunk ↗ jump to hunk
+TEST_F_FORK(layout1, fileattr_write_only) +{ + const struct rule rules[] = { + { + .path = file1_s1d1, + .access = LANDLOCK_ACCESS_FS_WRITE_METADATA, + }, + {}, + }; + unsigned int flags; + int fd, ruleset_fd; + + ruleset_fd = create_ruleset(_metadata, + LANDLOCK_ACCESS_FS_WRITE_METADATA, rules); + ASSERT_LE(0, ruleset_fd); + + fd = open(file1_s1d1, O_RDONLY | O_CLOEXEC); + ASSERT_LE(0, fd); + + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + /* + * WRITE_METADATA alone must be enough to set flags: the kernel-side + * read of the old flags must not require READ_METADATA. + */ + EXPECT_EQ(0, test_setflags(fd, FS_NODUMP_FL)); + flags = 0; + /* READ_METADATA is not handled: reading is allowed. */ + EXPECT_EQ(0, test_getflags(fd, &flags));
[Severity: Medium] Does this test logic contradict the commit message? The commit message states that for fileattr_write_only: "metadata reads (ioctl(FS_IOC_GETFLAGS)) are still denied". However, the test explicitly creates a ruleset that only handles LANDLOCK_ACCESS_FS_WRITE_METADATA. Because LANDLOCK_ACCESS_FS_READ_METADATA is unhandled, reading metadata is globally permitted, and the test expects test_getflags() to succeed. Additionally, by leaving READ_METADATA unhandled, does this test fail to effectively verify that FS_IOC_SETFLAGS (which internally reads metadata) works when READ_METADATA is actively restricted?
quoted hunk ↗ jump to hunk
+ EXPECT_EQ(FS_NODUMP_FL, flags & FS_NODUMP_FL); + EXPECT_EQ(0, test_setflags(fd, 0)); + + ASSERT_EQ(0, close(fd)); +}
-- Sashiko AI review · https://sashiko.dev/#/patchset/20261015125425.3246251-1-caixinchen1@huawei.com?part=16