[Cluster-devel] [PATCH 0/4] GFS2: Enforce gl_object integrity

[Cluster-devel] [PATCH 0/4] GFS2: Enforce gl_object integrity

[Bob Peterson]

This is a four patch set that tries to ensure the integrity of the
glock field gl_object. This is vital to guard against metadata corruption
because the glock ops (glops.c) often use gl_object as a starting point.
For example, when you lock an inode glock, inode_go_lock sets
ip = gl_object and makes decisions from that value. Function
inode_go_sync does a similar thing.

The problem is that gfs2 was sometimes setting and clearing gl_object
without any kind of locking. There were other places where it failed
to clear the value in error cases. Processes would indescriminately
overwrite the values of other processes, causing chaos.

The first patch introduces a new helper to clear gl_object.
This complements the function recently added to set gl_object.

The second patch moves the setting of gl_object after the block type
check in function gfs2_inode_lookup. This prevents corruption that
sometimes occurred when inode blocks were deleted and reused.

The third patch adds calls to clear gl_object if gfs2_create_inode
fails so residual values aren't left over in the glock.

The fourth patch adds calls to clear gl_object during the inode evict
process. The value must be freed prior to actually freeing the block
in the bitmap because the block could be reused for a different inode,
but the glock can remain in core. If the object is evicted but not
deleted, it must also be cleared, but in a different code path.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
Bob Peterson (4):
  GFS2: Introduce helper for clearing gl_object
  GFS2: Set gl_object in inode lookup only after block type check
  GFS2: Clear gl_object if gfs2_create_inode fails
  GFS2: Clear gl_object when deleting an inode in gfs2_delete_inode

 fs/gfs2/glock.h | 34 ++++++++++++++++++++++++++++++++++
 fs/gfs2/inode.c | 11 +++++++----
 fs/gfs2/super.c | 15 ++++++++++++---
 3 files changed, 53 insertions(+), 7 deletions(-)

-- 
2.13.3

[Cluster-devel] [PATCH 1/4] GFS2: Introduce helper for clearing gl_object

[Bob Peterson]

This patch introduces a new helper function in glock.h that
clears gl_object, with an added integrity check. An additional
integrity check has been added to glock_set_object, plus comments.
This is step 1 in a series to ensure gl_object integrity.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.h | 34 ++++++++++++++++++++++++++++++++++
 fs/gfs2/inode.c |  4 ++--
 fs/gfs2/super.c |  4 ++--
 3 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index 9ad4a6ac6c84..526d2123f758 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -13,6 +13,7 @@
 #include <linux/sched.h>
 #include <linux/parser.h>
 #include "incore.h"
+#include "util.h"
 
 /* Options for hostdata parser */
 
@@ -257,11 +258,44 @@ static inline bool gfs2_holder_initialized(struct gfs2_holder *gh)
 	return gh->gh_gl;
 }
 
+/**
+ * glock_set_object - set the gl_object field of a glock
+ * @gl: the glock
+ * @object: the object
+ */
 static inline void glock_set_object(struct gfs2_glock *gl, void *object)
 {
 	spin_lock(&gl->gl_lockref.lock);
+	if (gfs2_assert_warn(gl->gl_name.ln_sbd, gl->gl_object == NULL))
+		gfs2_dump_glock(NULL, gl);
 	gl->gl_object = object;
 	spin_unlock(&gl->gl_lockref.lock);
 }
 
