[Cluster-devel] [PATCH 1/3] fsck.gfs2: Fix segfault in build_and_check_metalist

[Cluster-devel] [PATCH 1/3] fsck.gfs2: Fix segfault in build_and_check_metalist

[Andrew Price]

In unlikely circumstances, indirect pointer corruption in a 'system'
inode's metadata tree can lead to the inode block state being marked as
'free' in pass1, which causes build_and_check_metalist() to be called in
pass 2. The pass has a NULL ->check_metalist function pointer and so a
segfault occurs when build_and_check_metalist attempts to call it.

Fix the segfault by calling ->check_metalist() only when it's not NULL.
This required some refactoring to make the extra level of if-nesting
easier to implement and read.

Resolves: rhbz#1487726

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/metawalk.c | 107 ++++++++++++++++++++-----------------------
 gfs2/fsck/metawalk.h |   1 +
 2 files changed, 50 insertions(+), 58 deletions(-)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index d256dd2f..5792be56 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -1210,6 +1210,51 @@ static void file_ra(struct gfs2_inode *ip, struct gfs2_buffer_head *bh,
 			      extlen * sdp->bsize, POSIX_FADV_WILLNEED);
 }
 
+static int do_check_metalist(struct gfs2_inode *ip, uint64_t block, int height,
+                             struct gfs2_buffer_head **bhp, struct metawalk_fxns *pass)
+{
+	int was_duplicate = 0;
+	int is_valid = 1;
+	int error;
+
+	if (pass->check_metalist == NULL)
+		return 0;
+
+	error = pass->check_metalist(ip, block, bhp, height, &is_valid,
+				     &was_duplicate, pass->private);
+	if (error == meta_error) {
+		stack;
+		log_info("\n");
+		log_info(_("Serious metadata error on block %"PRIu64" (0x%"PRIx64").\n"),
+		         block, block);
+		return error;
+	}
+	if (error == meta_skip_further) {
+		log_info("\n");
+		log_info(_("Unrecoverable metadata error on block %"PRIu64" (0x%"PRIx64")\n"),
+		         block, block);
+		log_info(_("Further metadata will be skipped.\n"));
+		return error;
+	}
+	if (!is_valid) {
+		log_debug("Skipping rejected block %"PRIu64" (0x%"PRIx64")\n", block, block);
+		if (pass->invalid_meta_is_fatal)
+			return meta_error;
+		return meta_skip_one;
+	}
+	if (was_duplicate) {
+		log_debug("Skipping duplicate %"PRIu64" (0x%"PRIx64")\n", block, block);
+		return meta_skip_one;
+	}
+	if (!valid_block_ip(ip, block)) {
+		log_debug("Skipping invalid block %"PRIu64" (0x%"PRIx64")\n", block, block);
+		if (pass->invalid_meta_is_fatal)
+			return meta_error;
+		return meta_skip_one;
+	}
+	return error;
+}
+
 /**
  * build_and_check_metalist - check a bunch of indirect blocks
  *                            This includes hash table blocks for directories
@@ -1229,8 +1274,8 @@ static int build_and_check_metalist(struct gfs2_inode *ip, osi_list_t *mlp,
 	osi_list_t *prev_list, *cur_list, *tmp;
 	int h, head_size, iblk_type;
 	uint64_t *ptr, block, *undoptr;
-	int error, was_duplicate, is_valid;
 	int maxptrs;
+	int error;
 
 	osi_list_add(&metabh->b_altlist, &mlp[0]);
 
@@ -1294,65 +1339,11 @@ static int build_and_check_metalist(struct gfs2_inode *ip, osi_list_t *mlp,
 					continue;
 
 				block = be64_to_cpu(*ptr);
-				was_duplicate = 0;
-				error = pass->check_metalist(ip, block, &nbh,
-							     h, &is_valid,
-							     &was_duplicate,
-							     pass->private);
-				/* check_metalist should hold any buffers
-				   it gets with "bread". */
-				if (error == meta_error) {
-					stack;
-					log_info(_("\nSerious metadata "
-						   "error on block %llu "
-						   "(0x%llx).\n"),
-						 (unsigned long long)block,
-						 (unsigned long long)block);
+				error = do_check_metalist(ip, block, h, &nbh, pass);
+				if (error == meta_error || error == meta_skip_further)
 					goto error_undo;
-				}
-				if (error == meta_skip_further) {
-					log_info(_("\nUnrecoverable metadata "
-						   "error on block %llu "
-						   "(0x%llx). Further metadata"
-						   " will be skipped.\n"),
-						 (unsigned long long)block,
-						 (unsigned long long)block);
-					goto error_undo;
-				}
-				if (!is_valid) {
-					log_debug( _("Skipping rejected block "
-						     "%llu (0x%llx)\n"),
-						   (unsigned long long)block,
-						   (unsigned long long)block);
-					if (pass->invalid_meta_is_fatal) {
-						error = meta_error;
-						goto error_undo;
-					}
-					continue;
-				}
-				/* Note that there's a special case in which
-				   we need to process the metadata block, even
-				   if it was a duplicate. That's for cases
-				   where we deleted the last reference as
-				   metadata. */
-				if (was_duplicate) {
-					log_debug( _("Skipping duplicate %llu "
-						     "(0x%llx)\n"),
-						   (unsigned long long)block,
-						   (unsigned long long)block);
+				if (error == meta_skip_one)
 					continue;
-				}
-				if (!valid_block_ip(ip, block)) {
-					log_debug( _("Skipping invalid block "
-						     "%lld (0x%llx)\n"),
-						   (unsigned long long)block,
-						   (unsigned long long)block);
-					if (pass->invalid_meta_is_fatal) {
-						error = meta_error;
-						goto error_undo;
-					}
-					continue;
-				}
 				if (!nbh)
 					nbh = bread(ip->i_sbd, block);
 				osi_list_add_prev(&nbh->b_altlist, cur_list);
