[v3] ceph: only use d_name directly when parent is locked
diff mbox series

Message ID 20190418170046.18593-1-jlayton@kernel.org
State New
Headers show
Series
  • [v3] ceph: only use d_name directly when parent is locked
Related show

Commit Message

Jeff Layton April 18, 2019, 5 p.m. UTC
Ben reported tripping the BUG_ON in create_request_message during some
performance testing. Analysis of the vmcore showed that the length of
the r_dentry->d_name string changed after we allocated the buffer, but
before we encoded it.

build_dentry_path returns pointers to d_name in the common case of
non-snapped dentries, but this optimization isn't safe unless the parent
directory is locked. When it isn't, have the code make a copy of the
d_name while holding the d_lock.

Cc: stable@vger.kernel.org
Reported-by: Ben England <bengland@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/mds_client.c | 60 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 49 insertions(+), 11 deletions(-)

Comments

Yan, Zheng April 19, 2019, 1:52 a.m. UTC | #1
On 4/19/19 1:00 AM, Jeff Layton wrote:
> Ben reported tripping the BUG_ON in create_request_message during some
> performance testing. Analysis of the vmcore showed that the length of
> the r_dentry->d_name string changed after we allocated the buffer, but
> before we encoded it.
> 
> build_dentry_path returns pointers to d_name in the common case of
> non-snapped dentries, but this optimization isn't safe unless the parent
> directory is locked. When it isn't, have the code make a copy of the
> d_name while holding the d_lock.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Ben England <bengland@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/ceph/mds_client.c | 60 ++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 49 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 747ac3585519..4817efaa7def 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2168,10 +2168,38 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *base,
>   	return path;
>   }
>   
> +/* Duplicate the dentry->d_name.name safely */
> +static int clone_dentry_name(struct dentry *dentry, const char **ppath,
> +			     int *ppathlen)
> +{
> +	u32 len;
> +	char *name;
> +retry:
> +	len = READ_ONCE(dentry->d_name.len);
> +	name = kmalloc(len + 1, GFP_NOFS);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	spin_lock(&dentry->d_lock);
> +	if (dentry->d_name.len != len) {
> +		spin_unlock(&dentry->d_lock);
> +		kfree(name);
> +		goto retry;
> +	}
> +	memcpy(name, dentry->d_name.name, len);
> +	spin_unlock(&dentry->d_lock);
> +
> +	name[len] = '\0';
> +	*ppath = name;
> +	*ppathlen = len;
> +	return 0;
> +}
> +
>   static int build_dentry_path(struct dentry *dentry, struct inode *dir,
>   			     const char **ppath, int *ppathlen, u64 *pino,
> -			     int *pfreepath)
> +			     bool *pfreepath, bool parent_locked)
>   {
> +	int ret;
>   	char *path;
>   
>   	rcu_read_lock();
> @@ -2180,8 +2208,15 @@ static int build_dentry_path(struct dentry *dentry, struct inode *dir,
>   	if (dir && ceph_snap(dir) == CEPH_NOSNAP) {
>   		*pino = ceph_ino(dir);
>   		rcu_read_unlock();
> -		*ppath = dentry->d_name.name;
> -		*ppathlen = dentry->d_name.len;
> +		if (parent_locked) {
> +			*ppath = dentry->d_name.name;
> +			*ppathlen = dentry->d_name.len;
> +		} else {
> +			ret = clone_dentry_name(dentry, ppath, ppathlen);
> +			if (ret)
> +				return ret;
> +			*pfreepath = true;
> +		}
>   		return 0;
>   	}
>   	rcu_read_unlock();
> @@ -2189,13 +2224,13 @@ static int build_dentry_path(struct dentry *dentry, struct inode *dir,
>   	if (IS_ERR(path))
>   		return PTR_ERR(path);
>   	*ppath = path;
> -	*pfreepath = 1;
> +	*pfreepath = true;
>   	return 0;
>   }
>   
>   static int build_inode_path(struct inode *inode,
>   			    const char **ppath, int *ppathlen, u64 *pino,
> -			    int *pfreepath)
> +			    bool *pfreepath)
>   {
>   	struct dentry *dentry;
>   	char *path;
> @@ -2211,7 +2246,7 @@ static int build_inode_path(struct inode *inode,
>   	if (IS_ERR(path))
>   		return PTR_ERR(path);
>   	*ppath = path;
> -	*pfreepath = 1;
> +	*pfreepath = true;
>   	return 0;
>   }
>   
> @@ -2222,7 +2257,7 @@ static int build_inode_path(struct inode *inode,
>   static int set_request_path_attr(struct inode *rinode, struct dentry *rdentry,
>   				  struct inode *rdiri, const char *rpath,
>   				  u64 rino, const char **ppath, int *pathlen,
> -				  u64 *ino, int *freepath)
> +				  u64 *ino, bool *freepath, bool parent_locked)
>   {
>   	int r = 0;
>   
> @@ -2232,7 +2267,7 @@ static int set_request_path_attr(struct inode *rinode, struct dentry *rdentry,
>   		     ceph_snap(rinode));
>   	} else if (rdentry) {
>   		r = build_dentry_path(rdentry, rdiri, ppath, pathlen, ino,
> -					freepath);
> +					freepath, parent_locked);
>   		dout(" dentry %p %llx/%.*s\n", rdentry, *ino, *pathlen,
>   		     *ppath);
>   	} else if (rpath || rino) {
> @@ -2258,7 +2293,7 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc,
>   	const char *path2 = NULL;
>   	u64 ino1 = 0, ino2 = 0;
>   	int pathlen1 = 0, pathlen2 = 0;
> -	int freepath1 = 0, freepath2 = 0;
> +	bool freepath1 = false, freepath2 = false;
>   	int len;
>   	u16 releases;
>   	void *p, *end;
> @@ -2266,16 +2301,19 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc,
>   
>   	ret = set_request_path_attr(req->r_inode, req->r_dentry,
>   			      req->r_parent, req->r_path1, req->r_ino1.ino,
> -			      &path1, &pathlen1, &ino1, &freepath1);
> +			      &path1, &pathlen1, &ino1, &freepath1,
> +			      test_bit(CEPH_MDS_R_PARENT_LOCKED,
> +					&req->r_req_flags));
>   	if (ret < 0) {
>   		msg = ERR_PTR(ret);
>   		goto out;
>   	}
>   
> +	/* If r_old_dentry is set, then assume that its parent is locked */
>   	ret = set_request_path_attr(NULL, req->r_old_dentry,
>   			      req->r_old_dentry_dir,
>   			      req->r_path2, req->r_ino2.ino,
> -			      &path2, &pathlen2, &ino2, &freepath2);
> +			      &path2, &pathlen2, &ino2, &freepath2, true);
>   	if (ret < 0) {
>   		msg = ERR_PTR(ret);
>   		goto out_free1;
> 

Reviewed-by: "Yan, Zheng" <zyan@redhat.com>

Patch
diff mbox series

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 747ac3585519..4817efaa7def 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2168,10 +2168,38 @@  char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *base,
 	return path;
 }
 
