[PATCH v3 0/4] landlock: truncate support

[PATCH v3 0/4] landlock: truncate support

[Günther Noack]

The goal of these patches is to work towards a more complete coverage
of file system operations that are restrictable with Landlock.

The known set of currently unsupported file system operations in
Landlock is described at [1]. Out of the operations listed there,
truncate is the only one that modifies file contents, so these patches
should make it possible to prevent the direct modification of file
contents with Landlock.

The patch introduces the truncation restriction feature as an
additional bit in the access_mask_t bitmap, in line with the existing
supported operations.

The truncation flag covers both the truncate(2) and ftruncate(2)
families of syscalls, as well as open(2) with the O_TRUNC flag.
This includes usages of creat() in the case where existing regular
files are overwritten.

Apart from Landlock, file truncation can also be restricted using
seccomp-bpf, but it is more difficult to use (requires BPF, requires
keeping up-to-date syscall lists) and it is not configurable by file
hierarchy, as Landlock is. The simplicity and flexibility of the
Landlock approach makes it worthwhile adding.

While it's possible to use the "write file" and "truncate" rights
independent of each other, it simplifies the mental model for
userspace callers to always use them together.

Specifically, the following behaviours might be surprising for users
when using these independently:

 * The commonly creat() syscall requires the truncate right when
   overwriting existing files, as it is equivalent to open(2) with
   O_TRUNC|O_CREAT|O_WRONLY.
 * The "write file" right is not always required to truncate a file,
   even through the open(2) syscall (when using O_RDONLY|O_TRUNC).

Nevertheless, keeping the two flags separate is the correct approach
to guarantee backwards compatibility for existing Landlock users.

These patches are based on version 5.19.

Best regards,
Günther

[1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags

History:

v3:
 * selftests:
   * Explicitly test ftruncate with readonly file descriptors
     (returns EINVAL).
   * Extract test_ftruncate, test_truncate, test_creat helpers,
     which simplified the previously mixed usage of EXPECT/ASSERT.
   * Test creat() behaviour as part of the big truncation test.
   * Stop testing the truncate64(2) and ftruncate64(2) syscalls.
     This simplifies the tests a bit. The kernel implementations are the
     same as for truncate(2) and ftruncate(2), so there is little benefit
     from testing them exhaustively. (We aren't testing all open(2)
     variants either.)
 * samples/landlock/sandboxer.c:
   * Use switch() to implement best effort mode.
 * Documentation:
   * Give more background on surprising truncation behaviour.
   * Use switch() in the example too, to stay in-line with the sample tool.
   * Small fixes in header file to address previous comments.
* misc:
  * Fix some typos and const usages.

v2:
 * Documentation: Mention the truncation flag where needed.
 * Documentation: Point out connection between truncation and file writing.
 * samples: Add file truncation to the landlock/sandboxer.c sample tool.
 * selftests: Exercise open(2) with O_TRUNC and creat(2) exhaustively.
 * selftests: Exercise truncation syscalls when the truncate right
   is not handled by Landlock.

Previous versions:
v1: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
v2: https://lore.kernel.org/all/20220712211405.14705-1-gnoack3000@gmail.com/

Günther Noack (4):
  landlock: Support file truncation
  selftests/landlock: Selftests for file truncation support
  samples/landlock: Extend sample tool to support
    LANDLOCK_ACCESS_FS_TRUNCATE
  landlock: Document Landlock's file truncation support

 Documentation/userspace-api/landlock.rst     |  47 ++++-
 include/uapi/linux/landlock.h                |  17 +-
 samples/landlock/sandboxer.c                 |  21 +-
 security/landlock/fs.c                       |   9 +-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   | 211 ++++++++++++++++++-
 8 files changed, 283 insertions(+), 28 deletions(-)


base-commit: 3d7cb6b04c3f3115719235cc6866b10326de34cd
--
2.37.1

[PATCH v3 1/4] landlock: Support file truncation

[Günther Noack]

Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.

This flag hooks into the path_truncate LSM hook and covers file
truncation using truncate(2), ftruncate(2), open(2) with O_TRUNC, as
well as creat().

This change also increments the Landlock ABI version, updates
corresponding selftests, and includes minor documentation changes to
document the flag.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 Documentation/userspace-api/landlock.rst     |  6 ++++++
 include/uapi/linux/landlock.h                | 17 ++++++++++++-----
 security/landlock/fs.c                       |  9 ++++++++-
 security/landlock/limits.h                   |  2 +-
 security/landlock/syscalls.c                 |  2 +-
 tools/testing/selftests/landlock/base_test.c |  2 +-
 tools/testing/selftests/landlock/fs_test.c   |  7 ++++---
 7 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index b8ea59493964..d92e335380d4 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -380,6 +380,12 @@ by the Documentation/admin-guide/cgroup-v1/memory.rst.
 Previous limitations
 ====================
 
+File truncation (ABI < 3)
+-------------------------
+
+File truncation could not be denied before the third Landlock ABI, so it is
+always allowed when using a kernel that only supports the first or second ABI.
+
 File renaming and linking (ABI 1)
 ---------------------------------
 
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 23df4e0e8ace..1beb8289708d 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -95,8 +95,15 @@ struct landlock_path_beneath_attr {
  * A file can only receive these access rights:
  *
  * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
- * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access.
+ * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access. Note that
+ *   you might additionally need the LANDLOCK_ACCESS_FS_TRUNCATE right in order
+ *   to overwrite files with :manpage:`open(2)` using O_TRUNC or
+ *   :manpage:`creat(2)`.
  * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
+ * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file through file truncation APIs
+ *   like :manpage:`truncate(2)`, :manpage:`ftruncate(2)`, or
+ *   :manpage:`open(2)` with O_TRUNC or :manpage:`creat(2)`. This access right
+ *   is available since the third version of the Landlock ABI.
  *
  * A directory can receive access rights related to files or directories.  The
  * following access right is applied to the directory itself, and the
@@ -139,10 +146,9 @@ struct landlock_path_beneath_attr {
  *
  *   It is currently not possible to restrict some file-related actions
  *   accessible through these syscall families: :manpage:`chdir(2)`,
- *   :manpage:`truncate(2)`, :manpage:`stat(2)`, :manpage:`flock(2)`,
- *   :manpage:`chmod(2)`, :manpage:`chown(2)`, :manpage:`setxattr(2)`,
- *   :manpage:`utime(2)`, :manpage:`ioctl(2)`, :manpage:`fcntl(2)`,
- *   :manpage:`access(2)`.
+ *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
+ *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
+ *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
  *   Future Landlock evolutions will enable to restrict them.
  */
 /* clang-format off */
@@ -160,6 +166,7 @@ struct landlock_path_beneath_attr {
 #define LANDLOCK_ACCESS_FS_MAKE_BLOCK			(1ULL << 11)
 #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
 #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
+#define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
 /* clang-format on */
 
 #endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index ec5a6247cd3e..c57f581a9cd5 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -146,7 +146,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 #define ACCESS_FILE ( \
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
-	LANDLOCK_ACCESS_FS_READ_FILE)
+	LANDLOCK_ACCESS_FS_READ_FILE | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 /* clang-format on */
 
 /*
@@ -1140,6 +1141,11 @@ static int hook_path_rmdir(const struct path *const dir,
 	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
 }
 
+static int hook_path_truncate(const struct path *const path)
+{
+	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
+}
+
 /* File hooks */
 
 static inline access_mask_t get_file_access(const struct file *const file)
@@ -1192,6 +1198,7 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(path_symlink, hook_path_symlink),
 	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
 	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
+	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
 
 	LSM_HOOK_INIT(file_open, hook_file_open),
 };
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index b54184ab9439..82288f0e9e5e 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -18,7 +18,7 @@
 #define LANDLOCK_MAX_NUM_LAYERS		16
 #define LANDLOCK_MAX_NUM_RULES		U32_MAX
 
-#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_REFER
+#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
 #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
 
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 735a0865ea11..f4d6fc7ed17f 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
 	.write = fop_dummy_write,
 };
 
-#define LANDLOCK_ABI_VERSION 2
+#define LANDLOCK_ABI_VERSION 3
 
 /**
  * sys_landlock_create_ruleset - Create a new ruleset
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index da9290817866..72cdae277b02 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -75,7 +75,7 @@ TEST(abi_version)
 	const struct landlock_ruleset_attr ruleset_attr = {
 		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
 	};
-	ASSERT_EQ(2, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
 					     LANDLOCK_CREATE_RULESET_VERSION));
 
 	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 21a2ce8fa739..cb77eaa01c91 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -399,9 +399,10 @@ TEST_F_FORK(layout1, inval)
 #define ACCESS_FILE ( \
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
-	LANDLOCK_ACCESS_FS_READ_FILE)
+	LANDLOCK_ACCESS_FS_READ_FILE | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 
-#define ACCESS_LAST LANDLOCK_ACCESS_FS_REFER
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
 
 #define ACCESS_ALL ( \
 	ACCESS_FILE | \
@@ -415,7 +416,7 @@ TEST_F_FORK(layout1, inval)
 	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
-	ACCESS_LAST)
+	LANDLOCK_ACCESS_FS_REFER)
 
 /* clang-format on */
 
-- 
2.37.1

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

[Günther Noack]

These tests exercise the following truncation operations:

* truncate() (truncate by path)
* ftruncate() (truncate by file descriptor)
* open with the O_TRUNC flag
* special case: creat(), which is open with O_CREAT|O_WRONLY|O_TRUNC.

in the following scenarios:

* Files with read, write and truncate rights.
* Files with read and truncate rights.
* Files with the truncate right.
* Files without the truncate right.

In particular, the following scenarios are enforced with the test:

* The truncate right is required to use ftruncate,
  even when the thread already has the right to write to the file.
* open() with O_TRUNC requires the truncate right, if it truncates a file.
  open() already checks security_path_truncate() in this case,
  and it required no additional check in the Landlock LSM's file_open hook.
* creat() requires the truncate right
  when called with an existing filename.
* creat() does *not* require the truncate right
  when it's creating a new file.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 tools/testing/selftests/landlock/fs_test.c | 204 +++++++++++++++++++++
 1 file changed, 204 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index cb77eaa01c91..3e84bae7e83a 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -58,6 +58,7 @@ static const char file1_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f1";
 static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2";
 
 static const char dir_s3d1[] = TMP_DIR "/s3d1";
+static const char file1_s3d1[] = TMP_DIR "/s3d1/f1";
 /* dir_s3d2 is a mount point. */
 static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2";
 static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
@@ -83,6 +84,7 @@ static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
  * │           ├── f1
  * │           └── f2
  * └── s3d1
+ *     ├── f1
  *     └── s3d2
  *         └── s3d3
  */
@@ -208,6 +210,7 @@ static void create_layout1(struct __test_metadata *const _metadata)
 	create_file(_metadata, file1_s2d3);
 	create_file(_metadata, file2_s2d3);
 
+	create_file(_metadata, file1_s3d1);
 	create_directory(_metadata, dir_s3d2);
 	set_cap(_metadata, CAP_SYS_ADMIN);
 	ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700"));
@@ -230,6 +233,7 @@ static void remove_layout1(struct __test_metadata *const _metadata)
 	EXPECT_EQ(0, remove_path(file1_s2d2));
 	EXPECT_EQ(0, remove_path(file1_s2d1));
 
+	EXPECT_EQ(0, remove_path(file1_s3d1));
 	EXPECT_EQ(0, remove_path(dir_s3d3));
 	set_cap(_metadata, CAP_SYS_ADMIN);
 	umount(dir_s3d2);
@@ -3023,6 +3027,206 @@ TEST_F_FORK(layout1, proc_pipe)
 	ASSERT_EQ(0, close(pipe_fds[1]));
 }
 
