[PATCH RFC v2 0/4] ovl: specify layers via file descriptors

[PATCH RFC v2 0/4] ovl: specify layers via file descriptors

[Christian Brauner]

Hey,

Currently overlayfs only allows specifying layers through path names.
This is inconvenient for users such as systemd that want to assemble an
overlayfs mount purely based on file descriptors.

When porting overlayfs to the new mount api I already mentioned this.
This enables user to specify both:

     fsconfig(fd_overlay, FSCONFIG_SET_FD, "upperdir+", NULL, fd_upper);
     fsconfig(fd_overlay, FSCONFIG_SET_FD, "workdir+",  NULL, fd_work);
     fsconfig(fd_overlay, FSCONFIG_SET_FD, "lowerdir+", NULL, fd_lower1);
     fsconfig(fd_overlay, FSCONFIG_SET_FD, "lowerdir+", NULL, fd_lower2);

in addition to:

     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "upperdir+", "/upper",  0);
     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "workdir+",  "/work",   0);
     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "lowerdir+", "/lower1", 0);
     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "lowerdir+", "/lower2", 0);

The selftest contains an example for this.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
Changes in v2:
- Alias fd and path based mount options.
- Link to v1: https://lore.kernel.org/r/20241011-work-overlayfs-v1-0-e34243841279@kernel.org

---
Christian Brauner (4):
      fs: add helper to use mount option as path or fd
      ovl: specify layers via file descriptors
      selftests: use shared header
      selftests: add overlayfs fd mounting selftests

 fs/fs_parser.c                                     |  19 ++++
 fs/overlayfs/params.c                              | 106 +++++++++++++-----
 include/linux/fs_parser.h                          |   5 +-
 .../selftests/filesystems/overlayfs/.gitignore     |   1 +
 .../selftests/filesystems/overlayfs/Makefile       |   2 +-
 .../selftests/filesystems/overlayfs/dev_in_maps.c  |  27 +----
 .../filesystems/overlayfs/set_layers_via_fds.c     | 122 +++++++++++++++++++++
 .../selftests/filesystems/overlayfs/wrappers.h     |  47 ++++++++
 8 files changed, 274 insertions(+), 55 deletions(-)
---
base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b
change-id: 20241011-work-overlayfs-dbcfa9223e87

[PATCH RFC v2 1/4] fs: add helper to use mount option as path or fd

[Christian Brauner]

Allow filesystems to use a mount option either as a
path or a file descriptor.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/fs_parser.c            | 19 +++++++++++++++++++
 include/linux/fs_parser.h |  5 ++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index 24727ec34e5aa434364e87879cccf9fe1ec19d37..a017415d8d6bc91608ece5d42fa4bea26e47456b 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -308,6 +308,25 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p,
 }
 EXPORT_SYMBOL(fs_param_is_fd);
 
