On Tue, 2020-02-11 at 10:53 +0100, Ilya Dryomov wrote: > syzbot reported that 4fbc0c711b24 ("ceph: remove the extra slashes in > the server path") had caused a regression where an allocation could be > done under a spinlock -- compare_mount_options() is called by sget_fc() > with sb_lock held. > > We don't really need the supplied server path, so canonicalize it > in place and compare it directly. To make this work, the leading > slash is kept around and the logic in ceph_real_mount() to skip it > is restored. CEPH_MSG_CLIENT_SESSION now reports the same (i.e. > canonicalized) path, with the leading slash of course. > > Fixes: 4fbc0c711b24 ("ceph: remove the extra slashes in the server path") > Reported-by: syzbot+98704a51af8e3d9425a9@xxxxxxxxxxxxxxxxxxxxxxxxx > Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx> > --- > fs/ceph/super.c | 121 +++++++++++------------------------------------- > fs/ceph/super.h | 2 +- > 2 files changed, 29 insertions(+), 94 deletions(-) > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c > index 1d9f083b8a11..64ea34ac330b 100644 > --- a/fs/ceph/super.c > +++ b/fs/ceph/super.c > @@ -202,6 +202,26 @@ struct ceph_parse_opts_ctx { > struct ceph_mount_options *opts; > }; > > +/* > + * Remove adjacent slashes and then the trailing slash, unless it is > + * the only remaining character. > + * > + * E.g. "//dir1////dir2///" --> "/dir1/dir2", "///" --> "/". > + */ > +static void canonicalize_path(char *path) > +{ > + int i, j = 0; > + > + for (i = 0; path[i] != '\0'; i++) { > + if (path[i] != '/' || j < 1 || path[j - 1] != '/') > + path[j++] = path[i]; > + } > + > + if (j > 1 && path[j - 1] == '/') > + j--; > + path[j] = '\0'; > +} > + > /* > * Parse the source parameter. Distinguish the server list from the path. > * > @@ -224,15 +244,16 @@ static int ceph_parse_source(struct fs_parameter *param, struct fs_context *fc) > > dev_name_end = strchr(dev_name, '/'); > if (dev_name_end) { > - kfree(fsopt->server_path); > - > /* > * The server_path will include the whole chars from userland > * including the leading '/'. > */ > + kfree(fsopt->server_path); > fsopt->server_path = kstrdup(dev_name_end, GFP_KERNEL); > if (!fsopt->server_path) > return -ENOMEM; > + > + canonicalize_path(fsopt->server_path); > } else { > dev_name_end = dev_name + strlen(dev_name); > } > @@ -456,73 +477,6 @@ 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) > @@ -530,7 +484,6 @@ 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); > @@ -540,21 +493,12 @@ static int compare_mount_options(struct ceph_mount_options *new_fsopt, > ret = strcmp_null(fsopt1->snapdir_name, fsopt2->snapdir_name); > if (ret) > return ret; > + > ret = strcmp_null(fsopt1->mds_namespace, fsopt2->mds_namespace); > if (ret) > return ret; > > - 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); > + ret = strcmp_null(fsopt1->server_path, fsopt2->server_path); > if (ret) > return ret; > > @@ -957,7 +901,9 @@ 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, *p; > + const char *path = fsc->mount_options->server_path ? > + fsc->mount_options->server_path + 1 : ""; > + > err = __ceph_open_session(fsc->client, started); > if (err < 0) > goto out; > @@ -969,22 +915,11 @@ static struct dentry *ceph_real_mount(struct ceph_fs_client *fsc, > goto out; > } > > - 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; > diff --git a/fs/ceph/super.h b/fs/ceph/super.h > index 1e456a9011bb..037cdfb2ad4f 100644 > --- a/fs/ceph/super.h > +++ b/fs/ceph/super.h > @@ -91,7 +91,7 @@ struct ceph_mount_options { > > char *snapdir_name; /* default ".snap" */ > char *mds_namespace; /* default NULL */ > - char *server_path; /* default "/" */ > + char *server_path; /* default NULL (means "/") */ > char *fscache_uniq; /* default NULL */ > }; > Nice work. Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>