On Fri, Jun 09, 2023 at 10:25:21PM +0300, Amir Goldstein wrote: > On Fri, Jun 9, 2023 at 6:42 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > We recently ported util-linux to the new mount api. Now the mount(8) > > tool will by default use the new mount api. While trying hard to fall > > back to the old mount api gracefully there are still cases where we run > > into issues that are difficult to handle nicely. > > > > Now with mount(8) and libmount supporting the new mount api I expect an > > increase in the number of bug reports and issues we're going to see with > > filesystems that don't yet support the new mount api. So it's time we > > rectify this. > > > > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx> > > --- > > fs/overlayfs/super.c | 515 ++++++++++++++++++++++++++++----------------------- > > 1 file changed, 279 insertions(+), 236 deletions(-) > > > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > > index f97ad8b40dbb..ceaf05743f45 100644 > > --- a/fs/overlayfs/super.c > > +++ b/fs/overlayfs/super.c > > @@ -16,6 +16,8 @@ > > #include <linux/posix_acl_xattr.h> > > #include <linux/exportfs.h> > > #include <linux/file.h> > > +#include <linux/fs_context.h> > > +#include <linux/fs_parser.h> > > #include "overlayfs.h" > > > > MODULE_AUTHOR("Miklos Szeredi <miklos@xxxxxxxxxx>"); > > @@ -67,6 +69,59 @@ module_param_named(metacopy, ovl_metacopy_def, bool, 0644); > > MODULE_PARM_DESC(metacopy, > > "Default to on or off for the metadata only copy up feature"); > > > > +enum { > > + Opt_lowerdir, > > + Opt_upperdir, > > + Opt_workdir, > > + Opt_default_permissions, > > + Opt_redirect_dir, > > + Opt_index, > > + Opt_uuid, > > + Opt_nfs_export, > > + Opt_userxattr, > > + Opt_xino, > > + Opt_metacopy, > > + Opt_volatile, > > +}; > > + > > +static const struct constant_table ovl_parameter_bool[] = { > > + { "on", true }, > > + { "off", false }, > > + {} > > +}; > > + > > +static const struct constant_table ovl_parameter_xino[] = { > > + { "on", OVL_XINO_ON }, > > + { "off", OVL_XINO_OFF }, > > + { "auto", OVL_XINO_AUTO }, > > + {} > > +}; > > + > > +static const struct fs_parameter_spec ovl_parameter_spec[] = { > > + fsparam_string("lowerdir", Opt_lowerdir), > > + fsparam_string("upperdir", Opt_upperdir), > > + fsparam_string("workdir", Opt_workdir), > > + fsparam_flag("default_permissions", Opt_default_permissions), > > + fsparam_enum("redirect_dir", Opt_redirect_dir, ovl_parameter_bool), > > + fsparam_enum("index", Opt_index, ovl_parameter_bool), > > + fsparam_enum("uuid", Opt_uuid, ovl_parameter_bool), > > + fsparam_enum("nfs_export", Opt_nfs_export, ovl_parameter_bool), > > + fsparam_flag("userxattr", Opt_userxattr), > > + fsparam_enum("xino", Opt_xino, ovl_parameter_xino), > > + fsparam_enum("metacopy", Opt_metacopy, ovl_parameter_bool), > > + fsparam_flag("volatile", Opt_volatile), > > + {} > > +}; > > + > > +#define OVL_METACOPY_SET BIT(0) > > +#define OVL_REDIRECT_SET BIT(1) > > +#define OVL_NFS_EXPORT_SET BIT(2) > > +#define OVL_INDEX_SET BIT(3) > > + > > +struct ovl_fs_context { > > + u8 set; > > +}; > > + > > static void ovl_dentry_release(struct dentry *dentry) > > { > > struct ovl_entry *oe = dentry->d_fsdata; > > @@ -394,27 +449,6 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry) > > return 0; > > } > > > > -static int ovl_remount(struct super_block *sb, int *flags, char *data) > > -{ > > - struct ovl_fs *ofs = sb->s_fs_info; > > - struct super_block *upper_sb; > > - int ret = 0; > > - > > - if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs)) > > - return -EROFS; > > - > > - if (*flags & SB_RDONLY && !sb_rdonly(sb)) { > > - upper_sb = ovl_upper_mnt(ofs)->mnt_sb; > > - if (ovl_should_sync(ofs)) { > > - down_read(&upper_sb->s_umount); > > - ret = sync_filesystem(upper_sb); > > - up_read(&upper_sb->s_umount); > > - } > > - } > > - > > - return ret; > > -} > > - > > static const struct super_operations ovl_super_operations = { > > .alloc_inode = ovl_alloc_inode, > > .free_inode = ovl_free_inode, > > @@ -424,76 +458,8 @@ static const struct super_operations ovl_super_operations = { > > .sync_fs = ovl_sync_fs, > > .statfs = ovl_statfs, > > .show_options = ovl_show_options, > > - .remount_fs = ovl_remount, > > -}; > > - > > -enum { > > - OPT_LOWERDIR, > > - OPT_UPPERDIR, > > - OPT_WORKDIR, > > - OPT_DEFAULT_PERMISSIONS, > > - OPT_REDIRECT_DIR, > > - OPT_INDEX_ON, > > - OPT_INDEX_OFF, > > - OPT_UUID_ON, > > - OPT_UUID_OFF, > > - OPT_NFS_EXPORT_ON, > > - OPT_USERXATTR, > > - OPT_NFS_EXPORT_OFF, > > - OPT_XINO_ON, > > - OPT_XINO_OFF, > > - OPT_XINO_AUTO, > > - OPT_METACOPY_ON, > > - OPT_METACOPY_OFF, > > - OPT_VOLATILE, > > - OPT_ERR, > > -}; > > - > > -static const match_table_t ovl_tokens = { > > - {OPT_LOWERDIR, "lowerdir=%s"}, > > - {OPT_UPPERDIR, "upperdir=%s"}, > > - {OPT_WORKDIR, "workdir=%s"}, > > - {OPT_DEFAULT_PERMISSIONS, "default_permissions"}, > > - {OPT_REDIRECT_DIR, "redirect_dir=%s"}, > > - {OPT_INDEX_ON, "index=on"}, > > - {OPT_INDEX_OFF, "index=off"}, > > - {OPT_USERXATTR, "userxattr"}, > > - {OPT_UUID_ON, "uuid=on"}, > > - {OPT_UUID_OFF, "uuid=off"}, > > - {OPT_NFS_EXPORT_ON, "nfs_export=on"}, > > - {OPT_NFS_EXPORT_OFF, "nfs_export=off"}, > > - {OPT_XINO_ON, "xino=on"}, > > - {OPT_XINO_OFF, "xino=off"}, > > - {OPT_XINO_AUTO, "xino=auto"}, > > - {OPT_METACOPY_ON, "metacopy=on"}, > > - {OPT_METACOPY_OFF, "metacopy=off"}, > > - {OPT_VOLATILE, "volatile"}, > > - {OPT_ERR, NULL} > > }; > > > > -static char *ovl_next_opt(char **s) > > -{ > > - char *sbegin = *s; > > - char *p; > > - > > - if (sbegin == NULL) > > - return NULL; > > - > > - for (p = sbegin; *p; p++) { > > - if (*p == '\\') { > > - p++; > > - if (!*p) > > - break; > > - } else if (*p == ',') { > > - *p = '\0'; > > - *s = p + 1; > > - return sbegin; > > - } > > - } > > - *s = NULL; > > - return sbegin; > > -} > > - > > static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode) > > { > > if (strcmp(mode, "on") == 0) { > > @@ -517,123 +483,14 @@ static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode) > > return 0; > > } > > > > -static int ovl_parse_opt(char *opt, struct ovl_config *config) > > +static int ovl_fs_params_verify(const struct ovl_fs_context *ctx, > > + struct ovl_config *config) > > { > > - char *p; > > int err; > > - bool metacopy_opt = false, redirect_opt = false; > > - bool nfs_export_opt = false, index_opt = false; > > - > > - config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL); > > - if (!config->redirect_mode) > > - return -ENOMEM; > > - > > - while ((p = ovl_next_opt(&opt)) != NULL) { > > - int token; > > - substring_t args[MAX_OPT_ARGS]; > > - > > - if (!*p) > > - continue; > > - > > - token = match_token(p, ovl_tokens, args); > > - switch (token) { > > - case OPT_UPPERDIR: > > - kfree(config->upperdir); > > - config->upperdir = match_strdup(&args[0]); > > - if (!config->upperdir) > > - return -ENOMEM; > > - break; > > - > > - case OPT_LOWERDIR: > > - kfree(config->lowerdir); > > - config->lowerdir = match_strdup(&args[0]); > > - if (!config->lowerdir) > > - return -ENOMEM; > > - break; > > - > > - case OPT_WORKDIR: > > - kfree(config->workdir); > > - config->workdir = match_strdup(&args[0]); > > - if (!config->workdir) > > - return -ENOMEM; > > - break; > > - > > - case OPT_DEFAULT_PERMISSIONS: > > - config->default_permissions = true; > > - break; > > - > > - case OPT_REDIRECT_DIR: > > - kfree(config->redirect_mode); > > - config->redirect_mode = match_strdup(&args[0]); > > - if (!config->redirect_mode) > > - return -ENOMEM; > > - redirect_opt = true; > > - break; > > - > > - case OPT_INDEX_ON: > > - config->index = true; > > - index_opt = true; > > - break; > > - > > - case OPT_INDEX_OFF: > > - config->index = false; > > - index_opt = true; > > - break; > > - > > - case OPT_UUID_ON: > > - config->uuid = true; > > - break; > > - > > - case OPT_UUID_OFF: > > - config->uuid = false; > > - break; > > - > > - case OPT_NFS_EXPORT_ON: > > - config->nfs_export = true; > > - nfs_export_opt = true; > > - break; > > - > > - case OPT_NFS_EXPORT_OFF: > > - config->nfs_export = false; > > - nfs_export_opt = true; > > - break; > > - > > - case OPT_XINO_ON: > > - config->xino = OVL_XINO_ON; > > - break; > > - > > - case OPT_XINO_OFF: > > - config->xino = OVL_XINO_OFF; > > - break; > > - > > - case OPT_XINO_AUTO: > > - config->xino = OVL_XINO_AUTO; > > - break; > > - > > - case OPT_METACOPY_ON: > > - config->metacopy = true; > > - metacopy_opt = true; > > - break; > > - > > - case OPT_METACOPY_OFF: > > - config->metacopy = false; > > - metacopy_opt = true; > > - break; > > - > > - case OPT_VOLATILE: > > - config->ovl_volatile = true; > > - break; > > - > > - case OPT_USERXATTR: > > - config->userxattr = true; > > - break; > > - > > - default: > > - pr_err("unrecognized mount option \"%s\" or missing value\n", > > - p); > > - return -EINVAL; > > - } > > - } > > + bool metacopy_opt = ctx->set & OVL_METACOPY_SET, > > + redirect_opt = ctx->set & OVL_REDIRECT_SET; > > + bool nfs_export_opt = ctx->set & OVL_NFS_EXPORT_SET, > > + index_opt = ctx->set & OVL_INDEX_SET; > > Nit: please split lines here. > > > > > /* Workdir/index are useless in non-upper mount */ > > if (!config->upperdir) { > > @@ -1882,12 +1739,143 @@ static struct dentry *ovl_get_root(struct super_block *sb, > > return root; > > } > > > > -static int ovl_fill_super(struct super_block *sb, void *data, int silent) > > +static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param) > > { > > - struct path upperpath = { }; > > + int err = 0; > > + struct fs_parse_result result; > > + struct ovl_fs *ofs = fc->s_fs_info; > > + struct ovl_config *config = &ofs->config; > > + struct ovl_fs_context *ctx = fc->fs_private; > > + struct path path; > > + char *dup; > > + int opt; > > + char *sval; > > + > > + /* > > + * On remount overlayfs has always ignored all mount options no > > + * matter if malformed or not so for backwards compatibility we > > + * do the same here. > > + */ > > + if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) > > + return 0; > > + > > + opt = fs_parse(fc, ovl_parameter_spec, param, &result); > > + if (opt < 0) > > + return opt; > > + > > + switch (opt) { > > + case Opt_lowerdir: > > + dup = kstrdup(param->string, GFP_KERNEL); > > + if (!dup) { > > + path_put(&path); > > + err = -ENOMEM; > > + break; > > + } > > + > > + kfree(config->lowerdir); > > + config->lowerdir = dup; > > + break; > > + case Opt_upperdir: > > + dup = kstrdup(param->string, GFP_KERNEL); > > + if (!dup) { > > + path_put(&path); > > + err = -ENOMEM; > > + break; > > + } > > + > > + kfree(config->upperdir); > > + config->upperdir = dup; > > + break; > > + case Opt_workdir: > > + dup = kstrdup(param->string, GFP_KERNEL); > > + if (!dup) { > > + path_put(&path); > > + err = -ENOMEM; > > + break; > > + } > > + > > + kfree(config->workdir); > > + config->workdir = dup; > > + break; > > + case Opt_default_permissions: > > + config->default_permissions = true; > > + break; > > + case Opt_index: > > + config->index = result.uint_32; > > + ctx->set |= OVL_INDEX_SET; > > + break; > > + case Opt_uuid: > > + config->uuid = result.uint_32; > > + break; > > + case Opt_nfs_export: > > + config->nfs_export = result.uint_32; > > + ctx->set |= OVL_NFS_EXPORT_SET; > > + break; > > + case Opt_metacopy: > > + config->metacopy = result.uint_32; > > + ctx->set |= OVL_METACOPY_SET; > > + break; > > + case Opt_userxattr: > > + config->userxattr = true; > > + break; > > + case Opt_volatile: > > + config->ovl_volatile = true; > > + break; > > + case Opt_xino: > > + config->xino = result.uint_32; > > + break; > > + case Opt_redirect_dir: > > + if (result.uint_32 == true) > > + sval = kstrdup("on", GFP_KERNEL); > > + else > > + sval = kstrdup("off", GFP_KERNEL); > > + if (!sval) { > > + err = -ENOMEM; > > + break; > > + } > > + > > + kfree(config->redirect_mode); > > + config->redirect_mode = sval; > > + ctx->set |= OVL_REDIRECT_SET; > > + break; > > + default: > > + pr_err("unrecognized mount option \"%s\" or missing value\n", param->key); > > + return -EINVAL; > > + } > > + > > + return err; > > +} > > > For the end result, all these functions above should be in params.c > I don't mind if you move it with an extra patch at the beginning > Probably easier at the end. Just as long as you do not move > ovl_fs_params_verify() in the same patch that changes half of it. Ok.