+/* Opens the file, invokes ftruncate(2) and returns the errno or 0. */
+static int test_ftruncate(struct __test_metadata *const _metadata,
+			  const char *const path, int flags)
+{
+	int res, err, fd;
+
+	fd = open(path, flags | O_CLOEXEC);
+	ASSERT_LE(0, fd);
+
+	res = ftruncate(fd, 10);
+	err = errno;
+	ASSERT_EQ(0, close(fd));
+
+	if (res < 0)
+		return err;
+	return 0;
+}
+
+/* Invokes truncate(2) and returns the errno or 0. */
+static int test_truncate(const char *const path)
+{
+	if (truncate(path, 10) < 0)
+		return errno;
+	return 0;
+}
+
+/* Invokes creat(2) and returns the errno or 0. */
+static int test_creat(struct __test_metadata *const _metadata,
+		      const char *const path, mode_t mode)
+{
+	int fd = creat(path, mode);
+
+	if (fd < 0)
+		return errno;
+	ASSERT_EQ(0, close(fd));
+	return 0;
+}
+
+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;
+	const struct rule rules[] = {
+		{
+			.path = file_rwt,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_WRITE_FILE |
+				  LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		{
+			.path = file_rw,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{
+			.path = file_rt,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		{
+			.path = file_t,
+			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		// Implicitly: No access rights for file_none.
+		{
+			.path = dir_t,
+			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		{
+			.path = dir_w,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{},
+	};
+	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
+			      LANDLOCK_ACCESS_FS_WRITE_FILE |
+			      LANDLOCK_ACCESS_FS_TRUNCATE;
+	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
+
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/*
+	 * Checks read, write and truncate rights: truncation works.
+	 *
+	 * Note: Independent of Landlock, ftruncate(2) on read-only
+	 * file descriptors never works.
+	 */
+	EXPECT_EQ(0, test_ftruncate(_metadata, file_rwt, O_WRONLY));
+	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rwt, O_RDONLY));
+	EXPECT_EQ(0, test_truncate(file_rwt));
+	EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
+
+	/* Checks read and write rights: no truncate variant works. */
+	EXPECT_EQ(EACCES, test_ftruncate(_metadata, file_rw, O_WRONLY));
+	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rw, O_RDONLY));
+	EXPECT_EQ(EACCES, test_truncate(file_rw));
+	EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC));
+
+	/*
+	 * Checks read and truncate rights: truncation works.
+	 *
+	 * Note: Files opened in O_RDONLY can get truncated as part of
+	 * the same operation.
+	 */
+	EXPECT_EQ(0, test_open(file_rt, O_RDONLY));
+	EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
+	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
+	EXPECT_EQ(0, test_truncate(file_rt));
+
+	/* Checks truncate right: truncate works, but can't open file. */
+	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY));
+	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY));
+	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(0, test_truncate(file_t));
+
+	/* Checks "no rights" case: No form of truncation works. */
+	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
+	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY));
+	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_truncate(file_none));
+
+	/* Checks truncate right on directory: truncate works on contained files */
+	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY));
+	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY));
+	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(0, test_truncate(file_in_dir_t));
+
+	/*
+	 * Checks creat in dir_w: This requires the truncate right
+	 * when overwriting an existing file, but does not require it
+	 * when the file is new.
+	 */
+	EXPECT_EQ(EACCES, test_creat(_metadata, file_in_dir_w, 0600));
+
+	ASSERT_EQ(0, unlink(file_in_dir_w));
+	EXPECT_EQ(0, test_creat(_metadata, file_in_dir_w, 0600));
+}
+
+/*
+ * Exercises file truncation when it's not restricted,
+ * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed.
+ */
+TEST_F_FORK(layout1, truncate_unhandled)
+{
+	const char *const file_r = file1_s1d1;
+	const char *const file_w = file2_s1d1;
+	const char *const file_none = file1_s1d2;
+	const struct rule rules[] = {
+		{
+			.path = file_r,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{
+			.path = file_w,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		// Implicitly: No rights for file_none.
+		{},
+	};
+	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
+			      LANDLOCK_ACCESS_FS_WRITE_FILE;
+	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
+
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/* Checks read right: truncation should work through truncate and open. */
+	EXPECT_EQ(0, test_truncate(file_r));
+	EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC));
+
+	/* Checks write right: truncation should work through truncate, ftruncate and open. */
+	EXPECT_EQ(0, test_truncate(file_w));
+	EXPECT_EQ(0, test_ftruncate(_metadata, file_w, O_WRONLY));
+	EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC));
+
+	/* Checks "no rights" case: truncate works but all open attempts fail. */
+	EXPECT_EQ(0, test_truncate(file_none));
+	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.37.1

[PATCH v3 3/4] samples/landlock: Extend sample tool to support LANDLOCK_ACCESS_FS_TRUNCATE

[Günther Noack]

Update the sandboxer sample to restrict truncate actions. This is
automatically enabled by default if the running kernel supports
LANDLOCK_ACCESS_FS_TRUNCATE, expect for the paths listed in the
LL_FS_RW environment variable.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 samples/landlock/sandboxer.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 3e404e51ec64..d266f3abbf63 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -76,7 +76,8 @@ static int parse_path(char *env_path, const char ***const path_list)
 #define ACCESS_FILE ( \
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
-	LANDLOCK_ACCESS_FS_READ_FILE)
+	LANDLOCK_ACCESS_FS_READ_FILE | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 
 /* clang-format on */
 
@@ -160,10 +161,8 @@ static int populate_ruleset(const char *const env_var, const int ruleset_fd,
 	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
-	LANDLOCK_ACCESS_FS_REFER)
-
-#define ACCESS_ABI_2 ( \
-	LANDLOCK_ACCESS_FS_REFER)
+	LANDLOCK_ACCESS_FS_REFER | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 
 /* clang-format on */
 
@@ -226,11 +225,15 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		return 1;
 	}
 	/* Best-effort security. */