+int fs_param_is_fd_or_path(struct p_log *log, const struct fs_parameter_spec *p,
+			   struct fs_parameter *param,
+			   struct fs_parse_result *result)
+{
+	switch (param->type) {
+	case fs_value_is_string:
+		return fs_param_is_string(log, p, param, result);
+	case fs_value_is_file:
+		result->uint_32 = param->dirfd;
+		if (result->uint_32 <= INT_MAX)
+			return 0;
+		break;
+	default:
+		break;
+	}
+	return fs_param_bad_value(log, param);
+}
+EXPORT_SYMBOL(fs_param_is_fd_or_path);
+
 int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p,
 		    struct fs_parameter *param, struct fs_parse_result *result)
 {
diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index 6cf713a7e6c6fc2402a68c87036264eaed921432..73fe4e119ee24b3bed1f0cf2bc23d6b31811cb69 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -28,7 +28,8 @@ typedef int fs_param_type(struct p_log *,
  */
 fs_param_type fs_param_is_bool, fs_param_is_u32, fs_param_is_s32, fs_param_is_u64,
 	fs_param_is_enum, fs_param_is_string, fs_param_is_blob, fs_param_is_blockdev,
-	fs_param_is_path, fs_param_is_fd, fs_param_is_uid, fs_param_is_gid;
+	fs_param_is_path, fs_param_is_fd, fs_param_is_uid, fs_param_is_gid,
+	fs_param_is_fd_or_path;
 
 /*
  * Specification of the type of value a parameter wants.
@@ -133,6 +134,8 @@ static inline bool fs_validate_description(const char *name,
 #define fsparam_bdev(NAME, OPT)	__fsparam(fs_param_is_blockdev, NAME, OPT, 0, NULL)
 #define fsparam_path(NAME, OPT)	__fsparam(fs_param_is_path, NAME, OPT, 0, NULL)
 #define fsparam_fd(NAME, OPT)	__fsparam(fs_param_is_fd, NAME, OPT, 0, NULL)
+#define fsparam_fd_or_path(NAME, OPT) \
+				__fsparam(fs_param_is_fd_or_path, NAME, OPT, 0, NULL)
 #define fsparam_uid(NAME, OPT) __fsparam(fs_param_is_uid, NAME, OPT, 0, NULL)
 #define fsparam_gid(NAME, OPT) __fsparam(fs_param_is_gid, NAME, OPT, 0, NULL)
 

-- 
2.45.2

[PATCH RFC v2 2/4] ovl: specify layers via file descriptors

[Christian Brauner]

Currently overlayfs only allows specifying layers through path names.
This is inconvenient for users such as systemd that want to assemble an
overlayfs mount purely based on file descriptors.

This enables user to specify both:

    fsconfig(fd_overlay, FSCONFIG_SET_FD, "upperdir+", NULL, fd_upper);
    fsconfig(fd_overlay, FSCONFIG_SET_FD, "workdir+",  NULL, fd_work);
    fsconfig(fd_overlay, FSCONFIG_SET_FD, "lowerdir+", NULL, fd_lower1);
    fsconfig(fd_overlay, FSCONFIG_SET_FD, "lowerdir+", NULL, fd_lower2);

in addition to:

    fsconfig(fd_overlay, FSCONFIG_SET_STRING, "upperdir+", "/upper",  0);
    fsconfig(fd_overlay, FSCONFIG_SET_STRING, "workdir+",  "/work",   0);
    fsconfig(fd_overlay, FSCONFIG_SET_STRING, "lowerdir+", "/lower1", 0);
    fsconfig(fd_overlay, FSCONFIG_SET_STRING, "lowerdir+", "/lower2", 0);

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/overlayfs/params.c | 106 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 79 insertions(+), 27 deletions(-)

diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index e42546c6c5dfbea930414856d791e3e4424a999e..2d5a072b8683ce94b6cec4b75ce5ddd6d6db8dc6 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -141,10 +141,10 @@ static int ovl_verity_mode_def(void)
 
 const struct fs_parameter_spec ovl_parameter_spec[] = {
 	fsparam_string_empty("lowerdir",    Opt_lowerdir),
-	fsparam_string("lowerdir+",         Opt_lowerdir_add),
-	fsparam_string("datadir+",          Opt_datadir_add),
-	fsparam_string("upperdir",          Opt_upperdir),
-	fsparam_string("workdir",           Opt_workdir),
+	fsparam_fd_or_path("lowerdir+",     Opt_lowerdir_add),
+	fsparam_fd_or_path("datadir+",      Opt_datadir_add),
+	fsparam_fd_or_path("upperdir",      Opt_upperdir),
+	fsparam_fd_or_path("workdir",       Opt_workdir),
 	fsparam_flag("default_permissions", Opt_default_permissions),
 	fsparam_enum("redirect_dir",        Opt_redirect_dir, ovl_parameter_redirect_dir),
 	fsparam_enum("index",               Opt_index, ovl_parameter_bool),
@@ -367,43 +367,89 @@ static void ovl_add_layer(struct fs_context *fc, enum ovl_opt layer,
 	}
 }
 
-static int ovl_parse_layer(struct fs_context *fc, const char *layer_name, enum ovl_opt layer)
+static inline bool is_upper_layer(enum ovl_opt layer)
 {
-	char *name = kstrdup(layer_name, GFP_KERNEL);
-	bool upper = (layer == Opt_upperdir || layer == Opt_workdir);
-	struct path path;
-	int err;
+	return layer == Opt_upperdir || layer == Opt_workdir;
+}
+
+/* Handle non-file descriptor-based layer options that require path lookup. */
+static inline int ovl_kern_path(const char *layer_name, struct path *layer_path,
+				enum ovl_opt layer)
+{
+	switch (layer) {
+	case Opt_upperdir:
+		fallthrough;
+	case Opt_workdir:
+		fallthrough;
+	case Opt_lowerdir:
+		return ovl_mount_dir(layer_name, layer_path);
+	case Opt_lowerdir_add:
+		fallthrough;
+	case Opt_datadir_add:
+		return ovl_mount_dir_noesc(layer_name, layer_path);
+	default:
+		WARN_ON_ONCE(true);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ovl_do_parse_layer(struct fs_context *fc, const char *layer_name,
+			      struct path *layer_path, enum ovl_opt layer)
+{
+	char *name __free(kfree) = kstrdup(layer_name, GFP_KERNEL);
+	bool upper;
+	int err = 0;
 
 	if (!name)
 		return -ENOMEM;
 
-	if (upper || layer == Opt_lowerdir)
-		err = ovl_mount_dir(name, &path);
-	else
-		err = ovl_mount_dir_noesc(name, &path);
+	upper = is_upper_layer(layer);
+	err = ovl_mount_dir_check(fc, layer_path, layer, name, upper);
 	if (err)
-		goto out_free;
-
-	err = ovl_mount_dir_check(fc, &path, layer, name, upper);
-	if (err)
-		goto out_put;
+		return err;
 
 	if (!upper) {
 		err = ovl_ctx_realloc_lower(fc);
 		if (err)
-			goto out_put;
+			return err;
 	}
 
 	/* Store the user provided path string in ctx to show in mountinfo */
-	ovl_add_layer(fc, layer, &path, &name);
-
-out_put:
-	path_put(&path);
-out_free:
-	kfree(name);
+	ovl_add_layer(fc, layer, layer_path, &name);
 	return err;
 }
 
+static int ovl_parse_layer(struct fs_context *fc, struct fs_parameter *param,
+			   enum ovl_opt layer)
+{
+	struct path path __free(path_put) = {};
+	char *buf __free(kfree) = NULL;
+	char *layer_name;
+	int err = 0;
+
+	if (param->type == fs_value_is_file) {
+		buf = kmalloc(PATH_MAX, GFP_KERNEL_ACCOUNT);
+		if (!buf)
+			return -ENOMEM;
+
+		path = param->file->f_path;
+		path_get(&path);
+
+		layer_name = d_path(&path, buf, PATH_MAX);
+		if (IS_ERR(layer_name))
+			return PTR_ERR(layer_name);
+	} else {
+		layer_name = param->string;
+		err = ovl_kern_path(layer_name, &path, layer);
+	}
+	if (err)
+		return err;
+
+	return ovl_do_parse_layer(fc, layer_name, &path, layer);
+}
+
 static void ovl_reset_lowerdirs(struct ovl_fs_context *ctx)
 {
 	struct ovl_fs_context_layer *l = ctx->lower;
@@ -474,7 +520,13 @@ static int ovl_parse_param_lowerdir(const char *name, struct fs_context *fc)
 
 	iter = dup;
 	for (nr = 0; nr < nr_lower; nr++) {
-		err = ovl_parse_layer(fc, iter, Opt_lowerdir);
+		struct path path __free(path_put) = {};
+
+		err = ovl_kern_path(iter, &path, Opt_lowerdir);
+		if (err)
+			goto out_err;
+
+		err = ovl_do_parse_layer(fc, iter, &path, Opt_lowerdir);
 		if (err)
 			goto out_err;
 
@@ -555,7 +607,7 @@ static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	case Opt_datadir_add:
 	case Opt_upperdir:
 	case Opt_workdir:
-		err = ovl_parse_layer(fc, param->string, opt);
+		err = ovl_parse_layer(fc, param, opt);
 		break;
 	case Opt_default_permissions:
 		config->default_permissions = true;

-- 
2.45.2

[PATCH RFC v2 3/4] selftests: use shared header

[Christian Brauner]

So that we don't have to redefine the same system calls over and over.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 .../selftests/filesystems/overlayfs/dev_in_maps.c  | 27 +-------------
 .../selftests/filesystems/overlayfs/wrappers.h     | 43 ++++++++++++++++++++++
 2 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/filesystems/overlayfs/dev_in_maps.c b/tools/testing/selftests/filesystems/overlayfs/dev_in_maps.c
index 2862aae58b79acbe175ab6b36b42798bb99a2225..3b796264223f81fc753d0adaeccc04077023520b 100644
--- a/tools/testing/selftests/filesystems/overlayfs/dev_in_maps.c
+++ b/tools/testing/selftests/filesystems/overlayfs/dev_in_maps.c
@@ -17,32 +17,7 @@
 
 #include "../../kselftest.h"
 #include "log.h"
-
-static int sys_fsopen(const char *fsname, unsigned int flags)
-{
-	return syscall(__NR_fsopen, fsname, flags);
-}
-
-static int sys_fsconfig(int fd, unsigned int cmd, const char *key, const char *value, int aux)
-{
-	return syscall(__NR_fsconfig, fd, cmd, key, value, aux);
-}
-
-static int sys_fsmount(int fd, unsigned int flags, unsigned int attr_flags)
-{
-	return syscall(__NR_fsmount, fd, flags, attr_flags);
-}
-static int sys_mount(const char *src, const char *tgt, const char *fst,
-		unsigned long flags, const void *data)
-{
-	return syscall(__NR_mount, src, tgt, fst, flags, data);
-}
-static int sys_move_mount(int from_dfd, const char *from_pathname,
-			  int to_dfd, const char *to_pathname,
-			  unsigned int flags)
-{
-	return syscall(__NR_move_mount, from_dfd, from_pathname, to_dfd, to_pathname, flags);
-}
+#include "wrappers.h"
 
 static long get_file_dev_and_inode(void *addr, struct statx *stx)
 {
diff --git a/tools/testing/selftests/filesystems/overlayfs/wrappers.h b/tools/testing/selftests/filesystems/overlayfs/wrappers.h
new file mode 100644
index 0000000000000000000000000000000000000000..4f99e10f7f018fd9a7be5263f68d34807da4c53c
--- /dev/null
+++ b/tools/testing/selftests/filesystems/overlayfs/wrappers.h
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+#ifndef __SELFTEST_OVERLAYFS_WRAPPERS_H__
+#define __SELFTEST_OVERLAYFS_WRAPPERS_H__
+
+#define _GNU_SOURCE
+
+#include <linux/types.h>
+#include <linux/mount.h>
+#include <sys/syscall.h>
+
+static inline int sys_fsopen(const char *fsname, unsigned int flags)
+{
+	return syscall(__NR_fsopen, fsname, flags);
+}
+
+static inline int sys_fsconfig(int fd, unsigned int cmd, const char *key,
+			       const char *value, int aux)
+{
+	return syscall(__NR_fsconfig, fd, cmd, key, value, aux);
+}
+
+static inline int sys_fsmount(int fd, unsigned int flags,
+			      unsigned int attr_flags)
+{
+	return syscall(__NR_fsmount, fd, flags, attr_flags);
+}
+
+static inline int sys_mount(const char *src, const char *tgt, const char *fst,
+			    unsigned long flags, const void *data)
+{
+	return syscall(__NR_mount, src, tgt, fst, flags, data);
+}
+
+static inline int sys_move_mount(int from_dfd, const char *from_pathname,
+				 int to_dfd, const char *to_pathname,
+				 unsigned int flags)
+{
+	return syscall(__NR_move_mount, from_dfd, from_pathname, to_dfd,
+		       to_pathname, flags);
+}
+
+#endif

-- 
2.45.2

[PATCH RFC v2 4/4] selftests: add overlayfs fd mounting selftests

[Christian Brauner]

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 .../selftests/filesystems/overlayfs/.gitignore     |   1 +
 .../selftests/filesystems/overlayfs/Makefile       |   2 +-
 .../filesystems/overlayfs/set_layers_via_fds.c     | 122 +++++++++++++++++++++
 .../selftests/filesystems/overlayfs/wrappers.h     |   4 +
 4 files changed, 128 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/filesystems/overlayfs/.gitignore b/tools/testing/selftests/filesystems/overlayfs/.gitignore
index 52ae618fdd980ee22424d35d79f077077b132401..e23a18c8b37f2cdbb121496b1df1faffd729ad79 100644
--- a/tools/testing/selftests/filesystems/overlayfs/.gitignore
+++ b/tools/testing/selftests/filesystems/overlayfs/.gitignore
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 dev_in_maps
+set_layers_via_fds
diff --git a/tools/testing/selftests/filesystems/overlayfs/Makefile b/tools/testing/selftests/filesystems/overlayfs/Makefile
index 56b2b48a765b1d6706faee14616597ed0315f267..e8d1adb021af44588dd7af1049de66833bb584ce 100644
--- a/tools/testing/selftests/filesystems/overlayfs/Makefile
+++ b/tools/testing/selftests/filesystems/overlayfs/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
-TEST_GEN_PROGS := dev_in_maps
+TEST_GEN_PROGS := dev_in_maps set_layers_via_fds
 
 CFLAGS := -Wall -Werror
 
diff --git a/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c b/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c
new file mode 100644
index 0000000000000000000000000000000000000000..d3b497eea5e5c9f718caa4957f7fec7c40970502
--- /dev/null
+++ b/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c
@@ -0,0 +1,122 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#define __SANE_USERSPACE_TYPES__ // Use ll64
+
+#include <fcntl.h>
+#include <sched.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/mount.h>
+#include <unistd.h>
+
+#include "../../kselftest_harness.h"
+#include "log.h"
+#include "wrappers.h"
+
+FIXTURE(set_layers_via_fds) {
+};
+
+FIXTURE_SETUP(set_layers_via_fds)
+{
+	ASSERT_EQ(mkdir("/set_layers_via_fds", 0755), 0);
+}
+
+FIXTURE_TEARDOWN(set_layers_via_fds)
+{
+	umount2("/set_layers_via_fds", 0);
+	ASSERT_EQ(rmdir("/set_layers_via_fds"), 0);
+}
+
+TEST_F(set_layers_via_fds, set_layers_via_fds)
+{
+	int fd_context, fd_tmpfs, fd_overlay;
+	int layer_fds[5] = { -EBADF, -EBADF, -EBADF, -EBADF, -EBADF };
+	bool layers_found[5] = { false, false, false, false, false };
+	size_t len = 0;
+	char *line = NULL;
+	FILE *f_mountinfo;
+
+	ASSERT_EQ(unshare(CLONE_NEWNS), 0);
+	ASSERT_EQ(sys_mount(NULL, "/", NULL, MS_SLAVE | MS_REC, NULL), 0);
+
+	fd_context = sys_fsopen("tmpfs", 0);
+	ASSERT_GE(fd_context, 0);
+
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_CMD_CREATE, NULL, NULL, 0), 0);
+	fd_tmpfs = sys_fsmount(fd_context, 0, 0);
+	ASSERT_GE(fd_tmpfs, 0);
+	ASSERT_EQ(close(fd_context), 0);
+
+	ASSERT_EQ(mkdirat(fd_tmpfs, "w", 0755), 0);
+	ASSERT_EQ(mkdirat(fd_tmpfs, "u", 0755), 0);
+	ASSERT_EQ(mkdirat(fd_tmpfs, "l1", 0755), 0);
+	ASSERT_EQ(mkdirat(fd_tmpfs, "l2", 0755), 0);
+	ASSERT_EQ(mkdirat(fd_tmpfs, "l3", 0755), 0);
+
+	layer_fds[0] = openat(fd_tmpfs, "w", O_DIRECTORY);
+	ASSERT_GE(layer_fds[0], 0);
+
+	layer_fds[1] = openat(fd_tmpfs, "u", O_DIRECTORY);
+	ASSERT_GE(layer_fds[1], 0);
+
+	layer_fds[2] = openat(fd_tmpfs, "l1", O_DIRECTORY);
+	ASSERT_GE(layer_fds[2], 0);
+
+	layer_fds[3] = openat(fd_tmpfs, "l2", O_DIRECTORY);
+	ASSERT_GE(layer_fds[3], 0);
+
+	layer_fds[4] = openat(fd_tmpfs, "l3", O_DIRECTORY);
+	ASSERT_GE(layer_fds[4], 0);
+
+	ASSERT_EQ(sys_move_mount(fd_tmpfs, "", -EBADF, "/tmp", MOVE_MOUNT_F_EMPTY_PATH), 0);
+	ASSERT_EQ(close(fd_tmpfs), 0);
+
+	fd_context = sys_fsopen("overlay", 0);
+	ASSERT_GE(fd_context, 0);
+
+	ASSERT_NE(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir", NULL, layer_fds[2]), 0);
+
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "workdir", NULL, layer_fds[0]), 0);
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "upperdir", NULL, layer_fds[1]), 0);
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir+", NULL, layer_fds[2]), 0);
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir+", NULL, layer_fds[3]), 0);
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir+", NULL, layer_fds[4]), 0);
+
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_CMD_CREATE, NULL, NULL, 0), 0);
+
+	fd_overlay = sys_fsmount(fd_context, 0, 0);
+	ASSERT_GE(fd_overlay, 0);
+
+	ASSERT_EQ(sys_move_mount(fd_overlay, "", -EBADF, "/set_layers_via_fds", MOVE_MOUNT_F_EMPTY_PATH), 0);
+
+	f_mountinfo = fopen("/proc/self/mountinfo", "r");
+	ASSERT_NE(f_mountinfo, NULL);
+
+	while (getline(&line, &len, f_mountinfo) != -1) {
+		char *haystack = line;
+
+		if (strstr(haystack, "workdir=/tmp/w"))
+			layers_found[0] = true;
+		if (strstr(haystack, "upperdir=/tmp/u"))
+			layers_found[1] = true;
+		if (strstr(haystack, "lowerdir+=/tmp/l1"))
+			layers_found[2] = true;
+		if (strstr(haystack, "lowerdir+=/tmp/l2"))
+			layers_found[3] = true;
+		if (strstr(haystack, "lowerdir+=/tmp/l3"))
+			layers_found[4] = true;
+	}
+	free(line);
+
+	for (int i = 0; i < 5; i++) {
+		ASSERT_EQ(layers_found[i], true);
+		ASSERT_EQ(close(layer_fds[i]), 0);
+	}
+
+	ASSERT_EQ(close(fd_context), 0);
+	ASSERT_EQ(close(fd_overlay), 0);
+	ASSERT_EQ(fclose(f_mountinfo), 0);
+}
+
+TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/filesystems/overlayfs/wrappers.h b/tools/testing/selftests/filesystems/overlayfs/wrappers.h
index 4f99e10f7f018fd9a7be5263f68d34807da4c53c..071b95fd2ac0ad7b02d90e8e89df73fd27be69c3 100644
--- a/tools/testing/selftests/filesystems/overlayfs/wrappers.h
+++ b/tools/testing/selftests/filesystems/overlayfs/wrappers.h
@@ -32,6 +32,10 @@ static inline int sys_mount(const char *src, const char *tgt, const char *fst,
 	return syscall(__NR_mount, src, tgt, fst, flags, data);
 }
 
