Re: [PATCH 2/2] landlock: Selftests for truncate(2) support.

["Mickaël Salaün" <mic@digikod.net>]

Please use "selftests/landlock:" as subject prefix and without a final dot.


On 07/07/2022 22:06, Günther Noack wrote:
> These tests exercise the following scenarios:
> 
> * File with Read, Write, Truncate rights.

Should we use a capital for access right names or does it come from Go? ;)


> * File with Read, Write rights.
> * File with Truncate rights.
> * File with no rights.
> * Directory with Truncate rights.
> 
> For each of the scenarios, both truncate() and the open() +
> ftruncate() syscalls get exercised and their results checked.
> 
> In particular, the test demonstrates that opening a file for writing
> is not enough to call truncate().

Looks good! According to my previous comment, O_TRUNC should be tested 
if it is checked by the kernel.


> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   tools/testing/selftests/landlock/fs_test.c | 80 ++++++++++++++++++++++
>   1 file changed, 80 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index cb77eaa01c91..c3e48fd12b2b 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -2237,6 +2237,86 @@ TEST_F_FORK(layout1, reparent_rename)
>   	ASSERT_EQ(EXDEV, errno);
>   }
>   
> +TEST_F_FORK(layout1, truncate)

Please move this test after the proc_pipe one.


> +{
> +	const struct rule rules[] = {

You can add a first layer of rules to check truncate and ftruncate with 
a ruleset not handling LANDLOCK_ACCESS_FS_TRUNCATE.


> +		{
> +			.path = file1_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE |
> +				  LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		{
> +			.path = file2_s1d2,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = file1_s1d2,
> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},

Move this entry before file2_s1d2 to keep the paths sorted and make this 
easier to read. You can change the access rights per path to also keep 
their ordering though.


> +		{
> +			.path = dir_s2d3,
> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		// Implicitly: No access rights for file2_s1d1.

Comment to move after the use of file1_s1d1.

> +		{},
> +	};
> +	const int ruleset_fd = create_ruleset(_metadata, ACCESS_ALL, rules);

Don't use ACCESS_ALL because it will change over time and we want tests 
to be deterministic. You can use rules[0].access instead.


> +	int reg_fd;
> +
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Read, write and truncate permissions => truncate and ftruncate work. */

It would be nice to have consistent comments such as: "Checks read, 
write and truncate access rights: truncate and ftruncate work."


> +	reg_fd = open(file1_s1d1, O_RDWR | O_CLOEXEC);
> +	ASSERT_LE(0, reg_fd);
> +	EXPECT_EQ(0, ftruncate(reg_fd, 10));

You should not use EXPECT but ASSERT here. I use EXPECT when an error 
could block a test or when it could stop a cleanup (i.e. teardown).


> +	EXPECT_EQ(0, ftruncate64(reg_fd, 20));
> +	ASSERT_EQ(0, close(reg_fd));
> +
> +	EXPECT_EQ(0, truncate(file1_s1d1, 10));
> +	EXPECT_EQ(0, truncate64(file1_s1d1, 20));
> +
> +	/* Just read and write permissions => no truncate variant works. */
> +	reg_fd = open(file2_s1d2, O_RDWR | O_CLOEXEC);
> +	ASSERT_LE(0, reg_fd);
> +	EXPECT_EQ(-1, ftruncate(reg_fd, 10));
> +	EXPECT_EQ(EACCES, errno);
> +	EXPECT_EQ(-1, ftruncate64(reg_fd, 20));
> +	EXPECT_EQ(EACCES, errno);
> +	ASSERT_EQ(0, close(reg_fd));
> +
> +	EXPECT_EQ(-1, truncate(file2_s1d2, 10));
> +	EXPECT_EQ(EACCES, errno);
> +	EXPECT_EQ(-1, truncate64(file2_s1d2, 20));
> +	EXPECT_EQ(EACCES, errno);
> +
> +	/* Just truncate permissions => truncate(64) works, but can't open file. */
> +	ASSERT_EQ(-1, open(file1_s1d2, O_RDWR | O_CLOEXEC));
> +	ASSERT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(0, truncate(file1_s1d2, 10));
> +	EXPECT_EQ(0, truncate64(file1_s1d2, 20));
> +
> +	/* Just truncate permission on directory => truncate(64) works, but can't open file. */
> +	ASSERT_EQ(-1, open(file1_s2d3, O_RDWR | O_CLOEXEC));
> +	ASSERT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(0, truncate(file1_s2d3, 10));
> +	EXPECT_EQ(0, truncate64(file1_s2d3, 20));
> +
> +	/* No permissions => Neither truncate nor ftruncate work. */
> +	ASSERT_EQ(-1, open(file2_s1d1, O_RDWR | O_CLOEXEC));
> +	ASSERT_EQ(EACCES, errno);
> +
> +	EXPECT_EQ(-1, truncate(file2_s1d1, 10));
> +	EXPECT_EQ(EACCES, errno);
> +	EXPECT_EQ(-1, truncate64(file2_s1d1, 20));
> +	EXPECT_EQ(EACCES, errno);

These tests are good!

> +}
> +
>   static void
>   reparent_exdev_layers_enforce1(struct __test_metadata *const _metadata)
>   {