diff --git a/gfs2/fsck/metawalk.h b/gfs2/fsck/metawalk.h
index 119efeed..b5a037a3 100644
--- a/gfs2/fsck/metawalk.h
+++ b/gfs2/fsck/metawalk.h
@@ -39,6 +39,7 @@ enum meta_check_rc {
 	meta_error = -1,
 	meta_is_good = 0,
 	meta_skip_further = 1,
+	meta_skip_one = 2,
 };
 
 /* metawalk_fxns: function pointers to check various parts of the fs
-- 
2.21.0

[Cluster-devel] [PATCH 2/3] fsck.gfs2: Retain context for indirect pointers in ->check_metalist

[Andrew Price]

When the pass->check_metalist() functions are called, the pointer is
thrown away and the function is just called with a block address,
meaning that ->check_metalist() is unable to fix the pointer itself if
it deems the block pointed to is garbage.

Add an iptr structure which collects together the inode, the metadata
block buffer and the offset of the pointer in that buffer. The iptr is
now passed to ->check_metadata instead of the separate inode and block
address arguments.

Resolves: rhbz#1487726

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/afterpass1_common.c |  6 ++--
 gfs2/fsck/afterpass1_common.h |  4 +--
 gfs2/fsck/fs_recovery.c       |  8 ++---
 gfs2/fsck/metawalk.c          | 55 ++++++++++++++++----------------
 gfs2/fsck/metawalk.h          | 15 +++++++--
 gfs2/fsck/pass1.c             | 59 ++++++++++++++++++-----------------
 gfs2/fsck/pass1b.c            | 22 +++++++------
 gfs2/fsck/pass2.c             |  8 +++--
 8 files changed, 98 insertions(+), 79 deletions(-)

diff --git a/gfs2/fsck/afterpass1_common.c b/gfs2/fsck/afterpass1_common.c
index b7476408..55a6c76b 100644
--- a/gfs2/fsck/afterpass1_common.c
+++ b/gfs2/fsck/afterpass1_common.c
@@ -175,10 +175,12 @@ int remove_dentry_from_dir(struct gfs2_sbd *sdp, uint64_t dir,
 	return error;
 }
 
-int delete_metadata(struct gfs2_inode *ip, uint64_t block,
-		    struct gfs2_buffer_head **bh, int h, int *is_valid,
+int delete_metadata(struct iptr iptr, struct gfs2_buffer_head **bh, int h, int *is_valid,
 		    int *was_duplicate, void *private)
 {
+	struct gfs2_inode *ip = iptr.ipt_ip;
+	uint64_t block = iptr_block(iptr);
+
 	*is_valid = 1;
 	*was_duplicate = 0;
 	return delete_block_if_notdup(ip, block, bh, _("metadata"),
diff --git a/gfs2/fsck/afterpass1_common.h b/gfs2/fsck/afterpass1_common.h
index 829828f7..74b913f3 100644
--- a/gfs2/fsck/afterpass1_common.h
+++ b/gfs2/fsck/afterpass1_common.h
@@ -2,9 +2,9 @@
 #define _AFTERPASS1_H
 
 #include "util.h"
+#include "metawalk.h"
 
-extern int delete_metadata(struct gfs2_inode *ip, uint64_t block,
-			   struct gfs2_buffer_head **bh, int h, int *is_valid,
+extern int delete_metadata(struct iptr iptr, struct gfs2_buffer_head **bh, int h, int *is_valid,
 			   int *was_duplicate, void *private);
 extern int delete_leaf(struct gfs2_inode *ip, uint64_t block, void *private);
 extern int delete_data(struct gfs2_inode *ip, uint64_t metablock,
diff --git a/gfs2/fsck/fs_recovery.c b/gfs2/fsck/fs_recovery.c
index 4c0932b8..84cd4cc9 100644
--- a/gfs2/fsck/fs_recovery.c
+++ b/gfs2/fsck/fs_recovery.c
@@ -636,11 +636,11 @@ static int rangecheck_jblock(struct gfs2_inode *ip, uint64_t block)
 	return meta_is_good;
 }
 
-static int rangecheck_jmeta(struct gfs2_inode *ip, uint64_t block,
-			       struct gfs2_buffer_head **bh, int h,
-			       int *is_valid, int *was_duplicate,
-			       void *private)
+static int rangecheck_jmeta(struct iptr iptr, struct gfs2_buffer_head **bh, int h,
+                            int *is_valid, int *was_duplicate, void *private)
 {
+	struct gfs2_inode *ip = iptr.ipt_ip;
+	uint64_t block = iptr_block(iptr);
 	int rc;
 
 	*bh = NULL;
diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index 5792be56..d8bc211c 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -1210,9 +1210,11 @@ static void file_ra(struct gfs2_inode *ip, struct gfs2_buffer_head *bh,
 			      extlen * sdp->bsize, POSIX_FADV_WILLNEED);
 }
 
-static int do_check_metalist(struct gfs2_inode *ip, uint64_t block, int height,
-                             struct gfs2_buffer_head **bhp, struct metawalk_fxns *pass)
+static int do_check_metalist(struct iptr iptr, int height, struct gfs2_buffer_head **bhp,
+                             struct metawalk_fxns *pass)
 {
+	struct gfs2_inode *ip = iptr.ipt_ip;
+	uint64_t block = iptr_block(iptr);
 	int was_duplicate = 0;
 	int is_valid = 1;
 	int error;
@@ -1220,7 +1222,7 @@ static int do_check_metalist(struct gfs2_inode *ip, uint64_t block, int height,
 	if (pass->check_metalist == NULL)
 		return 0;
 
-	error = pass->check_metalist(ip, block, bhp, height, &is_valid,
+	error = pass->check_metalist(iptr, bhp, height, &is_valid,
 				     &was_duplicate, pass->private);
 	if (error == meta_error) {
 		stack;
@@ -1270,10 +1272,11 @@ static int build_and_check_metalist(struct gfs2_inode *ip, osi_list_t *mlp,
 				    struct metawalk_fxns *pass)
 {
 	uint32_t height = ip->i_di.di_height;
-	struct gfs2_buffer_head *bh, *nbh, *metabh = ip->i_bh;
+	struct gfs2_buffer_head *metabh = ip->i_bh;
 	osi_list_t *prev_list, *cur_list, *tmp;
+	struct iptr iptr = { .ipt_ip = ip, 0};
 	int h, head_size, iblk_type;
-	uint64_t *ptr, block, *undoptr;
+	uint64_t *undoptr;
 	int maxptrs;
 	int error;
 
@@ -1313,39 +1316,37 @@ static int build_and_check_metalist(struct gfs2_inode *ip, osi_list_t *mlp,
 		prev_list = &mlp[h - 1];
 		cur_list = &mlp[h];
 
-		for (tmp = prev_list->next; tmp != prev_list; tmp = tmp->next){
-			bh = osi_list_entry(tmp, struct gfs2_buffer_head,
-					    b_altlist);
-			if (gfs2_check_meta(bh->b_data, iblk_type)) {
+		for (tmp = prev_list->next; tmp != prev_list; tmp = tmp->next) {
+			iptr.ipt_off = head_size;
+			iptr.ipt_bh = osi_list_entry(tmp, struct gfs2_buffer_head, b_altlist);
+
+			if (gfs2_check_meta(iptr_buf(iptr), iblk_type)) {
 				if (pass->invalid_meta_is_fatal)
 					return meta_error;
 
 				continue;
 			}
-
 			if (pass->readahead)
-				file_ra(ip, bh, head_size, maxptrs, h);
+				file_ra(ip, iptr.ipt_bh, head_size, maxptrs, h);
+
 			/* Now check the metadata itself */