+#ifndef MOVE_MOUNT_F_EMPTY_PATH
+#define MOVE_MOUNT_F_EMPTY_PATH 0x00000004 /* Empty from path permitted */
+#endif
+
 static inline int sys_move_mount(int from_dfd, const char *from_pathname,
 				 int to_dfd, const char *to_pathname,
 				 unsigned int flags)

-- 
2.45.2

Re: [PATCH RFC v2 2/4] ovl: specify layers via file descriptors

[Al Viro]

On Fri, Oct 11, 2024 at 11:45:51PM +0200, Christian Brauner wrote:

> +static int ovl_parse_layer(struct fs_context *fc, struct fs_parameter *param,
> +			   enum ovl_opt layer)
> +{
> +	struct path path __free(path_put) = {};
> +	char *buf __free(kfree) = NULL;

Move down to the scope where it's used.  And just initialize
with kmalloc().

> +	char *layer_name;
> +	int err = 0;
> +
> +	if (param->type == fs_value_is_file) {
> +		buf = kmalloc(PATH_MAX, GFP_KERNEL_ACCOUNT);
> +		if (!buf)
> +			return -ENOMEM;

Re: [PATCH RFC v2 4/4] selftests: add overlayfs fd mounting selftests

[Amir Goldstein]

On Fri, Oct 11, 2024 at 11:46 PM Christian Brauner <brauner@kernel.org> wrote:
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Very cool!

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  .../selftests/filesystems/overlayfs/.gitignore     |   1 +
>  .../selftests/filesystems/overlayfs/Makefile       |   2 +-
>  .../filesystems/overlayfs/set_layers_via_fds.c     | 122 +++++++++++++++++++++
>  .../selftests/filesystems/overlayfs/wrappers.h     |   4 +
>  4 files changed, 128 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/filesystems/overlayfs/.gitignore b/tools/testing/selftests/filesystems/overlayfs/.gitignore
> index 52ae618fdd980ee22424d35d79f077077b132401..e23a18c8b37f2cdbb121496b1df1faffd729ad79 100644
> --- a/tools/testing/selftests/filesystems/overlayfs/.gitignore
> +++ b/tools/testing/selftests/filesystems/overlayfs/.gitignore
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  dev_in_maps
> +set_layers_via_fds
> diff --git a/tools/testing/selftests/filesystems/overlayfs/Makefile b/tools/testing/selftests/filesystems/overlayfs/Makefile
> index 56b2b48a765b1d6706faee14616597ed0315f267..e8d1adb021af44588dd7af1049de66833bb584ce 100644
> --- a/tools/testing/selftests/filesystems/overlayfs/Makefile
> +++ b/tools/testing/selftests/filesystems/overlayfs/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>
> -TEST_GEN_PROGS := dev_in_maps
> +TEST_GEN_PROGS := dev_in_maps set_layers_via_fds
>
>  CFLAGS := -Wall -Werror
>
> diff --git a/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c b/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..d3b497eea5e5c9f718caa4957f7fec7c40970502
> --- /dev/null
> +++ b/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c
> @@ -0,0 +1,122 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#define __SANE_USERSPACE_TYPES__ // Use ll64
> +
> +#include <fcntl.h>
> +#include <sched.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/mount.h>
> +#include <unistd.h>
> +
> +#include "../../kselftest_harness.h"
> +#include "log.h"
> +#include "wrappers.h"
> +
> +FIXTURE(set_layers_via_fds) {
> +};
> +
> +FIXTURE_SETUP(set_layers_via_fds)
> +{
> +       ASSERT_EQ(mkdir("/set_layers_via_fds", 0755), 0);
> +}
> +
> +FIXTURE_TEARDOWN(set_layers_via_fds)
> +{
> +       umount2("/set_layers_via_fds", 0);
> +       ASSERT_EQ(rmdir("/set_layers_via_fds"), 0);
> +}
> +
> +TEST_F(set_layers_via_fds, set_layers_via_fds)
> +{
> +       int fd_context, fd_tmpfs, fd_overlay;
> +       int layer_fds[5] = { -EBADF, -EBADF, -EBADF, -EBADF, -EBADF };
> +       bool layers_found[5] = { false, false, false, false, false };
> +       size_t len = 0;
> +       char *line = NULL;
> +       FILE *f_mountinfo;
> +
> +       ASSERT_EQ(unshare(CLONE_NEWNS), 0);
> +       ASSERT_EQ(sys_mount(NULL, "/", NULL, MS_SLAVE | MS_REC, NULL), 0);
> +
> +       fd_context = sys_fsopen("tmpfs", 0);
> +       ASSERT_GE(fd_context, 0);
> +
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_CMD_CREATE, NULL, NULL, 0), 0);
> +       fd_tmpfs = sys_fsmount(fd_context, 0, 0);
> +       ASSERT_GE(fd_tmpfs, 0);
> +       ASSERT_EQ(close(fd_context), 0);
> +
> +       ASSERT_EQ(mkdirat(fd_tmpfs, "w", 0755), 0);
> +       ASSERT_EQ(mkdirat(fd_tmpfs, "u", 0755), 0);
> +       ASSERT_EQ(mkdirat(fd_tmpfs, "l1", 0755), 0);
> +       ASSERT_EQ(mkdirat(fd_tmpfs, "l2", 0755), 0);
> +       ASSERT_EQ(mkdirat(fd_tmpfs, "l3", 0755), 0);
> +
> +       layer_fds[0] = openat(fd_tmpfs, "w", O_DIRECTORY);
> +       ASSERT_GE(layer_fds[0], 0);
> +
> +       layer_fds[1] = openat(fd_tmpfs, "u", O_DIRECTORY);
> +       ASSERT_GE(layer_fds[1], 0);
> +
> +       layer_fds[2] = openat(fd_tmpfs, "l1", O_DIRECTORY);
> +       ASSERT_GE(layer_fds[2], 0);
> +
> +       layer_fds[3] = openat(fd_tmpfs, "l2", O_DIRECTORY);
> +       ASSERT_GE(layer_fds[3], 0);
> +
> +       layer_fds[4] = openat(fd_tmpfs, "l3", O_DIRECTORY);
> +       ASSERT_GE(layer_fds[4], 0);
> +
> +       ASSERT_EQ(sys_move_mount(fd_tmpfs, "", -EBADF, "/tmp", MOVE_MOUNT_F_EMPTY_PATH), 0);
> +       ASSERT_EQ(close(fd_tmpfs), 0);
> +
> +       fd_context = sys_fsopen("overlay", 0);
> +       ASSERT_GE(fd_context, 0);
> +
> +       ASSERT_NE(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir", NULL, layer_fds[2]), 0);
> +
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "workdir", NULL, layer_fds[0]), 0);
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "upperdir", NULL, layer_fds[1]), 0);
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir+", NULL, layer_fds[2]), 0);
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir+", NULL, layer_fds[3]), 0);
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir+", NULL, layer_fds[4]), 0);
> +
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_CMD_CREATE, NULL, NULL, 0), 0);
> +
> +       fd_overlay = sys_fsmount(fd_context, 0, 0);
> +       ASSERT_GE(fd_overlay, 0);
> +
> +       ASSERT_EQ(sys_move_mount(fd_overlay, "", -EBADF, "/set_layers_via_fds", MOVE_MOUNT_F_EMPTY_PATH), 0);
> +
> +       f_mountinfo = fopen("/proc/self/mountinfo", "r");
> +       ASSERT_NE(f_mountinfo, NULL);
> +
> +       while (getline(&line, &len, f_mountinfo) != -1) {
> +               char *haystack = line;
> +
> +               if (strstr(haystack, "workdir=/tmp/w"))
> +                       layers_found[0] = true;
> +               if (strstr(haystack, "upperdir=/tmp/u"))
> +                       layers_found[1] = true;
> +               if (strstr(haystack, "lowerdir+=/tmp/l1"))
> +                       layers_found[2] = true;
> +               if (strstr(haystack, "lowerdir+=/tmp/l2"))
> +                       layers_found[3] = true;
> +               if (strstr(haystack, "lowerdir+=/tmp/l3"))
> +                       layers_found[4] = true;
> +       }
> +       free(line);
> +
> +       for (int i = 0; i < 5; i++) {
> +               ASSERT_EQ(layers_found[i], true);
> +               ASSERT_EQ(close(layer_fds[i]), 0);
> +       }
> +
> +       ASSERT_EQ(close(fd_context), 0);
> +       ASSERT_EQ(close(fd_overlay), 0);
> +       ASSERT_EQ(fclose(f_mountinfo), 0);
> +}
> +
> +TEST_HARNESS_MAIN
> diff --git a/tools/testing/selftests/filesystems/overlayfs/wrappers.h b/tools/testing/selftests/filesystems/overlayfs/wrappers.h
> index 4f99e10f7f018fd9a7be5263f68d34807da4c53c..071b95fd2ac0ad7b02d90e8e89df73fd27be69c3 100644
> --- a/tools/testing/selftests/filesystems/overlayfs/wrappers.h
> +++ b/tools/testing/selftests/filesystems/overlayfs/wrappers.h
> @@ -32,6 +32,10 @@ static inline int sys_mount(const char *src, const char *tgt, const char *fst,
>         return syscall(__NR_mount, src, tgt, fst, flags, data);
>  }
>
> +#ifndef MOVE_MOUNT_F_EMPTY_PATH
> +#define MOVE_MOUNT_F_EMPTY_PATH 0x00000004 /* Empty from path permitted */
> +#endif
> +
>  static inline int sys_move_mount(int from_dfd, const char *from_pathname,
>                                  int to_dfd, const char *to_pathname,
>                                  unsigned int flags)
>
> --
> 2.45.2
>

