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

From: Sasha Levin
Date: Fri Feb 14 2020 - 13:55:27 EST


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