-	if (abi < 2) {
-		ruleset_attr.handled_access_fs &= ~ACCESS_ABI_2;
-		access_fs_ro &= ~ACCESS_ABI_2;
-		access_fs_rw &= ~ACCESS_ABI_2;
+	switch (abi) {
+	case 1:
+		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
+		__attribute__((fallthrough));
+	case 2:
+		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
 	}
+	access_fs_ro &= ruleset_attr.handled_access_fs;
+	access_fs_rw &= ruleset_attr.handled_access_fs;
 
 	ruleset_fd =
 		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
-- 
2.37.1

[PATCH v3 4/4] landlock: Document Landlock's file truncation support

[Günther Noack]

Use the LANDLOCK_ACCESS_FS_TRUNCATE flag in the tutorial.

Adapt the backwards compatibility example and discussion to remove the
truncation flag where needed.

Point out potential surprising behaviour related to truncate.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 Documentation/userspace-api/landlock.rst | 41 ++++++++++++++++++++----
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index d92e335380d4..9c3c9fa65958 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -60,7 +60,8 @@ the need to be explicit about the denied-by-default access rights.
             LANDLOCK_ACCESS_FS_MAKE_FIFO |
             LANDLOCK_ACCESS_FS_MAKE_BLOCK |
             LANDLOCK_ACCESS_FS_MAKE_SYM |
-            LANDLOCK_ACCESS_FS_REFER,
+            LANDLOCK_ACCESS_FS_REFER |
+            LANDLOCK_ACCESS_FS_TRUNCATE,
     };
 
 Because we may not know on which kernel version an application will be
@@ -69,16 +70,24 @@ should try to protect users as much as possible whatever the kernel they are
 using.  To avoid binary enforcement (i.e. either all security features or
 none), we can leverage a dedicated Landlock command to get the current version
 of the Landlock ABI and adapt the handled accesses.  Let's check if we should
-remove the `LANDLOCK_ACCESS_FS_REFER` access right which is only supported
-starting with the second version of the ABI.
+remove the `LANDLOCK_ACCESS_FS_REFER` and `LANDLOCK_ACCESS_FS_TRUNCATE` access
+rights, which are only supported starting with the second and third version of
+the ABI.
 
 .. code-block:: c
 
     int abi;
 
     abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION);
-    if (abi < 2) {
-        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
+    switch (abi) {
+    case -1:
+            perror("Landlock is not supported with the running kernel");
+            return 1;
+    case 1:
+            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
+            __attribute__((fallthrough));
+    case 2:
+            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
     }
 
 This enables to create an inclusive ruleset that will contain our rules.
@@ -127,8 +136,8 @@ descriptor.
 
 It may also be required to create rules following the same logic as explained
 for the ruleset creation, by filtering access rights according to the Landlock
-ABI version.  In this example, this is not required because
-`LANDLOCK_ACCESS_FS_REFER` is not allowed by any rule.
+ABI version.  In this example, this is not required because all of the requested
+``allowed_access`` rights are already available in ABI 1.
 
 We now have a ruleset with one rule allowing read access to ``/usr`` while
 denying all other handled accesses for the filesystem.  The next step is to
@@ -251,6 +260,24 @@ To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target
 process, a sandboxed process should have a subset of the target process rules,
 which means the tracee must be in a sub-domain of the tracer.
 
+Truncating files
+----------------
+
+The operations covered by `LANDLOCK_ACCESS_FS_WRITE_FILE` and
+`LANDLOCK_ACCESS_FS_TRUNCATE` both change the contents of a file and
+sometimes overlap in non-intuitive ways. It is recommended to always
+specify both of these together.
+
+A particularly surprising example is :manpage:`creat(2)`. The name
+suggests that this system call requires the rights to create and write
+files. However, it also requires the truncate right if an existing
+file under the same name is already present.
+
+It should also be noted that truncating files does not necessarily
+require the `LANDLOCK_ACCESS_FS_WRITE_FILE` right. Apart from the
+obvious :manpage:`truncate(2)` system call, this can also be done
+through :manpage:`open(2)` with the flags `O_RDONLY` and `O_TRUNC`.
+
 Compatibility
 =============
 
-- 
2.37.1

Re: [PATCH v3 1/4] landlock: Support file truncation

[Mickaël Salaün]

On 04/08/2022 21:37, Günther Noack wrote:
> Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
> 
> This flag hooks into the path_truncate LSM hook and covers file
> truncation using truncate(2), ftruncate(2), open(2) with O_TRUNC, as
> well as creat().
> 
> This change also increments the Landlock ABI version, updates
> corresponding selftests, and includes minor documentation changes to
> document the flag.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   Documentation/userspace-api/landlock.rst     |  6 ++++++
>   include/uapi/linux/landlock.h                | 17 ++++++++++++-----
>   security/landlock/fs.c                       |  9 ++++++++-
>   security/landlock/limits.h                   |  2 +-
>   security/landlock/syscalls.c                 |  2 +-
>   tools/testing/selftests/landlock/base_test.c |  2 +-
>   tools/testing/selftests/landlock/fs_test.c   |  7 ++++---
>   7 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index b8ea59493964..d92e335380d4 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -380,6 +380,12 @@ by the Documentation/admin-guide/cgroup-v1/memory.rst.
>   Previous limitations
>   ====================
>   
> +File truncation (ABI < 3)
> +-------------------------
> +
> +File truncation could not be denied before the third Landlock ABI, so it is
> +always allowed when using a kernel that only supports the first or second ABI.

I think this addition could make the documentation more consistent and 
helpful:

Starting with the Landlock ABI version 3, it is now possible to securely
control truncation thanks to the new `LANDLOCK_ACCESS_FS_TRUNCATE`
access right.


> +
>   File renaming and linking (ABI 1)
>   ---------------------------------
>   
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 23df4e0e8ace..1beb8289708d 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -95,8 +95,15 @@ struct landlock_path_beneath_attr {
>    * A file can only receive these access rights:
>    *
>    * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
> - * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access.
> + * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access. Note that
> + *   you might additionally need the LANDLOCK_ACCESS_FS_TRUNCATE right in order

Please use backquotes for code such as `LANDLOCK_ACCESS_FS_TRUNCATE`, 
`O_TRUNC`…


> + *   to overwrite files with :manpage:`open(2)` using O_TRUNC or
> + *   :manpage:`creat(2)`.
>    * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
> + * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file through file truncation APIs
> + *   like :manpage:`truncate(2)`, :manpage:`ftruncate(2)`, or
> + *   :manpage:`open(2)` with O_TRUNC or :manpage:`creat(2)`. This access right
> + *   is available since the third version of the Landlock ABI.
>    *
>    * A directory can receive access rights related to files or directories.  The
>    * following access right is applied to the directory itself, and the
> @@ -139,10 +146,9 @@ struct landlock_path_beneath_attr {
>    *
>    *   It is currently not possible to restrict some file-related actions
>    *   accessible through these syscall families: :manpage:`chdir(2)`,
> - *   :manpage:`truncate(2)`, :manpage:`stat(2)`, :manpage:`flock(2)`,
> - *   :manpage:`chmod(2)`, :manpage:`chown(2)`, :manpage:`setxattr(2)`,
> - *   :manpage:`utime(2)`, :manpage:`ioctl(2)`, :manpage:`fcntl(2)`,
> - *   :manpage:`access(2)`.
> + *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
> + *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
> + *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
>    *   Future Landlock evolutions will enable to restrict them.
>    */
>   /* clang-format off */

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

[Mickaël Salaün]

On 04/08/2022 21:37, Günther Noack wrote:
> These tests exercise the following truncation operations:
> 
> * truncate() (truncate by path)
> * ftruncate() (truncate by file descriptor)
> * open with the O_TRUNC flag
> * special case: creat(), which is open with O_CREAT|O_WRONLY|O_TRUNC.
> 
> in the following scenarios:
> 
> * Files with read, write and truncate rights.
> * Files with read and truncate rights.
> * Files with the truncate right.
> * Files without the truncate right.
> 
> In particular, the following scenarios are enforced with the test:
> 
> * The truncate right is required to use ftruncate,
>    even when the thread already has the right to write to the file.
> * open() with O_TRUNC requires the truncate right, if it truncates a file.
>    open() already checks security_path_truncate() in this case,
>    and it required no additional check in the Landlock LSM's file_open hook.
> * creat() requires the truncate right
>    when called with an existing filename.
> * creat() does *not* require the truncate right
>    when it's creating a new file.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   tools/testing/selftests/landlock/fs_test.c | 204 +++++++++++++++++++++
>   1 file changed, 204 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index cb77eaa01c91..3e84bae7e83a 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -58,6 +58,7 @@ static const char file1_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f1";
>   static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2";
>   
>   static const char dir_s3d1[] = TMP_DIR "/s3d1";
> +static const char file1_s3d1[] = TMP_DIR "/s3d1/f1";
>   /* dir_s3d2 is a mount point. */
>   static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2";
>   static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
> @@ -83,6 +84,7 @@ static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
>    * │           ├── f1
>    * │           └── f2
>    * └── s3d1
> + *     ├── f1
>    *     └── s3d2
>    *         └── s3d3
>    */
> @@ -208,6 +210,7 @@ static void create_layout1(struct __test_metadata *const _metadata)
>   	create_file(_metadata, file1_s2d3);
>   	create_file(_metadata, file2_s2d3);
>   
> +	create_file(_metadata, file1_s3d1);
>   	create_directory(_metadata, dir_s3d2);
>   	set_cap(_metadata, CAP_SYS_ADMIN);
>   	ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700"));
> @@ -230,6 +233,7 @@ static void remove_layout1(struct __test_metadata *const _metadata)
>   	EXPECT_EQ(0, remove_path(file1_s2d2));
>   	EXPECT_EQ(0, remove_path(file1_s2d1));
>   
> +	EXPECT_EQ(0, remove_path(file1_s3d1));
>   	EXPECT_EQ(0, remove_path(dir_s3d3));
>   	set_cap(_metadata, CAP_SYS_ADMIN);
>   	umount(dir_s3d2);
> @@ -3023,6 +3027,206 @@ TEST_F_FORK(layout1, proc_pipe)
>   	ASSERT_EQ(0, close(pipe_fds[1]));
>   }
>   
> +/* Opens the file, invokes ftruncate(2) and returns the errno or 0. */
> +static int test_ftruncate(struct __test_metadata *const _metadata,
> +			  const char *const path, int flags)
> +{
> +	int res, err, fd;
> +
> +	fd = open(path, flags | O_CLOEXEC);
> +	ASSERT_LE(0, fd);
> +
> +	res = ftruncate(fd, 10);
> +	err = errno;
> +	ASSERT_EQ(0, close(fd));
> +
> +	if (res < 0)
> +		return err;
> +	return 0;
> +}
> +
> +/* Invokes truncate(2) and returns the errno or 0. */
> +static int test_truncate(const char *const path)
> +{
> +	if (truncate(path, 10) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +/* Invokes creat(2) and returns the errno or 0. */
> +static int test_creat(struct __test_metadata *const _metadata,
> +		      const char *const path, mode_t mode)
> +{
> +	int fd = creat(path, mode);
> +
> +	if (fd < 0)
> +		return errno;
> +	ASSERT_EQ(0, close(fd));

test_creat() contains an ASSERT, which would only print this line, which 
would not help much if it is called multiple times, which is the case. I 
prefer not passing _metadata but only returning errno or 0 and handling 
the ASSERT in layout1.truncate* . It is not the case everywhere but we 
should try to follow this rule as much as possible.


> +	return 0;
> +}
> +
> +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;
> +	const struct rule rules[] = {
> +		{
> +			.path = file_rwt,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE |
> +				  LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		{
> +			.path = file_rw,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = file_rt,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		{
> +			.path = file_t,
> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		// Implicitly: No access rights for file_none.
> +		{
> +			.path = dir_t,
> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		{
> +			.path = dir_w,
> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{},
> +	};
> +	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
> +			      LANDLOCK_ACCESS_FS_WRITE_FILE |
> +			      LANDLOCK_ACCESS_FS_TRUNCATE;
> +	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> +
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/*
> +	 * Checks read, write and truncate rights: truncation works.
> +	 *
> +	 * Note: Independent of Landlock, ftruncate(2) on read-only
> +	 * file descriptors never works.
> +	 */
> +	EXPECT_EQ(0, test_ftruncate(_metadata, file_rwt, O_WRONLY));

Other than following the original Google Test documentation, what is the 
advantage to use EXPECT? Don't you think it would be harder to debug a 
bunch of failed expect? What is the reason for other test frameworks to 
not implement EXPECT? How Chromium or other Google code really use it? 
Genuine questions.


> +	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rwt, O_RDONLY));
> +	EXPECT_EQ(0, test_truncate(file_rwt));
> +	EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
> +
> +	/* Checks read and write rights: no truncate variant works. */
> +	EXPECT_EQ(EACCES, test_ftruncate(_metadata, file_rw, O_WRONLY));
> +	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rw, O_RDONLY));
> +	EXPECT_EQ(EACCES, test_truncate(file_rw));
> +	EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC));
> +
> +	/*
> +	 * Checks read and truncate rights: truncation works.
> +	 *
> +	 * Note: Files opened in O_RDONLY can get truncated as part of
> +	 * the same operation.
> +	 */
> +	EXPECT_EQ(0, test_open(file_rt, O_RDONLY));
> +	EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
> +	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
> +	EXPECT_EQ(0, test_truncate(file_rt));
> +
> +	/* Checks truncate right: truncate works, but can't open file. */
> +	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY));
> +	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY));
> +	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(0, test_truncate(file_t));
> +
> +	/* Checks "no rights" case: No form of truncation works. */
> +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_truncate(file_none));
> +
> +	/* Checks truncate right on directory: truncate works on contained files */
> +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY));
> +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY));
> +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(0, test_truncate(file_in_dir_t));
> +
> +	/*
> +	 * Checks creat in dir_w: This requires the truncate right
> +	 * when overwriting an existing file, but does not require it
> +	 * when the file is new.
> +	 */
> +	EXPECT_EQ(EACCES, test_creat(_metadata, file_in_dir_w, 0600));
> +
> +	ASSERT_EQ(0, unlink(file_in_dir_w));
> +	EXPECT_EQ(0, test_creat(_metadata, file_in_dir_w, 0600));
> +}
> +
> +/*
> + * Exercises file truncation when it's not restricted,
> + * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed.
> + */
> +TEST_F_FORK(layout1, truncate_unhandled)
> +{
> +	const char *const file_r = file1_s1d1;
> +	const char *const file_w = file2_s1d1;
> +	const char *const file_none = file1_s1d2;
> +	const struct rule rules[] = {
> +		{
> +			.path = file_r,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE,
> +		},
> +		{
> +			.path = file_w,
> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		// Implicitly: No rights for file_none.
> +		{},
> +	};
> +	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
> +			      LANDLOCK_ACCESS_FS_WRITE_FILE;
> +	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> +
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Checks read right: truncation should work through truncate and open. */
> +	EXPECT_EQ(0, test_truncate(file_r));
> +	EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC));
> +
> +	/* Checks write right: truncation should work through truncate, ftruncate and open. */
> +	EXPECT_EQ(0, test_truncate(file_w));
> +	EXPECT_EQ(0, test_ftruncate(_metadata, file_w, O_WRONLY));
> +	EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC));
> +
> +	/* Checks "no rights" case: truncate works but all open attempts fail. */
> +	EXPECT_EQ(0, test_truncate(file_none));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
> +}