+/**
+ * glock_clear_object - clear the gl_object field of a glock
+ * @gl: the glock
+ * @object: the object
+ *
+ * I'd love to similarly add this:
+ *	else if (gfs2_assert_warn(gl->gl_sbd, gl->gl_object == object))
+ *		gfs2_dump_glock(NULL, gl);
+ * Unfortunately, that's not possible because as soon as gfs2_delete_inode
+ * frees the block in the rgrp, another process can reassign it for an I_NEW
+ * inode in gfs2_create_inode because that calls new_inode, not gfs2_iget.
+ * That means gfs2_delete_inode may subsequently try to call this function
+ * for a glock that's already pointing to a brand new inode. If we clear the
+ * new inode's gl_object, we'll introduce metadata corruption. Function
+ * gfs2_delete_inode calls clear_inode which calls gfs2_clear_inode which also
+ * tries to clear gl_object, so it's more than just gfs2_delete_inode.
+ *
+ */
+static inline void glock_clear_object(struct gfs2_glock *gl, void *object)
+{
+	spin_lock(&gl->gl_lockref.lock);
+	if (gl->gl_object == object)
+		gl->gl_object = NULL;
+	spin_unlock(&gl->gl_lockref.lock);
+}
+
 #endif /* __GLOCK_DOT_H__ */
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index acca501f8110..608e4bf60754 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -202,14 +202,14 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
 
 fail_refresh:
 	ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
-	glock_set_object(ip->i_iopen_gh.gh_gl, NULL);
+	glock_clear_object(ip->i_iopen_gh.gh_gl, ip);
 	gfs2_glock_dq_uninit(&ip->i_iopen_gh);
 fail_put:
 	if (io_gl)
 		gfs2_glock_put(io_gl);
 	if (gfs2_holder_initialized(&i_gh))
 		gfs2_glock_dq_uninit(&i_gh);
-	glock_set_object(ip->i_gl, NULL);
+	glock_clear_object(ip->i_gl, ip);
 fail:
 	iget_failed(inode);
 	return ERR_PTR(error);
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index fdedec379b78..5fdc54158ff6 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1640,13 +1640,13 @@ static void gfs2_evict_inode(struct inode *inode)
 	gfs2_ordered_del_inode(ip);
 	clear_inode(inode);
 	gfs2_dir_hash_inval(ip);
-	glock_set_object(ip->i_gl, NULL);
+	glock_clear_object(ip->i_gl, ip);
 	wait_on_bit_io(&ip->i_flags, GIF_GLOP_PENDING, TASK_UNINTERRUPTIBLE);
 	gfs2_glock_add_to_lru(ip->i_gl);
 	gfs2_glock_put(ip->i_gl);
 	ip->i_gl = NULL;
 	if (gfs2_holder_initialized(&ip->i_iopen_gh)) {
-		glock_set_object(ip->i_iopen_gh.gh_gl, NULL);
+		glock_clear_object(ip->i_iopen_gh.gh_gl, ip);
 		ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
 		gfs2_glock_dq_uninit(&ip->i_iopen_gh);
 	}
-- 
2.13.3

[Cluster-devel] [PATCH 1/4] GFS2: Introduce helper for clearing gl_object

[Andreas Gruenbacher]

Bob,

the following cleanup is needed to avoid spilling the syslog with false
warnings.

Thanks,
Andreas

---
 fs/gfs2/rgrp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 836e38ba5d0a..b9ae787b52c7 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -705,7 +705,7 @@ void gfs2_clear_rgrpd(struct gfs2_sbd *sdp)
 		rb_erase(n, &sdp->sd_rindex_tree);
 
 		if (gl) {
-			glock_set_object(gl, NULL);
+			glock_clear_object(gl, rgd);
 			gfs2_glock_add_to_lru(gl);
 			gfs2_glock_put(gl);
 		}
-- 
2.13.3

[Cluster-devel] [PATCH 1/4] GFS2: Introduce helper for clearing gl_object

[Bob Peterson]

----- Original Message -----
| Bob,
| 
| the following cleanup is needed to avoid spilling the syslog with false
| warnings.
| 
| Thanks,
| Andreas
Hi,

Thanks. This is now pushed to the for-next branch of the linux-gfs2 tree:
https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/commit/fs/gfs2?h=for-next&id=7023a0b16f66a2f1358c95989d23142d8191fd6e

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 2/4] GFS2: Set gl_object in inode lookup only after block type check

[Bob Peterson]

Before this patch, the inode glock's gl_object was set after a
reference was acquired, but before the block type was verified.
In cases where the block was unlinked, then freed and reused on
another node, a residule delete callback (delete_work) would try
to look up the inode, eventually failing the block check, but
only after it overwrites gl_object with a pointer to the wrong
inode. This patch moves the assignment of gl_object after the
block check so it won't be improperly overwritten.

