[PATCH v4 2/4] selftests/landlock: Selftests for file truncation support

[Mickaël Salaün]

On 17/08/2022 21:35, Günther Noack wrote:
> On Wed, Aug 17, 2022 at 08:00:17PM +0200, Günther Noack wrote:
>> On Tue, Aug 16, 2022 at 07:08:20PM +0200, Mickaël Salaün wrote:
>>> On 14/08/2022 21:26, Günther Noack wrote:
>>>> +/*
>>>> + * Opens the file and invokes ftruncate(2).
>>>> + *
>>>> + * Returns the errno of ftruncate if ftruncate() fails.
>>>> + * Returns EBADFD if open() or close() fail (should not happen).
>>>> + * Returns 0 if ftruncate(), open() and close() were successful.
>>>> + */
>>>> +static int test_ftruncate(struct __test_metadata *const _metadata,
>>>
>>> _metadata is no longer needed. Same for test_creat().
>>
>> Thanks, well spotted!
>>
>>>
>>>
>>>> +			  const char *const path, int flags)
>>>> +{
>>>> +	int res, err, fd;
>>>> +
>>>> +	fd = open(path, flags | O_CLOEXEC);
>>>> +	if (fd < 0)
>>>> +		return EBADFD;
>>>
>>> Instead of EBADFD, which is a valid open(2) error, you can use ENOSYS and
>>> add a comment explaining that we are not interested by this open(2) error
>>> code but only the ftruncate(2) one because we are sure opening such path is
>>> allowed or because open(2) is already tested before calls to
>>> test_ftruncate().
>>
>> Changed to ENOSYS and added a comment in the code and as function documentation.
>>
>> The explanation follows the reasoning that callers must guarantee that
>> open() and close() will work, in order to test ftruncate() correctly.
>> If open() or close() fail, we return ENOSYS.
>>
>> Technically EBADFD does not get returned by open(2) according to the
>> man page, but I changed it to ENOSYS anyway (EBADF and EBADFD are easy
>> to mix up).
> 
> The more I think about it, the more I feel that test_ftruncate() in its current
> form was a mistake:
> 
>    * In reality, we just care about the ftruncate() result, not about open().
>    * The tests became slightly confusing and asymmetric because some
>      places could call test_ftruncate() while others would call test_open()
>    * Trying open(..., O_RDONLY) + ftruncate() is also confusing in tests,
>      because that never works anyway (ftruncate() only works on writable fds)
> 
> So: I'm contemplating to use a different approach instead:
> 
>    * Open a writable FD for each file *before enforcing Landlock*.
>    * *Then* enforce Landlock (now some of these files can't be opened any more)
>    * Then try ftruncate() with the previously opened file descriptor.
> 
> As a result,
> 
>    * we have some new repetitive but simple code for opening file descriptors
>    * we remove the long version of test_ftruncate() with all its error case
>      complication and replace it with a trivial one that takes an already-opened
>      file descriptor.
> 
> This way, each block in the test now checks the following things:
> 
>    * check truncate(file)
>    * check ftruncate(file_fd) <--- passing the FD!
>    * check open(file, O_RDONLY|O_TRUNC)
>    * check open(file, O_WRONLY|O_TRUNC)
> 
> It's now easy to see in the tests that the result from truncate() and
> ftruncate() is always the same. The complication of worrying whether open()
> works before ftruncate() is also gone (so no more special open() checks needed
> before checking ftruncate()). I removed the testing of ftruncate() on read-only
> file descriptors, because that is forbidden in the ftruncate() manpage anyway
> and always returns EINVAL independent of Landlock.
> 
> I feel that this helps clarify the tests, even if it undoes some of your
> comments about expecting open() to work before ftruncate().
> 
> Does that approach look reasonable to you?
> 
> I might just send you a patch version with that variant I think - this might be
> clearer in code than in my textual description here. :)

The FD from the pre-landlocked state is the right approach, thanks!