-			for (ptr = (uint64_t *)(bh->b_data + head_size);
-			     (char *)ptr < (bh->b_data + ip->i_sbd->bsize);
-			     ptr++) {
+			for (; iptr.ipt_off < ip->i_sbd->bsize; iptr.ipt_off += sizeof(uint64_t)) {
+				struct gfs2_buffer_head *nbh = NULL;
+
 				if (skip_this_pass || fsck_abort) {
 					free_metalist(ip, mlp);
 					return meta_is_good;
 				}
-				nbh = NULL;
-
-				if (!*ptr)
+				if (!iptr_block(iptr))
 					continue;
 
-				block = be64_to_cpu(*ptr);
-				error = do_check_metalist(ip, block, h, &nbh, pass);
+				error = do_check_metalist(iptr, h, &nbh, pass);
 				if (error == meta_error || error == meta_skip_further)
 					goto error_undo;
 				if (error == meta_skip_one)
 					continue;
 				if (!nbh)
-					nbh = bread(ip->i_sbd, block);
+					nbh = bread(ip->i_sbd, iptr_block(iptr));
 				osi_list_add_prev(&nbh->b_altlist, cur_list);
 			} /* for all data on the indirect block */
 		} /* for blocks at that height */
@@ -1356,16 +1357,16 @@ error_undo: /* undo what we've done so far for this block */
 	if (pass->undo_check_meta == NULL)
 		return error;
 
