Re: [PATCH v6 3/5] selftests/landlock: Selftests for file truncation support

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

On 23/09/2022 19:50, Günther Noack wrote:
> On Fri, Sep 16, 2022 at 07:05:44PM +0200, Mickaël Salaün wrote:
>> I'd like to have tests similar to base_test.c:ruleset_fd_transfer to check
>> ftruncate with different kind of file descriptors and not-sandboxed
>> processes. That would require some code refactoring to reuse the FD passing
>> code.
> 
> Done. I factored out the FD sending and receiving into helper function in common.h.

Please use a dedicated patch for this refactoring.

> 
>> On 08/09/2022 21:58, Günther Noack wrote:
>>> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
>>> index 87b28d14a1aa..ddc8c7e57e86 100644
>>> --- a/tools/testing/selftests/landlock/fs_test.c
>>> +++ b/tools/testing/selftests/landlock/fs_test.c
>>> ...
>>> +TEST_F_FORK(layout1, truncate)
>>> +{
>>> +	const char *const file_rwt = file1_s1d1;
>>> +	const char *const file_rw = file2_s1d1;
>>> +	const char *const file_rt = file1_s1d2;
>>> +	const char *const file_t = file2_s1d2;
>>> +	const char *const file_none = file1_s1d3;
>>> +	const char *const dir_t = dir_s2d1;
>>> +	const char *const file_in_dir_t = file1_s2d1;
>>> +	const char *const dir_w = dir_s3d1;
>>> +	const char *const file_in_dir_w = file1_s3d1;
>>> +	int file_rwt_fd, file_rw_fd;
>>
>> These variables are unused now.
> 
> Good catch, done.
> 
>>> +TEST_F_FORK(layout1, ftruncate)
>>
>> Great!
>>
>>> +{
>>> +	/*
>>> +	 * This test opens a new file descriptor at different stages of
>>> +	 * Landlock restriction:
>>> +	 *
>>> +	 * without restriction:                    ftruncate works
>>> +	 * something else but truncate restricted: ftruncate works
>>> +	 * truncate restricted and permitted:      ftruncate works
>>> +	 * truncate restricted and not permitted:  ftruncate fails
>>> +	 *
>>> +	 * Whether this works or not is expected to depend on the time when the
>>> +	 * FD was opened, not to depend on the time when ftruncate() was
>>> +	 * called.
>>> +	 */
>>> +	const char *const path = file1_s1d1;
>>> +	int fd0, fd1, fd2, fd3;
>>
>> You can rename them fd_layer0, fd_layer1…
> 
> Done.
> 
>>> +	fd0 = open(path, O_WRONLY);
>>> +	EXPECT_EQ(0, test_ftruncate(fd0));
>>> +
>>> +	landlock_single_path(_metadata, path,
>>> +			     LANDLOCK_ACCESS_FS_READ_FILE |
>>> +				     LANDLOCK_ACCESS_FS_WRITE_FILE,
>>> +			     LANDLOCK_ACCESS_FS_WRITE_FILE);
>>
>> I'd prefer to follow the current way to write rule layers: write all struct
>> rule at first and then call each enforcement steps. It is a bit more verbose
>> but easier to understand errors. The list of test_ftruncate checks are
>> straightforward to follow.
> 
> Done.
> 
> 
>>> +	fd1 = open(path, O_WRONLY);
>>> +	EXPECT_EQ(0, test_ftruncate(fd0));
>>> +	EXPECT_EQ(0, test_ftruncate(fd1));
>>> +
>>> +	landlock_single_path(_metadata, path, LANDLOCK_ACCESS_FS_TRUNCATE,
>>> +			     LANDLOCK_ACCESS_FS_TRUNCATE);
>>> +
>>> +	fd2 = open(path, O_WRONLY);
>>> +	EXPECT_EQ(0, test_ftruncate(fd0));
>>> +	EXPECT_EQ(0, test_ftruncate(fd1));
>>> +	EXPECT_EQ(0, test_ftruncate(fd2));
>>> +
>>> +	landlock_single_path(_metadata, path,
>>> +			     LANDLOCK_ACCESS_FS_TRUNCATE |
>>> +				     LANDLOCK_ACCESS_FS_WRITE_FILE,
>>> +			     LANDLOCK_ACCESS_FS_WRITE_FILE);
>>> +
>>> +	fd3 = open(path, O_WRONLY);
>>> +	EXPECT_EQ(0, test_ftruncate(fd0));
>>> +	EXPECT_EQ(0, test_ftruncate(fd1));
>>> +	EXPECT_EQ(0, test_ftruncate(fd2));
>>> +	EXPECT_EQ(EACCES, test_ftruncate(fd3));
>>> +
>>> +	ASSERT_EQ(0, close(fd0));
>>> +	ASSERT_EQ(0, close(fd1));
>>> +	ASSERT_EQ(0, close(fd2));
>>> +	ASSERT_EQ(0, close(fd3));
>>> +}
>>> +
>>>    /* clang-format off */
>>>    FIXTURE(layout1_bind) {};
>>>    /* clang-format on */
>