On Fri, 2019-12-20 at 06:46 -0500, Jeff Layton wrote:
> On Tue, 2019-12-17 at 19:40 -0500, xiubli@xxxxxxxxxx wrote:
> > From: Xiubo Li <xiubli@xxxxxxxxxx>
> >
> > When mounting if the server path has more than one slash continuously,
> > such as:
> >
> > 'mount.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:
> >
> > <7>[ ...] ceph: mount opening path //
> > <7>[ ...] ceph: open_root_inode opening '//'
> > <7>[ ...] ceph: fill_trace 0000000059b8a3bc is_dentry 0 is_target 1
> > <7>[ ...] ceph: alloc_inode 00000000dc4ca00b
> > <7>[ ...] ceph: get_inode created new inode 00000000dc4ca00b 1.ffffffffffffffff ino 1
> > <7>[ ...] ceph: get_inode on 1=1.ffffffffffffffff got 00000000dc4ca00b
> >
> > And then when creating any new file or directory under the mount
> > point, we can get the following crash core dump:
> >
> > <4>[ ...] ------------[ cut here ]------------
> > <2>[ ...] kernel BUG at fs/ceph/inode.c:1347!
> > <4>[ ...] invalid opcode: 0000 [#1] SMP PTI
> > <4>[ ...] CPU: 0 PID: 7 Comm: kworker/0:1 Tainted: G E 5.4.0-rc5+ #1
> > <4>[ ...] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 05/19/2017
> > <4>[ ...] Workqueue: ceph-msgr ceph_con_workfn [libceph]
> > <4>[ ...] RIP: 0010:ceph_fill_trace+0x992/0xb30 [ceph]
> > <4>[ ...] Code: ff 0f 0b 0f 0b 0f 0b 4c 89 fa 48 c7 c6 4d [...]
> > <4>[ ...] RSP: 0018:ffffa23d40067c70 EFLAGS: 00010297
> > <4>[ ...] RAX: fffffffffffffffe RBX: ffff8a229eb566c0 RCX: 0000000000000006
> > <4>[ ...] RDX: 0000000000000000 RSI: 0000000000000092 RDI: ffff8a23aec17900
> > <4>[ ...] RBP: ffff8a226bd91eb0 R08: 0000000000000001 R09: 0000000000000885
> > <4>[ ...] R10: 000000000002dfd8 R11: ffff8a226bd95b30 R12: ffff8a239347e000
> > <4>[ ...] R13: 0000000000000000 R14: ffff8a22fabeb000 R15: ffff8a2338b0c900
> > <4>[ ...] FS: 0000000000000000(0000) GS:ffff8a23aec00000(0000) knlGS:0000000000000000
> > <4>[ ...] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > <4>[ ...] CR2: 000055b479d92068 CR3: 00000003764f6004 CR4: 00000000003606f0
> > <4>[ ...] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > <4>[ ...] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > <4>[ ...] Call Trace:
> > <4>[ ...] dispatch+0x2ac/0x12b0 [ceph]
> > <4>[ ...] ceph_con_workfn+0xd40/0x27c0 [libceph]
> > <4>[ ...] process_one_work+0x1b0/0x350
> > <4>[ ...] worker_thread+0x50/0x3b0
> > <4>[ ...] kthread+0xfb/0x130
> > <4>[ ...] ? process_one_work+0x350/0x350
> > <4>[ ...] ? kthread_park+0x90/0x90
> > <4>[ ...] ret_from_fork+0x35/0x40
> > <4>[ ...] Modules linked in: ceph(E) libceph fscache [...]
> > <4>[ ...] ---[ end trace ba883d8ccf9afcb0 ]---
> > <4>[ ...] RIP: 0010:ceph_fill_trace+0x992/0xb30 [ceph]
> > <4>[ ...] Code: ff 0f 0b 0f 0b 0f 0b 4c 89 fa 48 c7 c6 [...]
> > <4>[ ...] RSP: 0018:ffffa23d40067c70 EFLAGS: 00010297
> > <4>[ ...] RAX: fffffffffffffffe RBX: ffff8a229eb566c0 RCX: 0000000000000006
> > <4>[ ...] RDX: 0000000000000000 RSI: 0000000000000092 RDI: ffff8a23aec17900
> > <4>[ ...] RBP: ffff8a226bd91eb0 R08: 0000000000000001 R09: 0000000000000885
> > <4>[ ...] R10: 000000000002dfd8 R11: ffff8a226bd95b30 R12: ffff8a239347e000
> > <4>[ ...] R13: 0000000000000000 R14: ffff8a22fabeb000 R15: ffff8a2338b0c900
> > <4>[ ...] FS: 0000000000000000(0000) GS:ffff8a23aec00000(0000) knlGS:0000000000000000
> > <4>[ ...] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > <4>[ ...] CR2: 000055b479d92068 CR3: 00000003764f6004 CR4: 00000000003606f0
> > <4>[ ...] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > <4>[ ...] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >
> > And we should ignore the extra slashes in the server path when mount
> > opening in case users have added them by mistake.
> >
> > This will also help us to get an existing superblock if all the
> > other options are the same, such as all the following server paths
> > are treated as the same:
> >
> > 1) "//mydir1///mydir//"
> > 2) "/mydir1/mydir"
> > 3) "/mydir1/mydir/"
> >
> > The mount_options->server_path will always save the original string
> > including the leading '/'.
> >
> > Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> > ---
> >
> > Changed in V3:
> > - switch strchr instead.
> >
> > Changed in V2:
> > - rebase to new mount API.
> >
> >
> > fs/ceph/super.c | 118 ++++++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 98 insertions(+), 20 deletions(-)
> >
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 6f33a265ccf1..9c9af183ad45 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);
> > }
> > @@ -459,6 +460,69 @@ 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;
> > +
> > + /* including the last '/' */
> > + len = cur - path + 1;
> > + memcpy(p, path, len);
> > + p += len;
> > +
> > + while (*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)
> > @@ -466,6 +530,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);
> > @@ -478,9 +543,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;
> > @@ -786,7 +863,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).
> > @@ -866,9 +942,6 @@ static struct dentry *open_root_dentry(struct ceph_fs_client *fsc,
> > return root;
> > }
> >
> > -
> > -
> > -
> > /*
> > * mount: join the ceph cluster, and open root directory.
> > */
> > @@ -883,7 +956,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;
> > @@ -895,17 +968,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;
>
> Ok. Given that the MDS treats "//" as special, I think we do have to fix
> this in the kclient after all. This looks fine to me, so merged. Should
> make 5.6.
>
Hmm, I'll take it back. I was doing a bit of testing with this, and I'm
seeing memory corruption issues when running xfstests on it against a
vstart cluster. Dropped for now.
Xiubo, you may want to try this patch with KASAN enabled and see if it
points out the issue.
Thanks,
--
Jeff Layton <jlayton@xxxxxxxxxx>