These tests looks good!

> +
>   /* clang-format off */
>   FIXTURE(layout1_bind) {};
>   /* clang-format on */

Re: [PATCH v3 4/4] landlock: Document Landlock's file truncation support

[Mickaël Salaün]

On 04/08/2022 21:37, Günther Noack wrote:
> Use the LANDLOCK_ACCESS_FS_TRUNCATE flag in the tutorial.
> 
> Adapt the backwards compatibility example and discussion to remove the
> truncation flag where needed.
> 
> Point out potential surprising behaviour related to truncate.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   Documentation/userspace-api/landlock.rst | 41 ++++++++++++++++++++----
>   1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index d92e335380d4..9c3c9fa65958 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -60,7 +60,8 @@ the need to be explicit about the denied-by-default access rights.
>               LANDLOCK_ACCESS_FS_MAKE_FIFO |
>               LANDLOCK_ACCESS_FS_MAKE_BLOCK |
>               LANDLOCK_ACCESS_FS_MAKE_SYM |
> -            LANDLOCK_ACCESS_FS_REFER,
> +            LANDLOCK_ACCESS_FS_REFER |
> +            LANDLOCK_ACCESS_FS_TRUNCATE,
>       };
>   
>   Because we may not know on which kernel version an application will be
> @@ -69,16 +70,24 @@ should try to protect users as much as possible whatever the kernel they are
>   using.  To avoid binary enforcement (i.e. either all security features or
>   none), we can leverage a dedicated Landlock command to get the current version
>   of the Landlock ABI and adapt the handled accesses.  Let's check if we should
> -remove the `LANDLOCK_ACCESS_FS_REFER` access right which is only supported
> -starting with the second version of the ABI.
> +remove the `LANDLOCK_ACCESS_FS_REFER` and `LANDLOCK_ACCESS_FS_TRUNCATE` access
> +rights, which are only supported starting with the second and third version of
> +the ABI.
>   
>   .. code-block:: c
>   
>       int abi;
>   
>       abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION);
> -    if (abi < 2) {
> -        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
> +    switch (abi) {
> +    case -1:
> +            perror("Landlock is not supported with the running kernel");

Because there is a distinction between "supported" and "enabled" (as 
explained in the sample), let's make this message more generic. The 
additional strerror() output would then be enough to distinguish the 
error type.

"The running kernel does not enable to use Landlock"

> +            return 1;
> +    case 1:

This switch/case logic might be a bit confusing; let's explain it for 
this doc *and the sample code*:

/* Removes LANDLOCK_ACCESS_FS_REFER for ABI < 2 */

> +            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
> +            __attribute__((fallthrough));
> +    case 2:

/* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */

> +            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
>       }
>   
>   This enables to create an inclusive ruleset that will contain our rules.
> @@ -127,8 +136,8 @@ descriptor.
>   
>   It may also be required to create rules following the same logic as explained
>   for the ruleset creation, by filtering access rights according to the Landlock
> -ABI version.  In this example, this is not required because
> -`LANDLOCK_ACCESS_FS_REFER` is not allowed by any rule.
> +ABI version.  In this example, this is not required because all of the requested
> +``allowed_access`` rights are already available in ABI 1.
>   
>   We now have a ruleset with one rule allowing read access to ``/usr`` while
>   denying all other handled accesses for the filesystem.  The next step is to
> @@ -251,6 +260,24 @@ To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target
>   process, a sandboxed process should have a subset of the target process rules,
>   which means the tracee must be in a sub-domain of the tracer.
>   
> +Truncating files
> +----------------
> +
> +The operations covered by `LANDLOCK_ACCESS_FS_WRITE_FILE` and
> +`LANDLOCK_ACCESS_FS_TRUNCATE` both change the contents of a file and
> +sometimes overlap in non-intuitive ways. It is recommended to always
> +specify both of these together.
> +
> +A particularly surprising example is :manpage:`creat(2)`. The name
> +suggests that this system call requires the rights to create and write
> +files. However, it also requires the truncate right if an existing
> +file under the same name is already present.
> +
> +It should also be noted that truncating files does not necessarily

FYI, I'll send a standalone patch to remove all contractions and get a 
more consistent documentation. Please, keep it this way.


> +require the `LANDLOCK_ACCESS_FS_WRITE_FILE` right. Apart from the
> +obvious :manpage:`truncate(2)` system call, this can also be done
> +through :manpage:`open(2)` with the flags `O_RDONLY` and `O_TRUNC`.

Good!

nit: you can use a 80-columns limit.

> +
>   Compatibility
>   =============
>   

Re: [PATCH v3 1/4] landlock: Support file truncation

[Günther Noack]

On Thu, Aug 11, 2022 at 06:59:28PM +0200, Mickaël Salaün wrote:
>
> On 04/08/2022 21:37, Günther Noack wrote:
> > Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
> >
> > This flag hooks into the path_truncate LSM hook and covers file
> > truncation using truncate(2), ftruncate(2), open(2) with O_TRUNC, as
> > well as creat().
> >
> > This change also increments the Landlock ABI version, updates
> > corresponding selftests, and includes minor documentation changes to
> > document the flag.
> >
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >   Documentation/userspace-api/landlock.rst     |  6 ++++++
> >   include/uapi/linux/landlock.h                | 17 ++++++++++++-----
> >   security/landlock/fs.c                       |  9 ++++++++-
> >   security/landlock/limits.h                   |  2 +-
> >   security/landlock/syscalls.c                 |  2 +-
> >   tools/testing/selftests/landlock/base_test.c |  2 +-
> >   tools/testing/selftests/landlock/fs_test.c   |  7 ++++---
> >   7 files changed, 33 insertions(+), 12 deletions(-)
> >
> > diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> > index b8ea59493964..d92e335380d4 100644
> > --- a/Documentation/userspace-api/landlock.rst
> > +++ b/Documentation/userspace-api/landlock.rst
> > @@ -380,6 +380,12 @@ by the Documentation/admin-guide/cgroup-v1/memory.rst.
> >   Previous limitations
> >   ====================
> > +File truncation (ABI < 3)
> > +-------------------------
> > +
> > +File truncation could not be denied before the third Landlock ABI, so it is
> > +always allowed when using a kernel that only supports the first or second ABI.
>
> I think this addition could make the documentation more consistent and
> helpful:
>
> Starting with the Landlock ABI version 3, it is now possible to securely
> control truncation thanks to the new `LANDLOCK_ACCESS_FS_TRUNCATE`
> access right.

Agreed, I added that sentence.

>
>
> > +
> >   File renaming and linking (ABI 1)
> >   ---------------------------------
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 23df4e0e8ace..1beb8289708d 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -95,8 +95,15 @@ struct landlock_path_beneath_attr {
> >    * A file can only receive these access rights:
> >    *
> >    * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
> > - * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access.
> > + * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access. Note that
> > + *   you might additionally need the LANDLOCK_ACCESS_FS_TRUNCATE right in order
>
> Please use backquotes for code such as `LANDLOCK_ACCESS_FS_TRUNCATE`,
> `O_TRUNC`…

Done.

>
>
> > + *   to overwrite files with :manpage:`open(2)` using O_TRUNC or
> > + *   :manpage:`creat(2)`.
> >    * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
> > + * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file through file truncation APIs
> > + *   like :manpage:`truncate(2)`, :manpage:`ftruncate(2)`, or
> > + *   :manpage:`open(2)` with O_TRUNC or :manpage:`creat(2)`. This access right
> > + *   is available since the third version of the Landlock ABI.
> >    *
> >    * A directory can receive access rights related to files or directories.  The
> >    * following access right is applied to the directory itself, and the
> > @@ -139,10 +146,9 @@ struct landlock_path_beneath_attr {
> >    *
> >    *   It is currently not possible to restrict some file-related actions
> >    *   accessible through these syscall families: :manpage:`chdir(2)`,
> > - *   :manpage:`truncate(2)`, :manpage:`stat(2)`, :manpage:`flock(2)`,
> > - *   :manpage:`chmod(2)`, :manpage:`chown(2)`, :manpage:`setxattr(2)`,
> > - *   :manpage:`utime(2)`, :manpage:`ioctl(2)`, :manpage:`fcntl(2)`,
> > - *   :manpage:`access(2)`.
> > + *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
> > + *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
> > + *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
> >    *   Future Landlock evolutions will enable to restrict them.
> >    */
> >   /* clang-format off */

--

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

[Günther Noack]

On Thu, Aug 11, 2022 at 06:59:38PM +0200, Mickaël Salaün wrote:
>
> On 04/08/2022 21:37, Günther Noack wrote:
> > These tests exercise the following truncation operations:
> >
> > * truncate() (truncate by path)
> > * ftruncate() (truncate by file descriptor)
> > * open with the O_TRUNC flag
> > * special case: creat(), which is open with O_CREAT|O_WRONLY|O_TRUNC.
> >
> > in the following scenarios:
> >
> > * Files with read, write and truncate rights.
> > * Files with read and truncate rights.
> > * Files with the truncate right.
> > * Files without the truncate right.
> >
> > In particular, the following scenarios are enforced with the test:
> >
> > * The truncate right is required to use ftruncate,
> >    even when the thread already has the right to write to the file.
> > * open() with O_TRUNC requires the truncate right, if it truncates a file.
> >    open() already checks security_path_truncate() in this case,
> >    and it required no additional check in the Landlock LSM's file_open hook.
> > * creat() requires the truncate right
> >    when called with an existing filename.
> > * creat() does *not* require the truncate right
> >    when it's creating a new file.
> >
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >   tools/testing/selftests/landlock/fs_test.c | 204 +++++++++++++++++++++
> >   1 file changed, 204 insertions(+)
> >
> > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > index cb77eaa01c91..3e84bae7e83a 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c
> > @@ -58,6 +58,7 @@ static const char file1_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f1";
> >   static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2";
> >   static const char dir_s3d1[] = TMP_DIR "/s3d1";
> > +static const char file1_s3d1[] = TMP_DIR "/s3d1/f1";
> >   /* dir_s3d2 is a mount point. */
> >   static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2";
> >   static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
> > @@ -83,6 +84,7 @@ static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
> >    * │           ├── f1
> >    * │           └── f2
> >    * └── s3d1
> > + *     ├── f1
> >    *     └── s3d2
> >    *         └── s3d3
> >    */
> > @@ -208,6 +210,7 @@ static void create_layout1(struct __test_metadata *const _metadata)
> >   	create_file(_metadata, file1_s2d3);
> >   	create_file(_metadata, file2_s2d3);
> > +	create_file(_metadata, file1_s3d1);
> >   	create_directory(_metadata, dir_s3d2);
> >   	set_cap(_metadata, CAP_SYS_ADMIN);
> >   	ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700"));
> > @@ -230,6 +233,7 @@ static void remove_layout1(struct __test_metadata *const _metadata)
> >   	EXPECT_EQ(0, remove_path(file1_s2d2));
> >   	EXPECT_EQ(0, remove_path(file1_s2d1));
> > +	EXPECT_EQ(0, remove_path(file1_s3d1));
> >   	EXPECT_EQ(0, remove_path(dir_s3d3));
> >   	set_cap(_metadata, CAP_SYS_ADMIN);
> >   	umount(dir_s3d2);
> > @@ -3023,6 +3027,206 @@ TEST_F_FORK(layout1, proc_pipe)
> >   	ASSERT_EQ(0, close(pipe_fds[1]));
> >   }
> > +/* Opens the file, invokes ftruncate(2) and returns the errno or 0. */
> > +static int test_ftruncate(struct __test_metadata *const _metadata,
> > +			  const char *const path, int flags)
> > +{
> > +	int res, err, fd;
> > +
> > +	fd = open(path, flags | O_CLOEXEC);
> > +	ASSERT_LE(0, fd);
> > +
> > +	res = ftruncate(fd, 10);
> > +	err = errno;
> > +	ASSERT_EQ(0, close(fd));
> > +
> > +	if (res < 0)
> > +		return err;
> > +	return 0;
> > +}
> > +
> > +/* Invokes truncate(2) and returns the errno or 0. */
> > +static int test_truncate(const char *const path)
> > +{
> > +	if (truncate(path, 10) < 0)
> > +		return errno;
> > +	return 0;
> > +}
> > +
> > +/* Invokes creat(2) and returns the errno or 0. */
> > +static int test_creat(struct __test_metadata *const _metadata,
> > +		      const char *const path, mode_t mode)
> > +{
> > +	int fd = creat(path, mode);
> > +
> > +	if (fd < 0)
> > +		return errno;
> > +	ASSERT_EQ(0, close(fd));
>
> test_creat() contains an ASSERT, which would only print this line, which
> would not help much if it is called multiple times, which is the case. I
> prefer not passing _metadata but only returning errno or 0 and handling the
> ASSERT in layout1.truncate* . It is not the case everywhere but we should
> try to follow this rule as much as possible.

Thanks, that's a good point that the line number attribution becomes confusing.

I changed these ASSERT_EQ() calls to just return the errno directly.


> > +	return 0;
> > +}
> > +
> > +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;
> > +	const struct rule rules[] = {
> > +		{
> > +			.path = file_rwt,
> > +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> > +				  LANDLOCK_ACCESS_FS_WRITE_FILE |
> > +				  LANDLOCK_ACCESS_FS_TRUNCATE,
> > +		},
> > +		{
> > +			.path = file_rw,
> > +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> > +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> > +		},
> > +		{
> > +			.path = file_rt,
> > +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> > +				  LANDLOCK_ACCESS_FS_TRUNCATE,
> > +		},
> > +		{
> > +			.path = file_t,
> > +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> > +		},
> > +		// Implicitly: No access rights for file_none.
> > +		{
> > +			.path = dir_t,
> > +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> > +		},
> > +		{
> > +			.path = dir_w,
> > +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> > +		},
> > +		{},
> > +	};
> > +	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
> > +			      LANDLOCK_ACCESS_FS_WRITE_FILE |
> > +			      LANDLOCK_ACCESS_FS_TRUNCATE;
> > +	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> > +
> > +	ASSERT_LE(0, ruleset_fd);
> > +	enforce_ruleset(_metadata, ruleset_fd);
> > +	ASSERT_EQ(0, close(ruleset_fd));
> > +
> > +	/*
> > +	 * Checks read, write and truncate rights: truncation works.
> > +	 *
> > +	 * Note: Independent of Landlock, ftruncate(2) on read-only
> > +	 * file descriptors never works.
> > +	 */
> > +	EXPECT_EQ(0, test_ftruncate(_metadata, file_rwt, O_WRONLY));
>
> Other than following the original Google Test documentation, what is the
> advantage to use EXPECT? Don't you think it would be harder to debug a bunch
> of failed expect?

<This is maybe slightly philosophical, which means that I have no hard
proof for any of this, but I'm happy to discuss it :)>

I think at the root, most test frameworks agree that the test output
should ideally be a list of mostly-independent test scenarios together
with their failure statuses.

I think it is actually *easier* to debug a bunch of failed
expectations if they fail at the same time: When multiple of these
scenarios fail at the same time, that would give you a hint that
something more fundamental is broken, whereas when only one of them
fails, you'd know that it is a problem specific to the test scenario
at hand.

> What is the reason for other test frameworks to not
> implement EXPECT?

The way that the different test frameworks try to achieve this "table
of scenarios" output is different:

The JUnit/xUnit-style frameworks only have an ASSERT-equivalent, and I
think in these testing cultures there is a greater emphasis on having
a separate test function for each exercised scenario. That way, you
still end up with a long table of test scenarios with their failure
statuses. -- But that is not the test structure that we have here in
the Landlock selftests.

In Googletest, or in the Go test framework, there is a distinction
between ASSERT and EXPECT, and people are more encouraged to test
multiple similar scenarios in the same function (in Go, they call it a
"table driven test"). It works as well, as long as people take care
that the scenarios that are tested together don't accidentally depend
on each others left-over state.

I'm not sure why some test frameworks do this with assert-only and
others distinguish between assert and expect.

I *suspect* that in C and C++, it is more difficult to factor out
shared test setup code than in Java or Python, and so people tend to
write bigger test functions that exercise more scenarios at once, and
then the distinction between assert and expect became more useful.

<end of philosophical derail>


> How Chromium or other Google code really use it? Genuine questions.

This is how Chromium uses it:

https://source.chromium.org/search?q=ASSERT_EQ (~6000 hits)
https://source.chromium.org/search?q=EXPECT_EQ (~5800 hits)

This is how Absl uses it (from the website: "Abseil is an open source
collection of C++ libraries drawn from the most fundamental pieces of
Google’s internal codebase."):

https://github.com/abseil/abseil-cpp/search?q=EXPECT_EQ (163 results)
https://github.com/abseil/abseil-cpp/search?q=ASSERT_EQ (24 results)

In Chromium, there are clearly some corners where they use ASSERT
exclusively. In Abseil, I think it gets used more in the spirit of the
framework, but it's also a smaller codebase.


**To make a constructive proposal**:

I think that using EXPECT improves debuggability in case of a test
failure, but both with EXPECT and with ASSERT, the tests will give the
same guarantee that the code works, so I feel that this should not be
blocking the truncate patch... so I'd just go and convert it all to
ASSERTs, it'll be consistent with the surrounding tests, and we can
have that EXPECT/ASSERT discussion separately if you like. Does that
sound good?

>
>
> > +	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rwt, O_RDONLY));
> > +	EXPECT_EQ(0, test_truncate(file_rwt));
> > +	EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));
> > +	EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
> > +
> > +	/* Checks read and write rights: no truncate variant works. */
> > +	EXPECT_EQ(EACCES, test_ftruncate(_metadata, file_rw, O_WRONLY));
> > +	EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rw, O_RDONLY));
> > +	EXPECT_EQ(EACCES, test_truncate(file_rw));
> > +	EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC));
> > +
> > +	/*
> > +	 * Checks read and truncate rights: truncation works.
> > +	 *
> > +	 * Note: Files opened in O_RDONLY can get truncated as part of
> > +	 * the same operation.
> > +	 */
> > +	EXPECT_EQ(0, test_open(file_rt, O_RDONLY));
> > +	EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY));
> > +	EXPECT_EQ(0, test_truncate(file_rt));
> > +
> > +	/* Checks truncate right: truncate works, but can't open file. */
> > +	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(0, test_truncate(file_t));
> > +
> > +	/* Checks "no rights" case: No form of truncation works. */
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_truncate(file_none));
> > +
> > +	/* Checks truncate right on directory: truncate works on contained files */
> > +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(0, test_truncate(file_in_dir_t));
> > +
> > +	/*
> > +	 * Checks creat in dir_w: This requires the truncate right
> > +	 * when overwriting an existing file, but does not require it
> > +	 * when the file is new.
> > +	 */
> > +	EXPECT_EQ(EACCES, test_creat(_metadata, file_in_dir_w, 0600));
> > +
> > +	ASSERT_EQ(0, unlink(file_in_dir_w));
> > +	EXPECT_EQ(0, test_creat(_metadata, file_in_dir_w, 0600));
> > +}
> > +
> > +/*
> > + * Exercises file truncation when it's not restricted,
> > + * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed.
> > + */
> > +TEST_F_FORK(layout1, truncate_unhandled)
> > +{
> > +	const char *const file_r = file1_s1d1;
> > +	const char *const file_w = file2_s1d1;
> > +	const char *const file_none = file1_s1d2;
> > +	const struct rule rules[] = {
> > +		{
> > +			.path = file_r,
> > +			.access = LANDLOCK_ACCESS_FS_READ_FILE,
> > +		},
> > +		{
> > +			.path = file_w,
> > +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> > +		},
> > +		// Implicitly: No rights for file_none.
> > +		{},
> > +	};
> > +	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
> > +			      LANDLOCK_ACCESS_FS_WRITE_FILE;
> > +	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> > +
> > +	ASSERT_LE(0, ruleset_fd);
> > +	enforce_ruleset(_metadata, ruleset_fd);
> > +	ASSERT_EQ(0, close(ruleset_fd));
> > +
> > +	/* Checks read right: truncation should work through truncate and open. */
> > +	EXPECT_EQ(0, test_truncate(file_r));
> > +	EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC));
> > +
> > +	/* Checks write right: truncation should work through truncate, ftruncate and open. */
> > +	EXPECT_EQ(0, test_truncate(file_w));
> > +	EXPECT_EQ(0, test_ftruncate(_metadata, file_w, O_WRONLY));
> > +	EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC));
> > +
> > +	/* Checks "no rights" case: truncate works but all open attempts fail. */
> > +	EXPECT_EQ(0, test_truncate(file_none));
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
> > +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY));
> > +}
>
> These tests looks good!

