[PATCH v2] ceph: fix recursively call between ceph_set_acl and __ceph_setattr

[PATCH v2] ceph: fix recursively call between ceph_set_acl and __ceph_setattr

[Yan, Zheng]

ceph_set_acl() calls __ceph_setattr() if the setacl operation needs
to modify inode's i_mode. __ceph_setattr() updates inode's i_mode,
then calls posix_acl_chmod().

The problem is that __ceph_setattr() calls posix_acl_chmod() before
sending the setattr request. The get_acl() call in posix_acl_chmod()
can trigger a getxattr request. The reply of the getxattr request
can restore inode's i_mode to its old value. The set_acl() call in
posix_acl_chmod() sees old value of inode's i_mode, so it calls
__ceph_setattr() again.

Link: http://tracker.ceph.com/issues/19688
Reported-by: Jerry Lee <leisurelysw24@gmail.com>
Tested-by: Luis Henriques <lhenriques@suse.com>
Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
---
 fs/ceph/inode.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index efee88c..976fd3a 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -2078,11 +2078,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
 	if (inode_dirty_flags)
 		__mark_inode_dirty(inode, inode_dirty_flags);
 
-	if (ia_valid & ATTR_MODE) {
-		err = posix_acl_chmod(inode, attr->ia_mode);
-		if (err)
-			goto out_put;
-	}
 
 	if (mask) {
 		req->r_inode = inode;
@@ -2096,13 +2091,13 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
 	     ceph_cap_string(dirtied), mask);
 
 	ceph_mdsc_put_request(req);
-	if (mask & CEPH_SETATTR_SIZE)
-		__ceph_do_pending_vmtruncate(inode);
-	ceph_free_cap_flush(prealloc_cf);
-	return err;
-out_put:
-	ceph_mdsc_put_request(req);
 	ceph_free_cap_flush(prealloc_cf);
+
+	if (err >= 0) {
+		if (mask & CEPH_SETATTR_SIZE) {
+			__ceph_do_pending_vmtruncate(inode);
+		}
+	}
 	return err;
 }
 
@@ -2121,7 +2116,12 @@ int ceph_setattr(struct dentry *dentry, struct iattr *attr)
 	if (err != 0)
 		return err;
 