Likewise, at the end of the function, gfs2_inode_lookup was
clearing gl_object, even in cases where it wasn't set, such as
when the block type check fails. The patch only clears it if
actually set it.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/inode.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 608e4bf60754..69f66e83920a 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -145,7 +145,6 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
 		if (unlikely(error))
 			goto fail;
 		flush_delayed_work(&ip->i_gl->gl_work);
-		glock_set_object(ip->i_gl, ip);
 
 		error = gfs2_glock_get(sdp, no_addr, &gfs2_iopen_glops, CREATE, &io_gl);
 		if (unlikely(error))
@@ -170,6 +169,7 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
 			}
 		}
 
+		glock_set_object(ip->i_gl, ip);
 		set_bit(GIF_INVALID, &ip->i_flags);
 		error = gfs2_glock_nq_init(io_gl, LM_ST_SHARED, GL_EXACT, &ip->i_iopen_gh);
 		if (unlikely(error))
@@ -207,9 +207,10 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
 fail_put:
 	if (io_gl)
 		gfs2_glock_put(io_gl);
-	if (gfs2_holder_initialized(&i_gh))
+	if (gfs2_holder_initialized(&i_gh)) {
+		glock_clear_object(ip->i_gl, ip);
 		gfs2_glock_dq_uninit(&i_gh);
-	glock_clear_object(ip->i_gl, ip);
+	}
 fail:
 	iget_failed(inode);
 	return ERR_PTR(error);
-- 
2.13.3

[Cluster-devel] [PATCH 2/4] GFS2: Set gl_object in inode lookup only after block type check

[Bob Peterson]

Hi,

The second half of this patch isn't quite right. I'll rework it
and send a replacement.

Bob Peterson

