| Message ID | 20241011-work-overlayfs-v2-2-1b43328c5a31@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ovl: specify layers via file descriptors | expand |
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;
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.
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!
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.
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;
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(-)