Re: [PATCH RFC v2 1/4] fs: add helper to use mount option as path or fd

[Amir Goldstein]

On Fri, Oct 11, 2024 at 11:46 PM Christian Brauner <brauner@kernel.org> wrote:
>
> Allow filesystems to use a mount option either as a
> path or a file descriptor.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks sane

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/fs_parser.c            | 19 +++++++++++++++++++
>  include/linux/fs_parser.h |  5 ++++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> index 24727ec34e5aa434364e87879cccf9fe1ec19d37..a017415d8d6bc91608ece5d42fa4bea26e47456b 100644
> --- a/fs/fs_parser.c
> +++ b/fs/fs_parser.c
> @@ -308,6 +308,25 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p,
>  }
>  EXPORT_SYMBOL(fs_param_is_fd);
>
> +int fs_param_is_fd_or_path(struct p_log *log, const struct fs_parameter_spec *p,
> +                          struct fs_parameter *param,
> +                          struct fs_parse_result *result)
> +{
> +       switch (param->type) {
> +       case fs_value_is_string:
> +               return fs_param_is_string(log, p, param, result);
> +       case fs_value_is_file:
> +               result->uint_32 = param->dirfd;
> +               if (result->uint_32 <= INT_MAX)
> +                       return 0;
> +               break;
> +       default:
> +               break;
> +       }
> +       return fs_param_bad_value(log, param);
> +}
> +EXPORT_SYMBOL(fs_param_is_fd_or_path);
> +
>  int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p,
>                     struct fs_parameter *param, struct fs_parse_result *result)
>  {
> diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
> index 6cf713a7e6c6fc2402a68c87036264eaed921432..73fe4e119ee24b3bed1f0cf2bc23d6b31811cb69 100644
> --- a/include/linux/fs_parser.h
> +++ b/include/linux/fs_parser.h
> @@ -28,7 +28,8 @@ typedef int fs_param_type(struct p_log *,
>   */
>  fs_param_type fs_param_is_bool, fs_param_is_u32, fs_param_is_s32, fs_param_is_u64,
>         fs_param_is_enum, fs_param_is_string, fs_param_is_blob, fs_param_is_blockdev,
> -       fs_param_is_path, fs_param_is_fd, fs_param_is_uid, fs_param_is_gid;
> +       fs_param_is_path, fs_param_is_fd, fs_param_is_uid, fs_param_is_gid,
> +       fs_param_is_fd_or_path;
>
>  /*
>   * Specification of the type of value a parameter wants.
> @@ -133,6 +134,8 @@ static inline bool fs_validate_description(const char *name,
>  #define fsparam_bdev(NAME, OPT)        __fsparam(fs_param_is_blockdev, NAME, OPT, 0, NULL)
>  #define fsparam_path(NAME, OPT)        __fsparam(fs_param_is_path, NAME, OPT, 0, NULL)
>  #define fsparam_fd(NAME, OPT)  __fsparam(fs_param_is_fd, NAME, OPT, 0, NULL)
> +#define fsparam_fd_or_path(NAME, OPT) \
> +                               __fsparam(fs_param_is_fd_or_path, NAME, OPT, 0, NULL)
>  #define fsparam_uid(NAME, OPT) __fsparam(fs_param_is_uid, NAME, OPT, 0, NULL)
>  #define fsparam_gid(NAME, OPT) __fsparam(fs_param_is_gid, NAME, OPT, 0, NULL)
>
>
> --
> 2.45.2
>

Re: [PATCH RFC v2 3/4] selftests: use shared header

[Amir Goldstein]

On Fri, Oct 11, 2024 at 11:46 PM Christian Brauner <brauner@kernel.org> wrote:
>
> So that we don't have to redefine the same system calls over and over.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Heh, I forgot that this selftest existed, even though I clearly reviewed it
I will even run it from now on :)

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  .../selftests/filesystems/overlayfs/dev_in_maps.c  | 27 +-------------
>  .../selftests/filesystems/overlayfs/wrappers.h     | 43 ++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 26 deletions(-)
>
> diff --git a/tools/testing/selftests/filesystems/overlayfs/dev_in_maps.c b/tools/testing/selftests/filesystems/overlayfs/dev_in_maps.c
> index 2862aae58b79acbe175ab6b36b42798bb99a2225..3b796264223f81fc753d0adaeccc04077023520b 100644
> --- a/tools/testing/selftests/filesystems/overlayfs/dev_in_maps.c
> +++ b/tools/testing/selftests/filesystems/overlayfs/dev_in_maps.c
> @@ -17,32 +17,7 @@
>
>  #include "../../kselftest.h"
>  #include "log.h"
> -
> -static int sys_fsopen(const char *fsname, unsigned int flags)
> -{
> -       return syscall(__NR_fsopen, fsname, flags);
> -}
> -
> -static int sys_fsconfig(int fd, unsigned int cmd, const char *key, const char *value, int aux)
> -{
> -       return syscall(__NR_fsconfig, fd, cmd, key, value, aux);
> -}
> -
> -static int sys_fsmount(int fd, unsigned int flags, unsigned int attr_flags)
> -{
> -       return syscall(__NR_fsmount, fd, flags, attr_flags);
> -}
> -static int sys_mount(const char *src, const char *tgt, const char *fst,
> -               unsigned long flags, const void *data)
> -{
> -       return syscall(__NR_mount, src, tgt, fst, flags, data);
> -}
> -static int sys_move_mount(int from_dfd, const char *from_pathname,
> -                         int to_dfd, const char *to_pathname,
> -                         unsigned int flags)
> -{
> -       return syscall(__NR_move_mount, from_dfd, from_pathname, to_dfd, to_pathname, flags);
> -}
> +#include "wrappers.h"
>
>  static long get_file_dev_and_inode(void *addr, struct statx *stx)
>  {
> diff --git a/tools/testing/selftests/filesystems/overlayfs/wrappers.h b/tools/testing/selftests/filesystems/overlayfs/wrappers.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..4f99e10f7f018fd9a7be5263f68d34807da4c53c
> --- /dev/null
> +++ b/tools/testing/selftests/filesystems/overlayfs/wrappers.h
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +#ifndef __SELFTEST_OVERLAYFS_WRAPPERS_H__
> +#define __SELFTEST_OVERLAYFS_WRAPPERS_H__
> +
> +#define _GNU_SOURCE
> +
> +#include <linux/types.h>
> +#include <linux/mount.h>
> +#include <sys/syscall.h>
> +
> +static inline int sys_fsopen(const char *fsname, unsigned int flags)
> +{
> +       return syscall(__NR_fsopen, fsname, flags);
> +}
> +
> +static inline int sys_fsconfig(int fd, unsigned int cmd, const char *key,
> +                              const char *value, int aux)
> +{
> +       return syscall(__NR_fsconfig, fd, cmd, key, value, aux);
> +}
> +
> +static inline int sys_fsmount(int fd, unsigned int flags,
> +                             unsigned int attr_flags)
> +{
> +       return syscall(__NR_fsmount, fd, flags, attr_flags);
> +}
> +
> +static inline int sys_mount(const char *src, const char *tgt, const char *fst,
> +                           unsigned long flags, const void *data)
> +{
> +       return syscall(__NR_mount, src, tgt, fst, flags, data);
> +}
> +
> +static inline int sys_move_mount(int from_dfd, const char *from_pathname,
> +                                int to_dfd, const char *to_pathname,
> +                                unsigned int flags)
> +{
> +       return syscall(__NR_move_mount, from_dfd, from_pathname, to_dfd,
> +                      to_pathname, flags);
> +}
> +
> +#endif
>
> --
> 2.45.2
>

Re: [PATCH RFC v2 1/4] fs: add helper to use mount option as path or fd

[Amir Goldstein]

On Sat, Oct 12, 2024 at 9:21 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Oct 11, 2024 at 11:46 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > Allow filesystems to use a mount option either as a
> > path or a file descriptor.
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
>
> Looks sane
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>
> > ---
> >  fs/fs_parser.c            | 19 +++++++++++++++++++
> >  include/linux/fs_parser.h |  5 ++++-
> >  2 files changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> > index 24727ec34e5aa434364e87879cccf9fe1ec19d37..a017415d8d6bc91608ece5d42fa4bea26e47456b 100644
> > --- a/fs/fs_parser.c
> > +++ b/fs/fs_parser.c
> > @@ -308,6 +308,25 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p,
> >  }
> >  EXPORT_SYMBOL(fs_param_is_fd);
> >
> > +int fs_param_is_fd_or_path(struct p_log *log, const struct fs_parameter_spec *p,
> > +                          struct fs_parameter *param,
> > +                          struct fs_parse_result *result)
> > +{
> > +       switch (param->type) {
> > +       case fs_value_is_string:
> > +               return fs_param_is_string(log, p, param, result);
> > +       case fs_value_is_file:
> > +               result->uint_32 = param->dirfd;
> > +               if (result->uint_32 <= INT_MAX)
> > +                       return 0;
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +       return fs_param_bad_value(log, param);
> > +}
> > +EXPORT_SYMBOL(fs_param_is_fd_or_path);
> > +

I just noticed that it is a little weird that fsparam_is_fd() accepts a numeric
string while fsparam_is_fd_or_path() does not.
Not to mention that fsparam_is_fd_or_path does not accept type filename.

Obviously a helper name fs_param_is_file_or_string() wouldn't have
raised those questions.
I will let you decide if this is something to worry about.

Thanks,
Amir.

Re: [PATCH RFC v2 2/4] ovl: specify layers via file descriptors

[Amir Goldstein]

On Fri, Oct 11, 2024 at 11:46 PM Christian Brauner <brauner@kernel.org> wrote:
>

nit: if you can avoid using the exact same title for the cover letter and
a patch that would be nice (gmail client collapses them together).

> Currently overlayfs only allows specifying layers through path names.
> This is inconvenient for users such as systemd that want to assemble an
> overlayfs mount purely based on file descriptors.
>
> This enables user to specify both:
>
>     fsconfig(fd_overlay, FSCONFIG_SET_FD, "upperdir+", NULL, fd_upper);
>     fsconfig(fd_overlay, FSCONFIG_SET_FD, "workdir+",  NULL, fd_work);
>     fsconfig(fd_overlay, FSCONFIG_SET_FD, "lowerdir+", NULL, fd_lower1);
>     fsconfig(fd_overlay, FSCONFIG_SET_FD, "lowerdir+", NULL, fd_lower2);
>
> in addition to:
>
>     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "upperdir+", "/upper",  0);
>     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "workdir+",  "/work",   0);
>     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "lowerdir+", "/lower1", 0);
>     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "lowerdir+", "/lower2", 0);
>

Please add a minimal example with FSCONFIG_SET_FD to overlayfs.rst.
I am not looking for a user manual, just one example to complement the
FSCONFIG_SET_STRING examples.

I don't mind adding config types on a per need basis, but out of curiosity
do you think the need will arise to support FSCONFIG_SET_PATH{,_EMPTY}
in the future? It is going to be any more challenging than just adding
support for
just FSCONFIG_SET_FD?

Again, not asking you to do extra work for a feature that no user asked for.

Other than that, it looks very nice and useful.

Thanks,
Amir.

Re: [PATCH RFC v2 2/4] ovl: specify layers via file descriptors

[Christian Brauner]

On Sat, Oct 12, 2024 at 10:25:38AM +0200, Amir Goldstein wrote:
> On Fri, Oct 11, 2024 at 11:46 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> 
> nit: if you can avoid using the exact same title for the cover letter and
> a patch that would be nice (gmail client collapses them together).

Fine, but fwiw, the solution to this problem is to use a proper email
client. ;)

> 
> > Currently overlayfs only allows specifying layers through path names.
> > This is inconvenient for users such as systemd that want to assemble an
> > overlayfs mount purely based on file descriptors.
> >
> > This enables user to specify both:
> >
> >     fsconfig(fd_overlay, FSCONFIG_SET_FD, "upperdir+", NULL, fd_upper);
> >     fsconfig(fd_overlay, FSCONFIG_SET_FD, "workdir+",  NULL, fd_work);
> >     fsconfig(fd_overlay, FSCONFIG_SET_FD, "lowerdir+", NULL, fd_lower1);
> >     fsconfig(fd_overlay, FSCONFIG_SET_FD, "lowerdir+", NULL, fd_lower2);
> >
> > in addition to:
> >
> >     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "upperdir+", "/upper",  0);
> >     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "workdir+",  "/work",   0);
> >     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "lowerdir+", "/lower1", 0);
> >     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "lowerdir+", "/lower2", 0);
> >
> 
> Please add a minimal example with FSCONFIG_SET_FD to overlayfs.rst.
> I am not looking for a user manual, just one example to complement the
> FSCONFIG_SET_STRING examples.
> 
> I don't mind adding config types on a per need basis, but out of curiosity
> do you think the need will arise to support FSCONFIG_SET_PATH{,_EMPTY}
> in the future? It is going to be any more challenging than just adding
> support for
> just FSCONFIG_SET_FD?

This could also be made to work rather easily but I wouldn't know why we
would want to add it. The current overlayfs FSCONFIG_SET_STRING variant
is mostly equivalent. Imho, it's a lot saner to let userspace do the
required open via regular openat{2}() and then use FSCONFIG_SET_FD, then
force *at() based semantics down into the filesystem via fsconfig().
U_PATH{_EMPTY} is unused and we could probably also get rid of it.

> 
> Again, not asking you to do extra work for a feature that no user asked for.
> 
> Other than that, it looks very nice and useful.

Thanks!

Re: [PATCH RFC v2 2/4] ovl: specify layers via file descriptors

[Amir Goldstein]

On Sat, Oct 12, 2024 at 12:37 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Sat, Oct 12, 2024 at 10:25:38AM +0200, Amir Goldstein wrote:
> > On Fri, Oct 11, 2024 at 11:46 PM Christian Brauner <brauner@kernel.org> wrote:
> > >
> >
> > nit: if you can avoid using the exact same title for the cover letter and
> > a patch that would be nice (gmail client collapses them together).
>
> Fine, but fwiw, the solution to this problem is to use a proper email
> client. ;)
>

touché :)

> >
> > > Currently overlayfs only allows specifying layers through path names.
> > > This is inconvenient for users such as systemd that want to assemble an
> > > overlayfs mount purely based on file descriptors.
> > >
> > > This enables user to specify both:
> > >
> > >     fsconfig(fd_overlay, FSCONFIG_SET_FD, "upperdir+", NULL, fd_upper);
> > >     fsconfig(fd_overlay, FSCONFIG_SET_FD, "workdir+",  NULL, fd_work);
> > >     fsconfig(fd_overlay, FSCONFIG_SET_FD, "lowerdir+", NULL, fd_lower1);
> > >     fsconfig(fd_overlay, FSCONFIG_SET_FD, "lowerdir+", NULL, fd_lower2);
> > >
> > > in addition to:
> > >
> > >     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "upperdir+", "/upper",  0);
> > >     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "workdir+",  "/work",   0);
> > >     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "lowerdir+", "/lower1", 0);
> > >     fsconfig(fd_overlay, FSCONFIG_SET_STRING, "lowerdir+", "/lower2", 0);
> > >
> >
> > Please add a minimal example with FSCONFIG_SET_FD to overlayfs.rst.
> > I am not looking for a user manual, just one example to complement the
> > FSCONFIG_SET_STRING examples.
> >
> > I don't mind adding config types on a per need basis, but out of curiosity
> > do you think the need will arise to support FSCONFIG_SET_PATH{,_EMPTY}
> > in the future? It is going to be any more challenging than just adding
> > support for
> > just FSCONFIG_SET_FD?
>
> This could also be made to work rather easily but I wouldn't know why we
> would want to add it. The current overlayfs FSCONFIG_SET_STRING variant
> is mostly equivalent. Imho, it's a lot saner to let userspace do the
> required open via regular openat{2}() and then use FSCONFIG_SET_FD, then
> force *at() based semantics down into the filesystem via fsconfig().

Fine be me. I am less familiar with the relevant use cases.

> U_PATH{_EMPTY} is unused and we could probably also get rid of it.
>

Oh. I didn't know that.

Thanks,
Amir.

Re: [PATCH RFC v2 1/4] fs: add helper to use mount option as path or fd

[Christian Brauner]

On Sat, Oct 12, 2024 at 10:20:04AM +0200, Amir Goldstein wrote:
> On Sat, Oct 12, 2024 at 9:21 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, Oct 11, 2024 at 11:46 PM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > Allow filesystems to use a mount option either as a
> > > path or a file descriptor.
> > >
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> >
> > Looks sane
> >
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> >
> > > ---
> > >  fs/fs_parser.c            | 19 +++++++++++++++++++
> > >  include/linux/fs_parser.h |  5 ++++-
> > >  2 files changed, 23 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> > > index 24727ec34e5aa434364e87879cccf9fe1ec19d37..a017415d8d6bc91608ece5d42fa4bea26e47456b 100644
> > > --- a/fs/fs_parser.c
> > > +++ b/fs/fs_parser.c
> > > @@ -308,6 +308,25 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p,
> > >  }
> > >  EXPORT_SYMBOL(fs_param_is_fd);
> > >
> > > +int fs_param_is_fd_or_path(struct p_log *log, const struct fs_parameter_spec *p,
> > > +                          struct fs_parameter *param,
> > > +                          struct fs_parse_result *result)
> > > +{
> > > +       switch (param->type) {
> > > +       case fs_value_is_string:
> > > +               return fs_param_is_string(log, p, param, result);
> > > +       case fs_value_is_file:
> > > +               result->uint_32 = param->dirfd;
> > > +               if (result->uint_32 <= INT_MAX)
> > > +                       return 0;
> > > +               break;
> > > +       default:
> > > +               break;
> > > +       }
> > > +       return fs_param_bad_value(log, param);
> > > +}
> > > +EXPORT_SYMBOL(fs_param_is_fd_or_path);
> > > +
> 
> I just noticed that it is a little weird that fsparam_is_fd() accepts a numeric
> string while fsparam_is_fd_or_path() does not.
> Not to mention that fsparam_is_fd_or_path does not accept type filename.
> 
> Obviously a helper name fs_param_is_file_or_string() wouldn't have

Yes, I'll use that. Thanks!