Thanks!

>
> > +
> >   /* clang-format off */
> >   FIXTURE(layout1_bind) {};
> >   /* clang-format on */

--

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

[Mickaël Salaün]

On 13/08/2022 12:07, Günther Noack wrote:
> On Thu, Aug 11, 2022 at 06:59:38PM +0200, Mickaël Salaün wrote:
>>
>> On 04/08/2022 21:37, Günther Noack wrote:
>>> These tests exercise the following truncation operations:
>>>
>>> * truncate() (truncate by path)
>>> * ftruncate() (truncate by file descriptor)
>>> * open with the O_TRUNC flag
>>> * special case: creat(), which is open with O_CREAT|O_WRONLY|O_TRUNC.
>>>
>>> in the following scenarios:
>>>
>>> * Files with read, write and truncate rights.
>>> * Files with read and truncate rights.
>>> * Files with the truncate right.
>>> * Files without the truncate right.
>>>
>>> In particular, the following scenarios are enforced with the test:
>>>
>>> * The truncate right is required to use ftruncate,
>>>     even when the thread already has the right to write to the file.
>>> * open() with O_TRUNC requires the truncate right, if it truncates a file.
>>>     open() already checks security_path_truncate() in this case,
>>>     and it required no additional check in the Landlock LSM's file_open hook.
>>> * creat() requires the truncate right
>>>     when called with an existing filename.
>>> * creat() does *not* require the truncate right
>>>     when it's creating a new file.
>>>
>>> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
>>> ---
>>>    tools/testing/selftests/landlock/fs_test.c | 204 +++++++++++++++++++++
>>>    1 file changed, 204 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
>>> index cb77eaa01c91..3e84bae7e83a 100644
>>> --- a/tools/testing/selftests/landlock/fs_test.c
>>> +++ b/tools/testing/selftests/landlock/fs_test.c
>>> @@ -58,6 +58,7 @@ static const char file1_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f1";
>>>    static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2";
>>>    static const char dir_s3d1[] = TMP_DIR "/s3d1";
>>> +static const char file1_s3d1[] = TMP_DIR "/s3d1/f1";
>>>    /* dir_s3d2 is a mount point. */
>>>    static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2";
>>>    static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
>>> @@ -83,6 +84,7 @@ static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
>>>     * │           ├── f1
>>>     * │           └── f2
>>>     * └── s3d1
>>> + *     ├── f1
>>>     *     └── s3d2
>>>     *         └── s3d3
>>>     */
>>> @@ -208,6 +210,7 @@ static void create_layout1(struct __test_metadata *const _metadata)
>>>    	create_file(_metadata, file1_s2d3);
>>>    	create_file(_metadata, file2_s2d3);
>>> +	create_file(_metadata, file1_s3d1);
>>>    	create_directory(_metadata, dir_s3d2);
>>>    	set_cap(_metadata, CAP_SYS_ADMIN);
>>>    	ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700"));
>>> @@ -230,6 +233,7 @@ static void remove_layout1(struct __test_metadata *const _metadata)
>>>    	EXPECT_EQ(0, remove_path(file1_s2d2));
>>>    	EXPECT_EQ(0, remove_path(file1_s2d1));
>>> +	EXPECT_EQ(0, remove_path(file1_s3d1));
>>>    	EXPECT_EQ(0, remove_path(dir_s3d3));
>>>    	set_cap(_metadata, CAP_SYS_ADMIN);
>>>    	umount(dir_s3d2);
>>> @@ -3023,6 +3027,206 @@ TEST_F_FORK(layout1, proc_pipe)
>>>    	ASSERT_EQ(0, close(pipe_fds[1]));
>>>    }
>>> +/* Opens the file, invokes ftruncate(2) and returns the errno or 0. */
>>> +static int test_ftruncate(struct __test_metadata *const _metadata,
>>> +			  const char *const path, int flags)
>>> +{
>>> +	int res, err, fd;
>>> +
>>> +	fd = open(path, flags | O_CLOEXEC);
>>> +	ASSERT_LE(0, fd);
>>> +
>>> +	res = ftruncate(fd, 10);
>>> +	err = errno;
>>> +	ASSERT_EQ(0, close(fd));
>>> +
>>> +	if (res < 0)
>>> +		return err;
>>> +	return 0;
>>> +}
>>> +
>>> +/* Invokes truncate(2) and returns the errno or 0. */
>>> +static int test_truncate(const char *const path)
>>> +{
>>> +	if (truncate(path, 10) < 0)
>>> +		return errno;
>>> +	return 0;
>>> +}
>>> +
>>> +/* Invokes creat(2) and returns the errno or 0. */
>>> +static int test_creat(struct __test_metadata *const _metadata,
>>> +		      const char *const path, mode_t mode)
>>> +{
>>> +	int fd = creat(path, mode);
>>> +
>>> +	if (fd < 0)
>>> +		return errno;
>>> +	ASSERT_EQ(0, close(fd));
>>
>> test_creat() contains an ASSERT, which would only print this line, which
>> would not help much if it is called multiple times, which is the case. I
>> prefer not passing _metadata but only returning errno or 0 and handling the
>> ASSERT in layout1.truncate* . It is not the case everywhere but we should
>> try to follow this rule as much as possible.
> 
> Thanks, that's a good point that the line number attribution becomes confusing.
> 
> I changed these ASSERT_EQ() calls to just return the errno directly.
> 
> 
>>> +	return 0;
>>> +}
>>> +
>>> +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;
>>> +	const struct rule rules[] = {
>>> +		{
>>> +			.path = file_rwt,
>>> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
>>> +				  LANDLOCK_ACCESS_FS_WRITE_FILE |
>>> +				  LANDLOCK_ACCESS_FS_TRUNCATE,
>>> +		},
>>> +		{
>>> +			.path = file_rw,
>>> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
>>> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
>>> +		},
>>> +		{
>>> +			.path = file_rt,
>>> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
>>> +				  LANDLOCK_ACCESS_FS_TRUNCATE,
>>> +		},
>>> +		{
>>> +			.path = file_t,
>>> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
>>> +		},
>>> +		// Implicitly: No access rights for file_none.
>>> +		{
>>> +			.path = dir_t,
>>> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
>>> +		},
>>> +		{
>>> +			.path = dir_w,
>>> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
>>> +		},
>>> +		{},
>>> +	};
>>> +	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
>>> +			      LANDLOCK_ACCESS_FS_WRITE_FILE |
>>> +			      LANDLOCK_ACCESS_FS_TRUNCATE;
>>> +	const int ruleset_fd = create_ruleset(_metadata, handled, rules);
>>> +
>>> +	ASSERT_LE(0, ruleset_fd);
>>> +	enforce_ruleset(_metadata, ruleset_fd);
>>> +	ASSERT_EQ(0, close(ruleset_fd));
>>> +
>>> +	/*
>>> +	 * Checks read, write and truncate rights: truncation works.
>>> +	 *
>>> +	 * Note: Independent of Landlock, ftruncate(2) on read-only
>>> +	 * file descriptors never works.
>>> +	 */
>>> +	EXPECT_EQ(0, test_ftruncate(_metadata, file_rwt, O_WRONLY));
>>
>> Other than following the original Google Test documentation, what is the
>> advantage to use EXPECT? Don't you think it would be harder to debug a bunch
>> of failed expect?
> 
> <This is maybe slightly philosophical, which means that I have no hard
> proof for any of this, but I'm happy to discuss it :)>
> 
> I think at the root, most test frameworks agree that the test output
> should ideally be a list of mostly-independent test scenarios together
> with their failure statuses.
> 
> I think it is actually *easier* to debug a bunch of failed
> expectations if they fail at the same time: When multiple of these
> scenarios fail at the same time, that would give you a hint that
> something more fundamental is broken, whereas when only one of them
> fails, you'd know that it is a problem specific to the test scenario
> at hand.
> 
>> What is the reason for other test frameworks to not
>> implement EXPECT?
> 
> The way that the different test frameworks try to achieve this "table
> of scenarios" output is different:
> 
> The JUnit/xUnit-style frameworks only have an ASSERT-equivalent, and I
> think in these testing cultures there is a greater emphasis on having
> a separate test function for each exercised scenario. That way, you
> still end up with a long table of test scenarios with their failure
> statuses. -- But that is not the test structure that we have here in
> the Landlock selftests.
> 
> In Googletest, or in the Go test framework, there is a distinction
> between ASSERT and EXPECT, and people are more encouraged to test
> multiple similar scenarios in the same function (in Go, they call it a
> "table driven test"). It works as well, as long as people take care
> that the scenarios that are tested together don't accidentally depend
> on each others left-over state.
> 
> I'm not sure why some test frameworks do this with assert-only and
> others distinguish between assert and expect.
> 
> I *suspect* that in C and C++, it is more difficult to factor out
> shared test setup code than in Java or Python, and so people tend to
> write bigger test functions that exercise more scenarios at once, and
> then the distinction between assert and expect became more useful.
> 
> <end of philosophical derail>
> 
> 
>> How Chromium or other Google code really use it? Genuine questions.
> 
> This is how Chromium uses it:
> 
> https://source.chromium.org/search?q=ASSERT_EQ (~6000 hits)
> https://source.chromium.org/search?q=EXPECT_EQ (~5800 hits)
> 
> This is how Absl uses it (from the website: "Abseil is an open source
> collection of C++ libraries drawn from the most fundamental pieces of
> Google’s internal codebase."):
> 
> https://github.com/abseil/abseil-cpp/search?q=EXPECT_EQ (163 results)
> https://github.com/abseil/abseil-cpp/search?q=ASSERT_EQ (24 results)
> 
> In Chromium, there are clearly some corners where they use ASSERT
> exclusively. In Abseil, I think it gets used more in the spirit of the
> framework, but it's also a smaller codebase.

Thanks for these explanations!

> 
> 
> **To make a constructive proposal**:
> 
> I think that using EXPECT improves debuggability in case of a test
> failure, but both with EXPECT and with ASSERT, the tests will give the
> same guarantee that the code works, so I feel that this should not be
> blocking the truncate patch... so I'd just go and convert it all to
> ASSERTs, it'll be consistent with the surrounding tests, and we can
> have that EXPECT/ASSERT discussion separately if you like. Does that
> sound good?

You did the work to differentiate EXPECT from ASSERT, and as long as 
failing an EXPECT doesn't change the semantic of the next tests (i.e. 
not changing a common state, e.g. by creating or removing a file), I 
think we should keep your current code. This may be tricky to do 
correctly, hence my reluctance. I'll think a bit more about that but it 
will be much more easier to replace EXPECT with ASSERT than to re-add 
EXPECT, and it wouldn't require another patch series, so let's not 
change your patch. :)

