diff mbox series

[10/10] e2fsck: Report only one sb corruption

Message ID 20180530125118.25274-11-jack@suse.cz
State Superseded, archived
Headers show
Series e2fsprogs: Handle s_inodes_count overflow better | expand

Commit Message

Jan Kara May 30, 2018, 12:51 p.m. UTC 
check_super_value() does not terminate in case of error anymore since
c8b20b40ebf0 "misc: add plausibility checks to
debugfs/tune2fs/dumpe2fs/e2fsck" which removed the PR_FATAL flag from
PR_0_SB_CORRUPT problem. This results in potentially many errors for
superblock being printed including the long message about how to deal
with corrupted superblock. Restore the original behavior of reporting
only one error and also remove the comments 'never get here' as they are
not true anymore.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 e2fsck/super.c | 98 ++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 58 insertions(+), 40 deletions(-)

Comments

Andreas Dilger June 4, 2018, 4:57 a.m. UTC | #1 
On May 30, 2018, at 6:51 AM, Jan Kara <jack@suse.cz> wrote:
> 
> check_super_value() does not terminate in case of error anymore since
> c8b20b40ebf0 "misc: add plausibility checks to
> debugfs/tune2fs/dumpe2fs/e2fsck" which removed the PR_FATAL flag from
> PR_0_SB_CORRUPT problem. This results in potentially many errors for
> superblock being printed including the long message about how to deal
> with corrupted superblock. Restore the original behavior of reporting
> only one error and also remove the comments 'never get here' as they are
> not true anymore.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> e2fsck/super.c | 98 ++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 58 insertions(+), 40 deletions(-)
> 
> diff --git a/e2fsck/super.c b/e2fsck/super.c
> index 00872b5ce5dc..26e531d5a13c 100644
> --- a/e2fsck/super.c
> +++ b/e2fsck/super.c
> @@ -24,7 +24,7 @@
> #define MAX_CHECK 2
> #define LOG2_CHECK 4
> 
> -static void check_super_value(e2fsck_t ctx, const char *descr,
> +static int check_super_value(e2fsck_t ctx, const char *descr,
> 			      unsigned long value, int flags,
> 			      unsigned long min_val, unsigned long max_val)
> {
> @@ -37,11 +37,13 @@ static void check_super_value(e2fsck_t ctx, const char *descr,
> 		pctx.num = value;
> 		pctx.str = descr;
> 		fix_problem(ctx, PR_0_MISC_CORRUPT_SUPER, &pctx);
> -		ctx->flags |= E2F_FLAG_ABORT; /* never get here! */
> +		ctx->flags |= E2F_FLAG_ABORT;
> +		return 0;
> 	}
> +	return 1;
> }
> 
> -static void check_super_value64(e2fsck_t ctx, const char *descr,
> +static int check_super_value64(e2fsck_t ctx, const char *descr,
> 				__u64 value, int flags,
> 				__u64 min_val, __u64 max_val)
> {
> @@ -54,8 +56,10 @@ static void check_super_value64(e2fsck_t ctx, const char *descr,
> 		pctx.num = value;
> 		pctx.str = descr;
> 		fix_problem(ctx, PR_0_MISC_CORRUPT_SUPER, &pctx);
> -		ctx->flags |= E2F_FLAG_ABORT; /* never get here! */
> +		ctx->flags |= E2F_FLAG_ABORT;
> +		return 0;
> 	}
> +	return 1;
> }
> 
> /*
> @@ -618,34 +622,46 @@ void check_super_block(e2fsck_t ctx)
> 	/*
> 	 * Verify the super block constants...
> 	 */
> -	check_super_value(ctx, "inodes_count", sb->s_inodes_count,
> -			  MIN_CHECK, 1, 0);
> -	check_super_value64(ctx, "blocks_count", ext2fs_blocks_count(sb),
> -			    MIN_CHECK | MAX_CHECK, 1, blks_max);
> -	check_super_value(ctx, "first_data_block", sb->s_first_data_block,
> -			  MAX_CHECK, 0, ext2fs_blocks_count(sb));
> -	check_super_value(ctx, "log_block_size", sb->s_log_block_size,
> -			  MIN_CHECK | MAX_CHECK, 0,
> -			  EXT2_MAX_BLOCK_LOG_SIZE - EXT2_MIN_BLOCK_LOG_SIZE);
> -	check_super_value(ctx, "log_cluster_size",
> -			  sb->s_log_cluster_size,
> -			  MIN_CHECK | MAX_CHECK, sb->s_log_block_size,
> -			  (EXT2_MAX_CLUSTER_LOG_SIZE -
> -			   EXT2_MIN_CLUSTER_LOG_SIZE));
> -	check_super_value(ctx, "clusters_per_group", sb->s_clusters_per_group,
> -			  MIN_CHECK | MAX_CHECK, 8, cpg_max);
> -	check_super_value(ctx, "blocks_per_group", sb->s_blocks_per_group,
> -			  MIN_CHECK | MAX_CHECK, 8, bpg_max);
> -	check_super_value(ctx, "inodes_per_group", sb->s_inodes_per_group,
> -			  MIN_CHECK | MAX_CHECK, inodes_per_block, ipg_max);
> -	check_super_value(ctx, "r_blocks_count", ext2fs_r_blocks_count(sb),
> -			  MAX_CHECK, 0, ext2fs_blocks_count(sb) / 2);
> -	check_super_value(ctx, "reserved_gdt_blocks",
> -			  sb->s_reserved_gdt_blocks, MAX_CHECK, 0,
> -			  fs->blocksize / sizeof(__u32));
> -	check_super_value(ctx, "desc_size",
> -			  sb->s_desc_size, MAX_CHECK | LOG2_CHECK, 0,
> -			  EXT2_MAX_DESC_SIZE);
> +	if (!check_super_value(ctx, "inodes_count", sb->s_inodes_count,
> +			       MIN_CHECK, 1, 0))
> +		return;
> +	if (!check_super_value64(ctx, "blocks_count", ext2fs_blocks_count(sb),
> +				 MIN_CHECK | MAX_CHECK, 1, blks_max))
> +		return;
> +	if (!check_super_value(ctx, "first_data_block", sb->s_first_data_block,
> +			       MAX_CHECK, 0, ext2fs_blocks_count(sb)))
> +		return;
> +	if (!check_super_value(ctx, "log_block_size", sb->s_log_block_size,
> +			       MIN_CHECK | MAX_CHECK, 0,
> +			       EXT2_MAX_BLOCK_LOG_SIZE - EXT2_MIN_BLOCK_LOG_SIZE))
> +		return;
> +	if (!check_super_value(ctx, "log_cluster_size",
> +			       sb->s_log_cluster_size,
> +			       MIN_CHECK | MAX_CHECK, sb->s_log_block_size,
> +			       (EXT2_MAX_CLUSTER_LOG_SIZE -
> +			        EXT2_MIN_CLUSTER_LOG_SIZE)))
> +		return;
> +	if (!check_super_value(ctx, "clusters_per_group",
> +			       sb->s_clusters_per_group,
> +			       MIN_CHECK | MAX_CHECK, 8, cpg_max))
> +		return;
> +	if (!check_super_value(ctx, "blocks_per_group", sb->s_blocks_per_group,
> +			       MIN_CHECK | MAX_CHECK, 8, bpg_max))
> +		return;
> +	if (!check_super_value(ctx, "inodes_per_group", sb->s_inodes_per_group,
> +			       MIN_CHECK | MAX_CHECK, inodes_per_block, ipg_max))
> +		return;
> +	if (!check_super_value(ctx, "r_blocks_count", ext2fs_r_blocks_count(sb),
> +			       MAX_CHECK, 0, ext2fs_blocks_count(sb) / 2))
> +		return;
> +	if (!check_super_value(ctx, "reserved_gdt_blocks",
> +			       sb->s_reserved_gdt_blocks, MAX_CHECK, 0,
> +			       fs->blocksize / sizeof(__u32)))
> +		return;
> +	if (!check_super_value(ctx, "desc_size",
> +			       sb->s_desc_size, MAX_CHECK | LOG2_CHECK, 0,
> +			       EXT2_MAX_DESC_SIZE))
> +		return;
> 
> 	should_be = (blk64_t)sb->s_inodes_per_group * fs->group_desc_count;
> 	if (should_be > UINT_MAX) {
> @@ -662,20 +678,22 @@ void check_super_block(e2fsck_t ctx)
> 			ext2fs_mark_super_dirty(fs);
> 		}
> 	}
> -	if (sb->s_rev_level > EXT2_GOOD_OLD_REV)
> -		check_super_value(ctx, "first_ino", sb->s_first_ino,
> -				  MIN_CHECK | MAX_CHECK,
> -				  EXT2_GOOD_OLD_FIRST_INO, sb->s_inodes_count);
> +	if (sb->s_rev_level > EXT2_GOOD_OLD_REV &&
> +	    !check_super_value(ctx, "first_ino", sb->s_first_ino,
> +			       MIN_CHECK | MAX_CHECK,
> +			       EXT2_GOOD_OLD_FIRST_INO, sb->s_inodes_count))
> +		return;
> 	inode_size = EXT2_INODE_SIZE(sb);
> -	check_super_value(ctx, "inode_size",
> -			  inode_size, MIN_CHECK | MAX_CHECK | LOG2_CHECK,
> -			  EXT2_GOOD_OLD_INODE_SIZE, fs->blocksize);
> +	if (!check_super_value(ctx, "inode_size",
> +			       inode_size, MIN_CHECK | MAX_CHECK | LOG2_CHECK,
> +			       EXT2_GOOD_OLD_INODE_SIZE, fs->blocksize))
> +		return;
> 	if (sb->s_blocks_per_group != (sb->s_clusters_per_group *
> 				       EXT2FS_CLUSTER_RATIO(fs))) {
> 		pctx.num = sb->s_clusters_per_group * EXT2FS_CLUSTER_RATIO(fs);
> 		pctx.str = "block_size";
> 		fix_problem(ctx, PR_0_MISC_CORRUPT_SUPER, &pctx);
> -		ctx->flags |= E2F_FLAG_ABORT; /* never get here! */
> +		ctx->flags |= E2F_FLAG_ABORT;
> 		return;
> 	}
> 
> --
> 2.13.6
> 


Cheers, Andreas
diff mbox series

Patch

diff --git a/e2fsck/super.c b/e2fsck/super.c
index 00872b5ce5dc..26e531d5a13c 100644
--- a/e2fsck/super.c
+++ b/e2fsck/super.c
@@ -24,7 +24,7 @@ 
 #define MAX_CHECK 2
 #define LOG2_CHECK 4
 
-static void check_super_value(e2fsck_t ctx, const char *descr,
+static int check_super_value(e2fsck_t ctx, const char *descr,
 			      unsigned long value, int flags,
 			      unsigned long min_val, unsigned long max_val)
 {
@@ -37,11 +37,13 @@  static void check_super_value(e2fsck_t ctx, const char *descr,
 		pctx.num = value;
 		pctx.str = descr;
 		fix_problem(ctx, PR_0_MISC_CORRUPT_SUPER, &pctx);
-		ctx->flags |= E2F_FLAG_ABORT; /* never get here! */
+		ctx->flags |= E2F_FLAG_ABORT;
+		return 0;
 	}
+	return 1;
 }
 
-static void check_super_value64(e2fsck_t ctx, const char *descr,
+static int check_super_value64(e2fsck_t ctx, const char *descr,
 				__u64 value, int flags,
 				__u64 min_val, __u64 max_val)
 {
@@ -54,8 +56,10 @@  static void check_super_value64(e2fsck_t ctx, const char *descr,
 		pctx.num = value;
 		pctx.str = descr;
 		fix_problem(ctx, PR_0_MISC_CORRUPT_SUPER, &pctx);
-		ctx->flags |= E2F_FLAG_ABORT; /* never get here! */
+		ctx->flags |= E2F_FLAG_ABORT;
+		return 0;
 	}
+	return 1;
 }
 
 /*
@@ -618,34 +622,46 @@  void check_super_block(e2fsck_t ctx)
 	/*
 	 * Verify the super block constants...
 	 */
-	check_super_value(ctx, "inodes_count", sb->s_inodes_count,
-			  MIN_CHECK, 1, 0);
-	check_super_value64(ctx, "blocks_count", ext2fs_blocks_count(sb),
-			    MIN_CHECK | MAX_CHECK, 1, blks_max);
-	check_super_value(ctx, "first_data_block", sb->s_first_data_block,
-			  MAX_CHECK, 0, ext2fs_blocks_count(sb));
-	check_super_value(ctx, "log_block_size", sb->s_log_block_size,
-			  MIN_CHECK | MAX_CHECK, 0,
-			  EXT2_MAX_BLOCK_LOG_SIZE - EXT2_MIN_BLOCK_LOG_SIZE);
-	check_super_value(ctx, "log_cluster_size",
-			  sb->s_log_cluster_size,
-			  MIN_CHECK | MAX_CHECK, sb->s_log_block_size,
-			  (EXT2_MAX_CLUSTER_LOG_SIZE -
-			   EXT2_MIN_CLUSTER_LOG_SIZE));
-	check_super_value(ctx, "clusters_per_group", sb->s_clusters_per_group,
-			  MIN_CHECK | MAX_CHECK, 8, cpg_max);
-	check_super_value(ctx, "blocks_per_group", sb->s_blocks_per_group,
-			  MIN_CHECK | MAX_CHECK, 8, bpg_max);
-	check_super_value(ctx, "inodes_per_group", sb->s_inodes_per_group,
-			  MIN_CHECK | MAX_CHECK, inodes_per_block, ipg_max);
-	check_super_value(ctx, "r_blocks_count", ext2fs_r_blocks_count(sb),
-			  MAX_CHECK, 0, ext2fs_blocks_count(sb) / 2);
-	check_super_value(ctx, "reserved_gdt_blocks",
-			  sb->s_reserved_gdt_blocks, MAX_CHECK, 0,
-			  fs->blocksize / sizeof(__u32));
-	check_super_value(ctx, "desc_size",
-			  sb->s_desc_size, MAX_CHECK | LOG2_CHECK, 0,
-			  EXT2_MAX_DESC_SIZE);
+	if (!check_super_value(ctx, "inodes_count", sb->s_inodes_count,
+			       MIN_CHECK, 1, 0))
+		return;
+	if (!check_super_value64(ctx, "blocks_count", ext2fs_blocks_count(sb),
+				 MIN_CHECK | MAX_CHECK, 1, blks_max))
+		return;
+	if (!check_super_value(ctx, "first_data_block", sb->s_first_data_block,
+			       MAX_CHECK, 0, ext2fs_blocks_count(sb)))
+		return;
+	if (!check_super_value(ctx, "log_block_size", sb->s_log_block_size,
+			       MIN_CHECK | MAX_CHECK, 0,
+			       EXT2_MAX_BLOCK_LOG_SIZE - EXT2_MIN_BLOCK_LOG_SIZE))
+		return;
+	if (!check_super_value(ctx, "log_cluster_size",
+			       sb->s_log_cluster_size,
+			       MIN_CHECK | MAX_CHECK, sb->s_log_block_size,
+			       (EXT2_MAX_CLUSTER_LOG_SIZE -
+			        EXT2_MIN_CLUSTER_LOG_SIZE)))
+		return;
+	if (!check_super_value(ctx, "clusters_per_group",
+			       sb->s_clusters_per_group,
+			       MIN_CHECK | MAX_CHECK, 8, cpg_max))
+		return;
+	if (!check_super_value(ctx, "blocks_per_group", sb->s_blocks_per_group,
+			       MIN_CHECK | MAX_CHECK, 8, bpg_max))
+		return;
+	if (!check_super_value(ctx, "inodes_per_group", sb->s_inodes_per_group,
+			       MIN_CHECK | MAX_CHECK, inodes_per_block, ipg_max))
+		return;
+	if (!check_super_value(ctx, "r_blocks_count", ext2fs_r_blocks_count(sb),
+			       MAX_CHECK, 0, ext2fs_blocks_count(sb) / 2))
+		return;
+	if (!check_super_value(ctx, "reserved_gdt_blocks",
+			       sb->s_reserved_gdt_blocks, MAX_CHECK, 0,
+			       fs->blocksize / sizeof(__u32)))
+		return;
+	if (!check_super_value(ctx, "desc_size",
+			       sb->s_desc_size, MAX_CHECK | LOG2_CHECK, 0,
+			       EXT2_MAX_DESC_SIZE))
+		return;
 
 	should_be = (blk64_t)sb->s_inodes_per_group * fs->group_desc_count;
 	if (should_be > UINT_MAX) {
@@ -662,20 +678,22 @@  void check_super_block(e2fsck_t ctx)
 			ext2fs_mark_super_dirty(fs);
 		}
 	}
-	if (sb->s_rev_level > EXT2_GOOD_OLD_REV)
-		check_super_value(ctx, "first_ino", sb->s_first_ino,
-				  MIN_CHECK | MAX_CHECK,
-				  EXT2_GOOD_OLD_FIRST_INO, sb->s_inodes_count);
+	if (sb->s_rev_level > EXT2_GOOD_OLD_REV &&
+	    !check_super_value(ctx, "first_ino", sb->s_first_ino,
+			       MIN_CHECK | MAX_CHECK,
+			       EXT2_GOOD_OLD_FIRST_INO, sb->s_inodes_count))
+		return;
 	inode_size = EXT2_INODE_SIZE(sb);
-	check_super_value(ctx, "inode_size",
-			  inode_size, MIN_CHECK | MAX_CHECK | LOG2_CHECK,
-			  EXT2_GOOD_OLD_INODE_SIZE, fs->blocksize);
+	if (!check_super_value(ctx, "inode_size",
+			       inode_size, MIN_CHECK | MAX_CHECK | LOG2_CHECK,
+			       EXT2_GOOD_OLD_INODE_SIZE, fs->blocksize))
+		return;
 	if (sb->s_blocks_per_group != (sb->s_clusters_per_group *
 				       EXT2FS_CLUSTER_RATIO(fs))) {
 		pctx.num = sb->s_clusters_per_group * EXT2FS_CLUSTER_RATIO(fs);
 		pctx.str = "block_size";
 		fix_problem(ctx, PR_0_MISC_CORRUPT_SUPER, &pctx);
-		ctx->flags |= E2F_FLAG_ABORT; /* never get here! */
+		ctx->flags |= E2F_FLAG_ABORT;
 		return;
 	}