-	return __ceph_setattr(inode, attr);
+	err = __ceph_setattr(inode, attr);
+
+	if (err >= 0 && (attr->ia_valid & ATTR_MODE)) {
+		err = posix_acl_chmod(inode, attr->ia_mode);
+	}
+	return err;
 }
 
 /*
-- 
2.9.3

Re: [PATCH v2] ceph: fix recursively call between ceph_set_acl and __ceph_setattr

[Jeff Layton]

On Wed, 2017-04-19 at 18:36 +0800, Yan, Zheng wrote:
> ceph_set_acl() calls __ceph_setattr() if the setacl operation needs
> to modify inode's i_mode. __ceph_setattr() updates inode's i_mode,
> then calls posix_acl_chmod().
> 
> The problem is that __ceph_setattr() calls posix_acl_chmod() before
> sending the setattr request. The get_acl() call in posix_acl_chmod()
> can trigger a getxattr request. The reply of the getxattr request
> can restore inode's i_mode to its old value. The set_acl() call in
> posix_acl_chmod() sees old value of inode's i_mode, so it calls
> __ceph_setattr() again.
> 
> Link: http://tracker.ceph.com/issues/19688
> Reported-by: Jerry Lee <leisurelysw24@gmail.com>
> Tested-by: Luis Henriques <lhenriques@suse.com>
> Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
> ---
>  fs/ceph/inode.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index efee88c..976fd3a 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -2078,11 +2078,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
>  	if (inode_dirty_flags)
>  		__mark_inode_dirty(inode, inode_dirty_flags);
>  
> -	if (ia_valid & ATTR_MODE) {
> -		err = posix_acl_chmod(inode, attr->ia_mode);
> -		if (err)
> -			goto out_put;
> -	}
>  
>  	if (mask) {
>  		req->r_inode = inode;
> @@ -2096,13 +2091,13 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
>  	     ceph_cap_string(dirtied), mask);
>  
>  	ceph_mdsc_put_request(req);
> -	if (mask & CEPH_SETATTR_SIZE)
> -		__ceph_do_pending_vmtruncate(inode);
> -	ceph_free_cap_flush(prealloc_cf);
> -	return err;
> -out_put:
> -	ceph_mdsc_put_request(req);
>  	ceph_free_cap_flush(prealloc_cf);
> +
> +	if (err >= 0) {
> +		if (mask & CEPH_SETATTR_SIZE) {
> +			__ceph_do_pending_vmtruncate(inode);
> +		}
> +	}
>  	return err;
>  }
>  

nit: Could drop some of the curly braces and nesting above:

if (err >= 0 && (mask & CEPH_SETATTR_SIZE))
	__ceph_do_pending_vmtruncate(inode);


> @@ -2121,7 +2116,12 @@ int ceph_setattr(struct dentry *dentry, struct iattr *attr)
>  	if (err != 0)
>  		return err;
>  
> -	return __ceph_setattr(inode, attr);
> +	err = __ceph_setattr(inode, attr);
> +
> +	if (err >= 0 && (attr->ia_valid & ATTR_MODE)) {
> +		err = posix_acl_chmod(inode, attr->ia_mode);
> +	}
> +	return err;
>  }
>  
>  /*

Much better without the bool argument though :)

Reviewed-by: Jeff Layton <jlayton@redhat.com>

Re: [PATCH v2] ceph: fix recursively call between ceph_set_acl and __ceph_setattr

[Luis Henriques]

On Wed, Apr 19, 2017 at 06:36:22PM +0800, Yan, Zheng wrote:
> ceph_set_acl() calls __ceph_setattr() if the setacl operation needs
> to modify inode's i_mode. __ceph_setattr() updates inode's i_mode,
> then calls posix_acl_chmod().
> 
> The problem is that __ceph_setattr() calls posix_acl_chmod() before
> sending the setattr request. The get_acl() call in posix_acl_chmod()
> can trigger a getxattr request. The reply of the getxattr request
> can restore inode's i_mode to its old value. The set_acl() call in
> posix_acl_chmod() sees old value of inode's i_mode, so it calls
> __ceph_setattr() again.
> 
> Link: http://tracker.ceph.com/issues/19688
> Reported-by: Jerry Lee <leisurelysw24@gmail.com>
> Tested-by: Luis Henriques <lhenriques@suse.com>
> Signed-off-by: "Yan, Zheng" <zyan@redhat.com>

An additional comment is that this should be tagged for stable kernels.

Cheers,
--
Luís

> ---
>  fs/ceph/inode.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index efee88c..976fd3a 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -2078,11 +2078,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
>  	if (inode_dirty_flags)
>  		__mark_inode_dirty(inode, inode_dirty_flags);
>  
> -	if (ia_valid & ATTR_MODE) {
> -		err = posix_acl_chmod(inode, attr->ia_mode);
> -		if (err)
> -			goto out_put;
> -	}
>  
>  	if (mask) {
>  		req->r_inode = inode;
> @@ -2096,13 +2091,13 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
>  	     ceph_cap_string(dirtied), mask);
>  
>  	ceph_mdsc_put_request(req);
> -	if (mask & CEPH_SETATTR_SIZE)
> -		__ceph_do_pending_vmtruncate(inode);
> -	ceph_free_cap_flush(prealloc_cf);
> -	return err;
> -out_put:
> -	ceph_mdsc_put_request(req);
>  	ceph_free_cap_flush(prealloc_cf);
> +
> +	if (err >= 0) {
> +		if (mask & CEPH_SETATTR_SIZE) {
> +			__ceph_do_pending_vmtruncate(inode);
> +		}
> +	}
>  	return err;
>  }
>  
> @@ -2121,7 +2116,12 @@ int ceph_setattr(struct dentry *dentry, struct iattr *attr)
>  	if (err != 0)
>  		return err;
>  
> -	return __ceph_setattr(inode, attr);
> +	err = __ceph_setattr(inode, attr);
> +
> +	if (err >= 0 && (attr->ia_valid & ATTR_MODE)) {
> +		err = posix_acl_chmod(inode, attr->ia_mode);
> +	}
> +	return err;
>  }
>  
>  /*
> -- 
> 2.9.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html