-	log_info(_("Undoing the work we did before the error on block %llu "
-		   "(0x%llx).\n"), (unsigned long long)bh->b_blocknr,
-		 (unsigned long long)bh->b_blocknr);
-	for (undoptr = (uint64_t *)(bh->b_data + head_size); undoptr < ptr &&
-		     (char *)undoptr < (bh->b_data + ip->i_sbd->bsize);
+	log_info(_("Undoing the work we did before the error on block %"PRIu64" (0x%"PRIx64").\n"),
+	         iptr.ipt_bh->b_blocknr, iptr.ipt_bh->b_blocknr);
+	for (undoptr = (uint64_t *)(iptr_buf(iptr) + head_size);
+	     undoptr < iptr_ptr(iptr) && undoptr < iptr_endptr(iptr);
 	     undoptr++) {
-		if (!*undoptr)
+		uint64_t block = be64_to_cpu(*undoptr);
+
+		if (block == 0)
 			continue;
 
-		block = be64_to_cpu(*undoptr);
 		pass->undo_check_meta(ip, block, h, pass->private);
 	}
 	return error;
diff --git a/gfs2/fsck/metawalk.h b/gfs2/fsck/metawalk.h
index b5a037a3..592479df 100644
--- a/gfs2/fsck/metawalk.h
+++ b/gfs2/fsck/metawalk.h
@@ -42,6 +42,17 @@ enum meta_check_rc {
 	meta_skip_one = 2,
 };
 
+struct iptr {
+	struct gfs2_inode *ipt_ip;
+	struct gfs2_buffer_head *ipt_bh;
+	unsigned ipt_off;
+};
+
+#define iptr_ptr(i) ((uint64_t *)(i.ipt_bh->b_data + i.ipt_off))
+#define iptr_block(i) be64_to_cpu(*iptr_ptr(i))
+#define iptr_endptr(i) ((uint64_t *)(iptr.ipt_bh->b_data + i.ipt_ip->i_sbd->bsize))
+#define iptr_buf(i) (i.ipt_bh->b_data)
+
 /* metawalk_fxns: function pointers to check various parts of the fs
  *
  * The functions should return -1 on fatal errors, 1 if the block
@@ -66,7 +77,7 @@ struct metawalk_fxns {
 	int (*check_leaf) (struct gfs2_inode *ip, uint64_t block,
 			   void *private);
 	/* parameters to the check_metalist sub-functions:
-	   ip: incore inode pointer
+	   iptr: reference to the inode and its indirect pointer that we're analyzing
 	   block: block number of the metadata block to be checked
 	   bh: buffer_head to be returned
 	   h: height
@@ -79,7 +90,7 @@ struct metawalk_fxns {
 	   returns: 0 - everything is good, but there may be duplicates
 	            1 - skip further processing
 	*/
-	int (*check_metalist) (struct gfs2_inode *ip, uint64_t block,
+	int (*check_metalist) (struct iptr iptr,
 			       struct gfs2_buffer_head **bh, int h,
 			       int *is_valid, int *was_duplicate,
 			       void *private);
diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index 442734ba..9f211eb6 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -39,9 +39,8 @@ struct block_count {
 };
 
 static int p1check_leaf(struct gfs2_inode *ip, uint64_t block, void *private);
-static int pass1_check_metalist(struct gfs2_inode *ip, uint64_t block,
-			  struct gfs2_buffer_head **bh, int h, int *is_valid,
-			  int *was_duplicate, void *private);
+static int pass1_check_metalist(struct iptr iptr, struct gfs2_buffer_head **bh, int h,
+                                int *is_valid, int *was_duplicate, void *private);
 static int undo_check_metalist(struct gfs2_inode *ip, uint64_t block,
 			       int h, void *private);
 static int pass1_check_data(struct gfs2_inode *ip, uint64_t metablock,
@@ -69,10 +68,8 @@ static int check_extended_leaf_eattr(struct gfs2_inode *ip, int i,
 				     void *private);
 static int finish_eattr_indir(struct gfs2_inode *ip, int leaf_pointers,
 			      int leaf_pointer_errors, void *private);
-static int invalidate_metadata(struct gfs2_inode *ip, uint64_t block,
-			       struct gfs2_buffer_head **bh, int h,
-			       int *is_valid, int *was_duplicate,
-			       void *private);
+static int invalidate_metadata(struct iptr iptr, struct gfs2_buffer_head **bh, int h,
+                               int *is_valid, int *was_duplicate, void *private);
 static int invalidate_leaf(struct gfs2_inode *ip, uint64_t block,
 			   void *private);
 static int invalidate_data(struct gfs2_inode *ip, uint64_t metablock,
@@ -223,12 +220,12 @@ struct metawalk_fxns invalidate_fxns = {
  * marked "in use" by the bitmap.  You don't want root's indirect blocks
  * deleted, do you? Or worse, reused for lost+found.
  */
-static int resuscitate_metalist(struct gfs2_inode *ip, uint64_t block,
-				struct gfs2_buffer_head **bh, int h,
-				int *is_valid, int *was_duplicate,
-				void *private)
+static int resuscitate_metalist(struct iptr iptr, struct gfs2_buffer_head **bh, int h,
+                                int *is_valid, int *was_duplicate, void *private)
 {
 	struct block_count *bc = (struct block_count *)private;
+	struct gfs2_inode *ip = iptr.ipt_ip;
+	uint64_t block = iptr_block(iptr);
 
 	*is_valid = 1;
 	*was_duplicate = 0;
@@ -344,15 +341,16 @@ static int p1check_leaf(struct gfs2_inode *ip, uint64_t block, void *private)
 	return 0;
 }
 
-static int pass1_check_metalist(struct gfs2_inode *ip, uint64_t block,
-			  struct gfs2_buffer_head **bh, int h, int *is_valid,
-			  int *was_duplicate, void *private)
+static int pass1_check_metalist(struct iptr iptr, struct gfs2_buffer_head **bh, int h,
+                                int *is_valid, int *was_duplicate, void *private)
 {
-	int q;
-	int iblk_type;
-	struct gfs2_buffer_head *nbh;
 	struct block_count *bc = (struct block_count *)private;
+	struct gfs2_inode *ip = iptr.ipt_ip;
+	uint64_t block = iptr_block(iptr);
+	struct gfs2_buffer_head *nbh;
 	const char *blktypedesc;
+	int iblk_type;
+	int q;
 
 	*bh = NULL;
 
@@ -1111,11 +1109,12 @@ static int mark_block_invalid(struct gfs2_inode *ip, uint64_t block,
 	return meta_is_good;
 }
 
-static int invalidate_metadata(struct gfs2_inode *ip, uint64_t block,
-			       struct gfs2_buffer_head **bh, int h,
-			       int *is_valid, int *was_duplicate,
-			       void *private)
+static int invalidate_metadata(struct iptr iptr, struct gfs2_buffer_head **bh, int h,
+                               int *is_valid, int *was_duplicate, void *private)
 {
+	struct gfs2_inode *ip = iptr.ipt_ip;
+	uint64_t block = iptr_block(iptr);
+
 	*is_valid = 1;
 	*was_duplicate = 0;
 	return mark_block_invalid(ip, block, ref_as_meta, _("metadata"),
@@ -1214,11 +1213,12 @@ static int rangecheck_block(struct gfs2_inode *ip, uint64_t block,
 	return meta_is_good;
 }
 
-static int rangecheck_metadata(struct gfs2_inode *ip, uint64_t block,
-			       struct gfs2_buffer_head **bh, int h,
-			       int *is_valid, int *was_duplicate,
-			       void *private)
+static int rangecheck_metadata(struct iptr iptr, struct gfs2_buffer_head **bh, int h,
+                               int *is_valid, int *was_duplicate, void *private)
 {
+	struct gfs2_inode *ip = iptr.ipt_ip;
+	uint64_t block = iptr_block(iptr);
+
 	*is_valid = 1;
 	*was_duplicate = 0;
 	return rangecheck_block(ip, block, bh, btype_meta, private);
@@ -1317,12 +1317,13 @@ static int set_ip_blockmap(struct gfs2_inode *ip)
 	return 0;
 }
 
-static int alloc_metalist(struct gfs2_inode *ip, uint64_t block,
-			  struct gfs2_buffer_head **bh, int h, int *is_valid,
-			  int *was_duplicate, void *private)
+static int alloc_metalist(struct iptr iptr, struct gfs2_buffer_head **bh, int h,
+                          int *is_valid, int *was_duplicate, void *private)
 {
-	int q;
 	const char *desc = (const char *)private;
+	struct gfs2_inode *ip = iptr.ipt_ip;
+	uint64_t block = iptr_block(iptr);
+	int q;
 
 	/* No need to range_check here--if it was added, it's in range. */
 	/* We can't check the bitmap here because this function is called
diff --git a/gfs2/fsck/pass1b.c b/gfs2/fsck/pass1b.c
index 574622bf..91d7fa53 100644
--- a/gfs2/fsck/pass1b.c
+++ b/gfs2/fsck/pass1b.c
@@ -69,9 +69,8 @@ static void log_inode_reference(struct duptree *dt, osi_list_t *tmp, int inval)
 		  (unsigned long long)dt->block, reftypestring);
 }
 
-static int findref_meta(struct gfs2_inode *ip, uint64_t block,
-			struct gfs2_buffer_head **bh, int h,
-			int *is_valid, int *was_duplicate, void *private)
+static int findref_meta(struct iptr iptr, struct gfs2_buffer_head **bh, int h,
+                        int *is_valid, int *was_duplicate, void *private)
 {
 	*is_valid = 1;
 	*was_duplicate = 0;
@@ -393,10 +392,12 @@ static void resolve_dup_references(struct gfs2_sbd *sdp, struct duptree *dt,
 	return;
 }
 
-static int clone_check_meta(struct gfs2_inode *ip, uint64_t block,
-			    struct gfs2_buffer_head **bh, int h,
-			    int *is_valid, int *was_duplicate, void *private)
+static int clone_check_meta(struct iptr iptr, struct gfs2_buffer_head **bh, int h,
+                            int *is_valid, int *was_duplicate, void *private)
 {
+	struct gfs2_inode *ip = iptr.ipt_ip;
+	uint64_t block = iptr_block(iptr);
+
 	*was_duplicate = 0;
 	*is_valid = 1;
 	*bh = bread(ip->i_sbd, block);
@@ -788,11 +789,12 @@ static int check_leaf_refs(struct gfs2_inode *ip, uint64_t block,
 	return add_duplicate_ref(ip, block, ref_as_meta, 1, INODE_VALID);
 }
 
-static int check_metalist_refs(struct gfs2_inode *ip, uint64_t block,
-			       struct gfs2_buffer_head **bh, int h,
-			       int *is_valid, int *was_duplicate,
-			       void *private)
+static int check_metalist_refs(struct iptr iptr, struct gfs2_buffer_head **bh, int h,
+                               int *is_valid, int *was_duplicate, void *private)
 {
+	struct gfs2_inode *ip = iptr.ipt_ip;
+	uint64_t block = iptr_block(iptr);
+
 	*was_duplicate = 0;
 	*is_valid = 1;
 	return add_duplicate_ref(ip, block, ref_as_meta, 1, INODE_VALID);
diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
index 0a54c9be..3c1fae06 100644
--- a/gfs2/fsck/pass2.c
+++ b/gfs2/fsck/pass2.c
@@ -1873,10 +1873,12 @@ struct metawalk_fxns pass2_fxns = {
 	.repair_leaf = pass2_repair_leaf,
 };
 
-static int check_metalist_qc(struct gfs2_inode *ip, uint64_t block,
-			     struct gfs2_buffer_head **bh, int h,
-			     int *is_valid, int *was_duplicate, void *private)
+static int check_metalist_qc(struct iptr iptr, struct gfs2_buffer_head **bh, int h,
+                             int *is_valid, int *was_duplicate, void *private)
 {
+	struct gfs2_inode *ip = iptr.ipt_ip;
+	uint64_t block = iptr_block(iptr);
+
 	*was_duplicate = 0;
 	*is_valid = 1;
 	*bh = bread(ip->i_sbd, block);
-- 
2.21.0

[Cluster-devel] [PATCH 3/3] fsck.gfs2: Clear bad indirect block pointers when bitmap meets expectations

[Andrew Price]

This issue only occurs when an indirect pointer of a 'system' directory
points to an invalid block but the block's bitmap state is as expected.
If the block's state is not as expected, the corruption is fixed by an
earlier check.

In this scenario, pass1_check_metalist() checks the type of a block and
compares it with what it expected the indirect pointer to point to. If
there is a metadata mismatch a bad indirect pointer is reported but the
entire inode is considered invalid, causing it to be removed later, or
rebuilt in the case of protected structures such as the root inode.
This is heavy-handed and the right thing to do is to zero the indirect
block pointer.

Previously we had no access to the pointer block itself to update it in
pass1_check_metalist() but now that an iptr is passed in, it's just a
case of zeroing the pointer at the correct offset and marking the bh as
modified. This means that fsck.gfs2 can now fix bad indirect pointers of
the root directory without throwing away the entire directory tree.

Resolves: rhbz#1487726

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/pass1.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index 9f211eb6..437ceeec 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -401,8 +401,15 @@ static int pass1_check_metalist(struct iptr iptr, struct gfs2_buffer_head **bh,
 			 (unsigned long long)ip->i_di.di_num.no_addr,
 			 (unsigned long long)block,
 			 (unsigned long long)block, blktypedesc);
-		brelse(nbh);
-		return meta_skip_further;
+		if (query(_("Zero the indirect block pointer? (y/n) "))){
+			*iptr_ptr(iptr) = 0;
+			bmodified(iptr.ipt_bh);
+			*is_valid = 1;
+			return meta_skip_one;
+		} else {
+			brelse(nbh);
+			return meta_skip_further;
+		}
 	}
 
 	bc->indir_count++;
-- 
2.21.0