On 17/08/2022 21:35, Günther Noack wrote:

On Wed, Aug 17, 2022 at 08:00:17PM +0200, Günther Noack wrote:

quoted

On Tue, Aug 16, 2022 at 07:08:20PM +0200, Mickaël Salaün wrote:

quoted

On 14/08/2022 21:26, Günther Noack wrote:

quoted

+/*
+ * 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!

quoted

quoted

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