[Cluster-devel] [PATCH] gfs2: Allow gfs2_xattr_set to be called with the glock held

[Cluster-devel] [PATCH] gfs2: Allow gfs2_xattr_set to be called with the glock held

[Andreas Gruenbacher]

On the following call path:

  gfs2_setattr -> setattr_prepare -> ... ->
    cap_inode_killpriv -> ... ->
      gfs2_xattr_set

the glock is locked in gfs2_setattr, so check for recursive locking in
gfs2_xattr_set as gfs2_xattr_get already does.  While at it, get rid of
need_unlock in gfs2_xattr_get.

Fixes xfstest generic/093.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/xattr.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
index 588d4f21a273..d9925e0057d9 100644
--- a/fs/gfs2/xattr.c
+++ b/fs/gfs2/xattr.c
@@ -616,7 +616,6 @@ static int gfs2_xattr_get(const struct xattr_handler *handler,
 {
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_holder gh;
-	bool need_unlock = false;
 	int ret;
 
 	/* During lookup, SELinux calls this function with the glock locked. */
@@ -625,10 +624,11 @@ static int gfs2_xattr_get(const struct xattr_handler *handler,
 		ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, LM_FLAG_ANY, &gh);
 		if (ret)
 			return ret;
-		need_unlock = true;
+	} else {
+		gfs2_holder_mark_uninitialized(&gh);
 	}
 	ret = __gfs2_xattr_get(inode, name, buffer, size, handler->flags);
-	if (need_unlock)
+	if (gfs2_holder_initialized(&gh))
 		gfs2_glock_dq_uninit(&gh);
 	return ret;
 }
@@ -1250,11 +1250,20 @@ static int gfs2_xattr_set(const struct xattr_handler *handler,
 	if (ret)
 		return ret;
 
-	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &gh);
-	if (ret)
-		return ret;
+	/* May be called from gfs_setattr with the glock locked. */
+
+	if (!gfs2_glock_is_locked_by_me(ip->i_gl)) {
+		ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &gh);
+		if (ret)
+			return ret;
+	} else {
+		if (WARN_ON_ONCE(ip->i_gl->gl_state != LM_ST_EXCLUSIVE))
+			return -EIO;
+		gfs2_holder_mark_uninitialized(&gh);
+	}
 	ret = __gfs2_xattr_set(inode, name, value, size, flags, handler->flags);
-	gfs2_glock_dq_uninit(&gh);
+	if (gfs2_holder_initialized(&gh))
+		gfs2_glock_dq_uninit(&gh);
 	return ret;
 }
 
-- 
2.13.6

[Cluster-devel] [PATCH] gfs2: Allow gfs2_xattr_set to be called with the glock held

[Abhijith Das]

Hi,

Looks good. ACK

Cheers!
--Abhi

----- Original Message -----
> From: "Andreas Gruenbacher" <agruenba@redhat.com>
> To: cluster-devel at redhat.com
> Sent: Thursday, October 12, 2017 7:04:00 PM
> Subject: [Cluster-devel] [PATCH] gfs2: Allow gfs2_xattr_set to be called	with the glock held
> 
> On the following call path:
> 
>   gfs2_setattr -> setattr_prepare -> ... ->
>     cap_inode_killpriv -> ... ->
>       gfs2_xattr_set
> 
> the glock is locked in gfs2_setattr, so check for recursive locking in
> gfs2_xattr_set as gfs2_xattr_get already does.  While at it, get rid of
> need_unlock in gfs2_xattr_get.
> 
> Fixes xfstest generic/093.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/gfs2/xattr.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
> index 588d4f21a273..d9925e0057d9 100644
> --- a/fs/gfs2/xattr.c
> +++ b/fs/gfs2/xattr.c
> @@ -616,7 +616,6 @@ static int gfs2_xattr_get(const struct xattr_handler
> *handler,
>  {
>  	struct gfs2_inode *ip = GFS2_I(inode);
>  	struct gfs2_holder gh;
> -	bool need_unlock = false;
>  	int ret;
>  
>  	/* During lookup, SELinux calls this function with the glock locked. */
> @@ -625,10 +624,11 @@ static int gfs2_xattr_get(const struct xattr_handler
> *handler,
>  		ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, LM_FLAG_ANY, &gh);
>  		if (ret)
>  			return ret;
> -		need_unlock = true;
> +	} else {
> +		gfs2_holder_mark_uninitialized(&gh);
>  	}
>  	ret = __gfs2_xattr_get(inode, name, buffer, size, handler->flags);
> -	if (need_unlock)
> +	if (gfs2_holder_initialized(&gh))
>  		gfs2_glock_dq_uninit(&gh);
>  	return ret;
>  }
> @@ -1250,11 +1250,20 @@ static int gfs2_xattr_set(const struct xattr_handler
> *handler,
>  	if (ret)
>  		return ret;
>  
> -	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &gh);
> -	if (ret)
> -		return ret;
> +	/* May be called from gfs_setattr with the glock locked. */
> +
> +	if (!gfs2_glock_is_locked_by_me(ip->i_gl)) {
> +		ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &gh);
> +		if (ret)
> +			return ret;
> +	} else {
> +		if (WARN_ON_ONCE(ip->i_gl->gl_state != LM_ST_EXCLUSIVE))
> +			return -EIO;
> +		gfs2_holder_mark_uninitialized(&gh);
> +	}
>  	ret = __gfs2_xattr_set(inode, name, value, size, flags, handler->flags);
> -	gfs2_glock_dq_uninit(&gh);
> +	if (gfs2_holder_initialized(&gh))
> +		gfs2_glock_dq_uninit(&gh);
>  	return ret;
>  }
>  
> --
> 2.13.6
> 
>