Re: [PATCH v3 4/4] landlock: Document Landlock's file truncation support

[Günther Noack]

On Fri, Aug 12, 2022 at 01:19:47PM +0200, Mickaël Salaün wrote:
>
> On 04/08/2022 21:37, Günther Noack wrote:
> > Use the LANDLOCK_ACCESS_FS_TRUNCATE flag in the tutorial.
> >
> > Adapt the backwards compatibility example and discussion to remove the
> > truncation flag where needed.
> >
> > Point out potential surprising behaviour related to truncate.
> >
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >   Documentation/userspace-api/landlock.rst | 41 ++++++++++++++++++++----
> >   1 file changed, 34 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> > index d92e335380d4..9c3c9fa65958 100644
> > --- a/Documentation/userspace-api/landlock.rst
> > +++ b/Documentation/userspace-api/landlock.rst
> > @@ -60,7 +60,8 @@ the need to be explicit about the denied-by-default access rights.
> >               LANDLOCK_ACCESS_FS_MAKE_FIFO |
> >               LANDLOCK_ACCESS_FS_MAKE_BLOCK |
> >               LANDLOCK_ACCESS_FS_MAKE_SYM |
> > -            LANDLOCK_ACCESS_FS_REFER,
> > +            LANDLOCK_ACCESS_FS_REFER |
> > +            LANDLOCK_ACCESS_FS_TRUNCATE,
> >       };
> >   Because we may not know on which kernel version an application will be
> > @@ -69,16 +70,24 @@ should try to protect users as much as possible whatever the kernel they are
> >   using.  To avoid binary enforcement (i.e. either all security features or
> >   none), we can leverage a dedicated Landlock command to get the current version
> >   of the Landlock ABI and adapt the handled accesses.  Let's check if we should
> > -remove the `LANDLOCK_ACCESS_FS_REFER` access right which is only supported
> > -starting with the second version of the ABI.
> > +remove the `LANDLOCK_ACCESS_FS_REFER` and `LANDLOCK_ACCESS_FS_TRUNCATE` access
> > +rights, which are only supported starting with the second and third version of
> > +the ABI.
> >   .. code-block:: c
> >       int abi;
> >       abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION);
> > -    if (abi < 2) {
> > -        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
> > +    switch (abi) {
> > +    case -1:
> > +            perror("Landlock is not supported with the running kernel");
>
> Because there is a distinction between "supported" and "enabled" (as
> explained in the sample), let's make this message more generic. The
> additional strerror() output would then be enough to distinguish the error
> type.
>
> "The running kernel does not enable to use Landlock"

Done, good point.

>
> > +            return 1;
> > +    case 1:
>
> This switch/case logic might be a bit confusing; let's explain it for this
> doc *and the sample code*:
>
> /* Removes LANDLOCK_ACCESS_FS_REFER for ABI < 2 */

Done, added them to both the sample and the documentation.

> > +            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
> > +            __attribute__((fallthrough));
> > +    case 2:
>
> /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
>
> > +            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
> >       }
> >   This enables to create an inclusive ruleset that will contain our rules.
> > @@ -127,8 +136,8 @@ descriptor.
> >   It may also be required to create rules following the same logic as explained
> >   for the ruleset creation, by filtering access rights according to the Landlock
> > -ABI version.  In this example, this is not required because
> > -`LANDLOCK_ACCESS_FS_REFER` is not allowed by any rule.
> > +ABI version.  In this example, this is not required because all of the requested
> > +``allowed_access`` rights are already available in ABI 1.
> >   We now have a ruleset with one rule allowing read access to ``/usr`` while
> >   denying all other handled accesses for the filesystem.  The next step is to
> > @@ -251,6 +260,24 @@ To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target
> >   process, a sandboxed process should have a subset of the target process rules,
> >   which means the tracee must be in a sub-domain of the tracer.
> > +Truncating files
> > +----------------
> > +
> > +The operations covered by `LANDLOCK_ACCESS_FS_WRITE_FILE` and
> > +`LANDLOCK_ACCESS_FS_TRUNCATE` both change the contents of a file and
> > +sometimes overlap in non-intuitive ways. It is recommended to always
> > +specify both of these together.
> > +
> > +A particularly surprising example is :manpage:`creat(2)`. The name
> > +suggests that this system call requires the rights to create and write
> > +files. However, it also requires the truncate right if an existing
> > +file under the same name is already present.
> > +
> > +It should also be noted that truncating files does not necessarily
>
> FYI, I'll send a standalone patch to remove all contractions and get a more
> consistent documentation. Please, keep it this way.

Acknowledged.

>
>
> > +require the `LANDLOCK_ACCESS_FS_WRITE_FILE` right. Apart from the
> > +obvious :manpage:`truncate(2)` system call, this can also be done
> > +through :manpage:`open(2)` with the flags `O_RDONLY` and `O_TRUNC`.
>
> Good!
>
> nit: you can use a 80-columns limit.

Sounds good, applied that in this place and a few others that I touched.

>
> > +
> >   Compatibility
> >   =============

--

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

[Günther Noack]

On Sat, Aug 13, 2022 at 02:45:12PM +0200, Mickaël Salaün wrote:
> On 13/08/2022 12:07, Günther Noack wrote:
> > On Thu, Aug 11, 2022 at 06:59:38PM +0200, Mickaël Salaün wrote:
> > > On 04/08/2022 21:37, Günther Noack wrote:
> > > > +/* Invokes creat(2) and returns the errno or 0. */
> > > > +static int test_creat(struct __test_metadata *const _metadata,
> > > > +		      const char *const path, mode_t mode)
> > > > +{
> > > > +	int fd = creat(path, mode);
> > > > +
> > > > +	if (fd < 0)
> > > > +		return errno;
> > > > +	ASSERT_EQ(0, close(fd));
> > >
> > > test_creat() contains an ASSERT, which would only print this line, which
> > > would not help much if it is called multiple times, which is the case. I
> > > prefer not passing _metadata but only returning errno or 0 and handling the
> > > ASSERT in layout1.truncate* . It is not the case everywhere but we should
> > > try to follow this rule as much as possible.
> >
> > Thanks, that's a good point that the line number attribution becomes confusing.
> >
> > I changed these ASSERT_EQ() calls to just return the errno directly.

Brief follow up on this; I changed it to return a special error EBADFD
in the places in the test_foo() helpers where I previously used
ASSERT_EQ. The errors checked there are errors from open() and close()
in cases where they are needed as prerequisites or cleanups to the
actual thing we want to test.

Specifically open() can also return EACCES due to Landlock policies,
and I don't want to confuse that with the EACCES that ftruncate() may
return.

I use EBADFD because it's a reasonably obscure error code and should
not overlap. (Suggestions welcome)

> > **To make a constructive proposal**:
> >
> > I think that using EXPECT improves debuggability in case of a test
> > failure, but both with EXPECT and with ASSERT, the tests will give the
> > same guarantee that the code works, so I feel that this should not be
> > blocking the truncate patch... so I'd just go and convert it all to
> > ASSERTs, it'll be consistent with the surrounding tests, and we can
> > have that EXPECT/ASSERT discussion separately if you like. Does that
> > sound good?
>
> You did the work to differentiate EXPECT from ASSERT, and as long as failing
> an EXPECT doesn't change the semantic of the next tests (i.e. not changing a
> common state, e.g. by creating or removing a file), I think we should keep
> your current code. This may be tricky to do correctly, hence my reluctance.
> I'll think a bit more about that but it will be much more easier to replace
> EXPECT with ASSERT than to re-add EXPECT, and it wouldn't require another
> patch series, so let's not change your patch. :)

Ack, thanks :)

--