[PATCH] ceph: canonicalize server path in place

[PATCH] ceph: canonicalize server path in place

[Ilya Dryomov]

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@syzkaller.appspotmail.com
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 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 */
 };
 
-- 
2.19.2

Re: [PATCH] ceph: canonicalize server path in place

[Jeff Layton]

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@syzkaller.appspotmail.com
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
>  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@kernel.org>

Re: [PATCH] ceph: canonicalize server path in place

[Luis Henriques]

On Tue, Feb 11, 2020 at 07:06:11AM -0500, Jeff Layton wrote:
> 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")

I think both this fix and the original patch that introduced this should
go into stable kernels, because the original issue [1] can be easily
reproducible in the LTS kernels.

[1] https://tracker.ceph.com/issues/42771

I've done a quick attempt at backporting these patches to 5.4 and attached
them to the tracker issue.  I can send them to the stable mailing-list if
you guys think the backports are OK (the new mount API conversion moved
things around and it's not a clean cherry-pick).

Cheers,
--
Luís

> > Reported-by: syzbot+98704a51af8e3d9425a9@syzkaller.appspotmail.com
> > Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> > ---
> >  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@kernel.org>
> 

Re: [PATCH] ceph: canonicalize server path in place

[Jeff Layton]

On Tue, 2020-04-07 at 14:52 +0100, Luis Henriques wrote:
> On Tue, Feb 11, 2020 at 07:06:11AM -0500, Jeff Layton wrote:
> > 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")
> 
> I think both this fix and the original patch that introduced this should
> go into stable kernels, because the original issue [1] can be easily
> reproducible in the LTS kernels.
> 
> [1] https://tracker.ceph.com/issues/42771
> 
> I've done a quick attempt at backporting these patches to 5.4 and attached
> them to the tracker issue.  I can send them to the stable mailing-list if
> you guys think the backports are OK (the new mount API conversion moved
> things around and it's not a clean cherry-pick).
> 
> Cheers,
> --
> Luís
> 
> > > Reported-by: syzbot+98704a51af8e3d9425a9@syzkaller.appspotmail.com
> > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> > > ---
> > >  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@kernel.org>
> > 

I'm fine with getting stable to pick both of those up.

Looks like the main conflicts are due to the mount option parsing
changes? If you had to massage the patches to get them to merge, then
you should note that in the changelog so Greg knows why you're sending
him a backport rather than just asking him to pick it.

Otherwise I think these look good to me.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: canonicalize server path in place

[Luis Henriques]

On Tue, Apr 07, 2020 at 11:19:48AM -0400, Jeff Layton wrote:
> On Tue, 2020-04-07 at 14:52 +0100, Luis Henriques wrote:
...
> I'm fine with getting stable to pick both of those up.

Great!

> Looks like the main conflicts are due to the mount option parsing
> changes? If you had to massage the patches to get them to merge, then
> you should note that in the changelog so Greg knows why you're sending
> him a backport rather than just asking him to pick it.
> 
> Otherwise I think these look good to me.

Awesome, thanks.  I'll try to get these tested a bit for those stable
kernels and send the backports to Greg.

Cheers,
--
Luís