Re: [PATCH v2 -next 16/18] selftests/landlock: Add tests for metadata access rights

[sashiko-bot@kernel.org]

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 <caixinchen1@huawei.com>

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.

> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c

[ ... ]

> +#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.

[ ... ]

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

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