+/* Duplicate the dentry->d_name.name safely */
+static int clone_dentry_name(struct dentry *dentry, const char **ppath,
+			     int *ppathlen)
+{
+	u32 len;
+	char *name;
+retry:
+	len = READ_ONCE(dentry->d_name.len);
+	name = kmalloc(len + 1, GFP_NOFS);
+	if (!name)
+		return -ENOMEM;
+
+	spin_lock(&dentry->d_lock);
+	if (dentry->d_name.len != len) {
+		spin_unlock(&dentry->d_lock);
+		kfree(name);
+		goto retry;
+	}
+	memcpy(name, dentry->d_name.name, len);
+	spin_unlock(&dentry->d_lock);
+
+	name[len] = '\0';
+	*ppath = name;
+	*ppathlen = len;
+	return 0;
+}
+
 static int build_dentry_path(struct dentry *dentry, struct inode *dir,
 			     const char **ppath, int *ppathlen, u64 *pino,
-			     int *pfreepath)
+			     bool *pfreepath, bool parent_locked)
 {
+	int ret;
 	char *path;
 
 	rcu_read_lock();
@@ -2180,8 +2208,15 @@  static int build_dentry_path(struct dentry *dentry, struct inode *dir,
 	if (dir && ceph_snap(dir) == CEPH_NOSNAP) {
 		*pino = ceph_ino(dir);
 		rcu_read_unlock();
-		*ppath = dentry->d_name.name;
-		*ppathlen = dentry->d_name.len;
+		if (parent_locked) {
+			*ppath = dentry->d_name.name;
+			*ppathlen = dentry->d_name.len;
+		} else {
+			ret = clone_dentry_name(dentry, ppath, ppathlen);
+			if (ret)
+				return ret;
+			*pfreepath = true;
+		}
 		return 0;
 	}
 	rcu_read_unlock();
@@ -2189,13 +2224,13 @@  static int build_dentry_path(struct dentry *dentry, struct inode *dir,
 	if (IS_ERR(path))
 		return PTR_ERR(path);
 	*ppath = path;
-	*pfreepath = 1;
+	*pfreepath = true;
 	return 0;
 }
 
 static int build_inode_path(struct inode *inode,
 			    const char **ppath, int *ppathlen, u64 *pino,
-			    int *pfreepath)
+			    bool *pfreepath)
 {
 	struct dentry *dentry;
 	char *path;
@@ -2211,7 +2246,7 @@  static int build_inode_path(struct inode *inode,
 	if (IS_ERR(path))
 		return PTR_ERR(path);
 	*ppath = path;
-	*pfreepath = 1;
+	*pfreepath = true;
 	return 0;
 }
 
@@ -2222,7 +2257,7 @@  static int build_inode_path(struct inode *inode,
 static int set_request_path_attr(struct inode *rinode, struct dentry *rdentry,
 				  struct inode *rdiri, const char *rpath,
 				  u64 rino, const char **ppath, int *pathlen,
-				  u64 *ino, int *freepath)
+				  u64 *ino, bool *freepath, bool parent_locked)
 {
 	int r = 0;
 
@@ -2232,7 +2267,7 @@  static int set_request_path_attr(struct inode *rinode, struct dentry *rdentry,
 		     ceph_snap(rinode));
 	} else if (rdentry) {
 		r = build_dentry_path(rdentry, rdiri, ppath, pathlen, ino,
-					freepath);
+					freepath, parent_locked);
 		dout(" dentry %p %llx/%.*s\n", rdentry, *ino, *pathlen,
 		     *ppath);
 	} else if (rpath || rino) {
@@ -2258,7 +2293,7 @@  static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc,
 	const char *path2 = NULL;
 	u64 ino1 = 0, ino2 = 0;
 	int pathlen1 = 0, pathlen2 = 0;
-	int freepath1 = 0, freepath2 = 0;
+	bool freepath1 = false, freepath2 = false;
 	int len;
 	u16 releases;
 	void *p, *end;
@@ -2266,16 +2301,19 @@  static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc,
 
 	ret = set_request_path_attr(req->r_inode, req->r_dentry,
 			      req->r_parent, req->r_path1, req->r_ino1.ino,
-			      &path1, &pathlen1, &ino1, &freepath1);
+			      &path1, &pathlen1, &ino1, &freepath1,
+			      test_bit(CEPH_MDS_R_PARENT_LOCKED,
+					&req->r_req_flags));
 	if (ret < 0) {
 		msg = ERR_PTR(ret);
 		goto out;
 	}
 
+	/* If r_old_dentry is set, then assume that its parent is locked */
 	ret = set_request_path_attr(NULL, req->r_old_dentry,
 			      req->r_old_dentry_dir,
 			      req->r_path2, req->r_ino2.ino,
-			      &path2, &pathlen2, &ino2, &freepath2);
+			      &path2, &pathlen2, &ino2, &freepath2, true);
 	if (ret < 0) {
 		msg = ERR_PTR(ret);
 		goto out_free1;