commit ab1c367459b5fd71eb80ad40645f25c21b95e70d
Author: Andrew Price <anprice@redhat.com>
Date:   Fri Oct 18 16:07:44 2019 +0100

    fsck.gfs2: Fix segfault in build_and_check_metalist
    
    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>

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index 4d6a2d2c..dbe9f1f6 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -1207,6 +1207,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
@@ -1226,8 +1271,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]);
 
@@ -1291,65 +1336,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