Re: [PATCH AUTOSEL 5.5 487/542] ceph: remove the extra slashes in the server path

[Sasha Levin]

On Fri, Feb 14, 2020 at 05:13:21PM +0100, Ilya Dryomov wrote:
> On Fri, Feb 14, 2020 at 4:59 PM Sasha Levin <sashal@xxxxxxxxxx> wrote:
> > From: Xiubo Li <xiubli@xxxxxxxxxx>
> >
> > [ Upstream commit 4fbc0c711b2464ee1551850b85002faae0b775d5 ]
> >
> > It's possible to pass the mount helper a server path that has more
> > than one contiguous slash character. For example:
> >
> >   $ mount -t ceph 192.168.195.165:40176:/// /mnt/cephfs/
> >
> > In the MDS server side the extra slashes of the server path will be
> > treated as snap dir, and then we can get the following debug logs:
> >
> >   ceph:  mount opening path //
> >   ceph:  open_root_inode opening '//'
> >   ceph:  fill_trace 0000000059b8a3bc is_dentry 0 is_target 1
> >   ceph:  alloc_inode 00000000dc4ca00b
> >   ceph:  get_inode created new inode 00000000dc4ca00b 1.ffffffffffffffff ino 1
> >   ceph:  get_inode on 1=1.ffffffffffffffff got 00000000dc4ca00b
> >
> > And then when creating any new file or directory under the mount
> > point, we can hit the following BUG_ON in ceph_fill_trace():
> >
> >   BUG_ON(ceph_snap(dir) != dvino.snap);
> >
> > Have the client ignore the extra slashes in the server path when
> > mounting. This will also canonicalize the path, so that identical mounts
> > can be consilidated.
> >
> > 1) "//mydir1///mydir//"
> > 2) "/mydir1/mydir"
> > 3) "/mydir1/mydir/"
> >
> > Regardless of the internal treatment of these paths, the kernel still
> > stores the original string including the leading '/' for presentation
> > to userland.
> >
> > URL: https://tracker.ceph.com/issues/42771
> > Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> > Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx>
> > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> > ---
> >  fs/ceph/super.c | 122 ++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 102 insertions(+), 20 deletions(-)
> >
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 430dcf329723a..112927dbd2f20 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -107,7 +107,6 @@ static int ceph_statfs(struct dentry *dentry, struct kstatfs *buf)
> >         return 0;
> >  }
> >
> > -
> >  static int ceph_sync_fs(struct super_block *sb, int wait)
> >  {
> >         struct ceph_fs_client *fsc = ceph_sb_to_client(sb);
> > @@ -211,7 +210,6 @@ struct ceph_parse_opts_ctx {
> >
> >  /*
> >   * Parse the source parameter.  Distinguish the server list from the path.
> > - * Internally we do not include the leading '/' in the path.
> >   *
> >   * The source will look like:
> >   *     <server_spec>[,<server_spec>...]:[<path>]
> > @@ -232,12 +230,15 @@ static int ceph_parse_source(struct fs_parameter *param, struct fs_context *fc)
> >
> >         dev_name_end = strchr(dev_name, '/');
> >         if (dev_name_end) {
> > -               if (strlen(dev_name_end) > 1) {
> > -                       kfree(fsopt->server_path);
> > -                       fsopt->server_path = kstrdup(dev_name_end, GFP_KERNEL);
> > -                       if (!fsopt->server_path)
> > -                               return -ENOMEM;
> > -               }
> > +               kfree(fsopt->server_path);
> > +
> > +               /*
> > +                * The server_path will include the whole chars from userland
> > +                * including the leading '/'.
> > +                */
> > +               fsopt->server_path = kstrdup(dev_name_end, GFP_KERNEL);
> > +               if (!fsopt->server_path)
> > +                       return -ENOMEM;
> >         } else {
> >                 dev_name_end = dev_name + strlen(dev_name);
> >         }
> > @@ -461,6 +462,73 @@ static int strcmp_null(const char *s1, const char *s2)
> >         return strcmp(s1, s2);
> >  }
> >
> > +/**
> > + * path_remove_extra_slash - Remove the extra slashes in the server path
> > + * @server_path: the server path and could be NULL
> > + *
> > + * Return NULL if the path is NULL or only consists of "/", or a string
> > + * without any extra slashes including the leading slash(es) and the
> > + * slash(es) at the end of the server path, such as:
> > + * "//dir1////dir2///" --> "dir1/dir2"
> > + */
> > +static char *path_remove_extra_slash(const char *server_path)
> > +{
> > +       const char *path = server_path;
> > +       const char *cur, *end;
> > +       char *buf, *p;
> > +       int len;
> > +
> > +       /* if the server path is omitted */
> > +       if (!path)
> > +               return NULL;
> > +
> > +       /* remove all the leading slashes */
> > +       while (*path == '/')
> > +               path++;
> > +
> > +       /* if the server path only consists of slashes */
> > +       if (*path == '\0')
> > +               return NULL;
> > +
> > +       len = strlen(path);
> > +
> > +       buf = kmalloc(len + 1, GFP_KERNEL);
> > +       if (!buf)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       end = path + len;
> > +       p = buf;
> > +       do {
> > +               cur = strchr(path, '/');
> > +               if (!cur)
> > +                       cur = end;
> > +
> > +               len = cur - path;
> > +
> > +               /* including one '/' */
> > +               if (cur != end)
> > +                       len += 1;
> > +
> > +               memcpy(p, path, len);
> > +               p += len;
> > +
> > +               while (cur <= end && *cur == '/')
> > +                       cur++;
> > +               path = cur;
> > +       } while (path < end);
> > +
> > +       *p = '\0';
> > +
> > +       /*
> > +        * remove the last slash if there has and just to make sure that
> > +        * we will get something like "dir1/dir2"
> > +        */
> > +       if (*(--p) == '/')
> > +               *p = '\0';
> > +
> > +       return buf;
> > +}
> > +
> >  static int compare_mount_options(struct ceph_mount_options *new_fsopt,
> >                                  struct ceph_options *new_opt,
> >                                  struct ceph_fs_client *fsc)
> > @@ -468,6 +536,7 @@ static int compare_mount_options(struct ceph_mount_options *new_fsopt,
> >         struct ceph_mount_options *fsopt1 = new_fsopt;
> >         struct ceph_mount_options *fsopt2 = fsc->mount_options;
> >         int ofs = offsetof(struct ceph_mount_options, snapdir_name);
> > +       char *p1, *p2;
> >         int ret;
> >
> >         ret = memcmp(fsopt1, fsopt2, ofs);
> > @@ -480,9 +549,21 @@ static int compare_mount_options(struct ceph_mount_options *new_fsopt,
> >         ret = strcmp_null(fsopt1->mds_namespace, fsopt2->mds_namespace);
> >         if (ret)
> >                 return ret;
> > -       ret = strcmp_null(fsopt1->server_path, fsopt2->server_path);
> > +
> > +       p1 = path_remove_extra_slash(fsopt1->server_path);
> > +       if (IS_ERR(p1))
> > +               return PTR_ERR(p1);
> > +       p2 = path_remove_extra_slash(fsopt2->server_path);
> > +       if (IS_ERR(p2)) {
> > +               kfree(p1);
> > +               return PTR_ERR(p2);
> > +       }
> > +       ret = strcmp_null(p1, p2);
> > +       kfree(p1);
> > +       kfree(p2);
> >         if (ret)
> >                 return ret;
> > +
> >         ret = strcmp_null(fsopt1->fscache_uniq, fsopt2->fscache_uniq);
> >         if (ret)
> >                 return ret;
> > @@ -788,7 +869,6 @@ static void destroy_caches(void)
> >         ceph_fscache_unregister();
> >  }
> >
> > -
> >  /*
> >   * ceph_umount_begin - initiate forced umount.  Tear down down the
> >   * mount, skipping steps that may hang while waiting for server(s).
> > @@ -868,9 +948,6 @@ static struct dentry *open_root_dentry(struct ceph_fs_client *fsc,
> >         return root;
> >  }
> >
> > -
> > -
> > -
> >  /*
> >   * mount: join the ceph cluster, and open root directory.
> >   */
> > @@ -885,7 +962,7 @@ static struct dentry *ceph_real_mount(struct ceph_fs_client *fsc,
> >         mutex_lock(&fsc->client->mount_mutex);
> >
> >         if (!fsc->sb->s_root) {
> > -               const char *path;
> > +               const char *path, *p;
> >                 err = __ceph_open_session(fsc->client, started);
> >                 if (err < 0)
> >                         goto out;
> > @@ -897,17 +974,22 @@ static struct dentry *ceph_real_mount(struct ceph_fs_client *fsc,
> >                                 goto out;
> >                 }
> >
> > -               if (!fsc->mount_options->server_path) {
> > -                       path = "";
> > -                       dout("mount opening path \\t\n");
> > -               } else {
> > -                       path = fsc->mount_options->server_path + 1;
> > -                       dout("mount opening path %s\n", path);
> > +               p = path_remove_extra_slash(fsc->mount_options->server_path);
> > +               if (IS_ERR(p)) {
> > +                       err = PTR_ERR(p);
> > +                       goto out;
> >                 }
> > +               /* if the server path is omitted or just consists of '/' */
> > +               if (!p)
> > +                       path = "";
> > +               else
> > +                       path = p;
> > +               dout("mount opening path '%s'\n", path);
> >
> >                 ceph_fs_debugfs_init(fsc);
> >
> >                 root = open_root_dentry(fsc, path, started);
> > +               kfree(p);
> >                 if (IS_ERR(root)) {
> >                         err = PTR_ERR(root);
> >                         goto out;
>
> Hi Sasha,
>
> This commit is buggy, the fix should land in 5.6-rc2.  Please drop it
> for now -- it would need to be taken together with "ceph: canonicalize
> server path in place" or not backported at all.

I'll try and queue up the fix when it's upstream. This series will
probably be queued closer to -rc3 anyways.

--
Thanks,
Sasha