Re: [PATCH v2 1/2] ovl: port to new mount api

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Thanks,
Amir.




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux