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>