----- Original Message -----
| Before this patch, the inode glock's gl_object was set after a
| reference was acquired, but before the block type was verified.
| In cases where the block was unlinked, then freed and reused on
| another node, a residule delete callback (delete_work) would try
| to look up the inode, eventually failing the block check, but
| only after it overwrites gl_object with a pointer to the wrong
| inode. This patch moves the assignment of gl_object after the
| block check so it won't be improperly overwritten.
| 
| Likewise, at the end of the function, gfs2_inode_lookup was
| clearing gl_object, even in cases where it wasn't set, such as
| when the block type check fails. The patch only clears it if
| actually set it.
| 
| Signed-off-by: Bob Peterson <rpeterso@redhat.com>
| ---
|  fs/gfs2/inode.c | 7 ++++---
|  1 file changed, 4 insertions(+), 3 deletions(-)
| 
| diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
| index 608e4bf60754..69f66e83920a 100644
| --- a/fs/gfs2/inode.c
| +++ b/fs/gfs2/inode.c
| @@ -145,7 +145,6 @@ struct inode *gfs2_inode_lookup(struct super_block *sb,
| unsigned int type,
|  		if (unlikely(error))
|  			goto fail;
|  		flush_delayed_work(&ip->i_gl->gl_work);
| -		glock_set_object(ip->i_gl, ip);
|  
|  		error = gfs2_glock_get(sdp, no_addr, &gfs2_iopen_glops, CREATE, &io_gl);
|  		if (unlikely(error))
| @@ -170,6 +169,7 @@ struct inode *gfs2_inode_lookup(struct super_block *sb,
| unsigned int type,
|  			}
|  		}
|  
| +		glock_set_object(ip->i_gl, ip);
|  		set_bit(GIF_INVALID, &ip->i_flags);
|  		error = gfs2_glock_nq_init(io_gl, LM_ST_SHARED, GL_EXACT,
|  		&ip->i_iopen_gh);
|  		if (unlikely(error))
| @@ -207,9 +207,10 @@ struct inode *gfs2_inode_lookup(struct super_block *sb,
| unsigned int type,
|  fail_put:
|  	if (io_gl)
|  		gfs2_glock_put(io_gl);
| -	if (gfs2_holder_initialized(&i_gh))
| +	if (gfs2_holder_initialized(&i_gh)) {
| +		glock_clear_object(ip->i_gl, ip);
|  		gfs2_glock_dq_uninit(&i_gh);
| -	glock_clear_object(ip->i_gl, ip);
| +	}
|  fail:
|  	iget_failed(inode);
|  	return ERR_PTR(error);
| --
| 2.13.3
| 
| 

[Cluster-devel] [PATCH 3/4] GFS2: Clear gl_object if gfs2_create_inode fails

[Bob Peterson]

If function gfs2_create_inode fails after the inode has been
created (for example, if the inode_refresh fails for some reason)
the function was setting gl_object but never clearing it again.
The glocks are left pointing to a freed inode. This patch adds
the calls to clear gl_object in the appropriate error paths.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/inode.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 69f66e83920a..e366c634e22d 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -776,11 +776,13 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
 	return error;
 
 fail_gunlock3:
+	glock_clear_object(io_gl, ip);
 	gfs2_glock_dq_uninit(&ip->i_iopen_gh);
 	gfs2_glock_put(io_gl);
 fail_gunlock2:
 	if (io_gl)
 		clear_bit(GLF_INODE_CREATING, &io_gl->gl_flags);
+	glock_clear_object(ip->i_gl, ip);
 fail_free_inode:
 	if (ip->i_gl)
 		gfs2_glock_put(ip->i_gl);
-- 
2.13.3

[Cluster-devel] [PATCH 4/4] GFS2: Clear gl_object when deleting an inode in gfs2_delete_inode

[Bob Peterson]

This patch adds some calls to clear gl_object in function
gfs2_delete_inode. Since we are deleting the inode, and the glock
typically outlives the inode in core, we must clear gl_object
so subsequent use of the glock (e.g. for a new inode in its place)
will not have the old pointer sitting there. In error cases we
need to tidy up after ourselves. In non-error cases, we need to
clear gl_object before we set the block free in the bitmap so
residules aren't left for potential inode creators.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/super.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 5fdc54158ff6..87271a859a8d 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1547,6 +1547,7 @@ static void gfs2_evict_inode(struct inode *inode)
 	/* Must not read inode block until block type has been verified */
 	error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, &gh);
 	if (unlikely(error)) {
+		glock_clear_object(ip->i_iopen_gh.gh_gl, ip);
 		ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
 		gfs2_glock_dq_uninit(&ip->i_iopen_gh);
 		goto out;
@@ -1595,6 +1596,11 @@ static void gfs2_evict_inode(struct inode *inode)
 			goto out_unlock;
 	}
 
+	/* We're about to clear the bitmap for the dinode, but as soon as we
+	   do, gfs2_create_inode can create another inode at the same block
+	   location and try to set gl_object again. We clear gl_object here so
+	   that subsequent inode creates don't see an old gl_object. */
+	glock_clear_object(ip->i_gl, ip);
 	error = gfs2_dinode_dealloc(ip);
 	goto out_unlock;
 
@@ -1623,14 +1629,17 @@ static void gfs2_evict_inode(struct inode *inode)
 		gfs2_rs_deltree(&ip->i_res);
 
 	if (gfs2_holder_initialized(&ip->i_iopen_gh)) {
+		glock_clear_object(ip->i_iopen_gh.gh_gl, ip);
 		if (test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
 			ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
 			gfs2_glock_dq(&ip->i_iopen_gh);
 		}
 		gfs2_holder_uninit(&ip->i_iopen_gh);
 	}
-	if (gfs2_holder_initialized(&gh))
+	if (gfs2_holder_initialized(&gh)) {
+		glock_clear_object(ip->i_gl, ip);
 		gfs2_glock_dq_uninit(&gh);
+	}
 	if (error && error != GLR_TRYFAILED && error != -EROFS)
 		fs_warn(sdp, "gfs2_evict_inode: %d\n", error);
 out:
-- 
2.13.3