[06/10] e2fsck: Handle s_inodes_count corruption properly

Message ID 20180612095328.5215-7-jack@suse.cz
State Accepted, archived
Headers show
Series
  • e2fsprogs: Handle s_inodes_count overflow better
Related show

Commit Message

Jan Kara June 12, 2018, 9:53 a.m.
When s_inodes_count would overflow given number of groups and inodes per
group, we cannot currently fix the breakage in e2fsck as that requires
trimming number of groups or inodes per group which both means data &
inode migration etc. Just trimming sb->s_inodes_count is not enough as
kernel's inode allocation code is not able to handle filesystems where
not all inodes in the last group are usable. So don't pretend we can fix
s_inodes_count overflow by just trimming the s_inodes_count value.

When s_inodes_count is just wrong but will not overflow, let's fix it.
Also move this check before we use s_inodes_count for checking
s_first_ino.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 e2fsck/problem.c |  5 +++++
 e2fsck/problem.h |  2 ++
 e2fsck/super.c   | 27 ++++++++++++++++-----------
 3 files changed, 23 insertions(+), 11 deletions(-)

Comments

Andreas Dilger June 12, 2018, 7:04 p.m. | #1
On Jun 12, 2018, at 3:53 AM, Jan Kara <jack@suse.cz> wrote:
> 
> When s_inodes_count would overflow given number of groups and inodes per
> group, we cannot currently fix the breakage in e2fsck as that requires
> trimming number of groups or inodes per group which both means data &
> inode migration etc. Just trimming sb->s_inodes_count is not enough as
> kernel's inode allocation code is not able to handle filesystems where
> not all inodes in the last group are usable. So don't pretend we can fix
> s_inodes_count overflow by just trimming the s_inodes_count value.
> 
> When s_inodes_count is just wrong but will not overflow, let's fix it.
> Also move this check before we use s_inodes_count for checking
> s_first_ino.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

I guess that still leaves an open issue of what a user can do if the
superblock (correctly or incorrectly) has groups * inodes_per_group
that overflows 2^32, since it seems e2fsck would abort in that case.

I'd think that one option would be to reduce the group count until
the number of inodes < 2^32, which might lose some files at the end
of the filesystem, but more likely would not lose anything since the
kernel would have written those inodes into the wrong place anyway.

The existing code takes the approach of truncating the last group,
which at least makes the filesystem usable again, though it risks
corrupting inode #1 (EXT2_BAD_INO) on older kernels should the last
inode be allocated.  That probably isn't a huge danger in the end.

Cheers, Andreas

> ---
> e2fsck/problem.c |  5 +++++
> e2fsck/problem.h |  2 ++
> e2fsck/super.c   | 27 ++++++++++++++++-----------
> 3 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/e2fsck/problem.c b/e2fsck/problem.c
> index edc9d51fcf31..8de558edc5dd 100644
> --- a/e2fsck/problem.c
> +++ b/e2fsck/problem.c
> @@ -184,6 +184,11 @@ static struct e2fsck_problem problem_table[] = {
> 	  N_("@i count in @S is %i, @s %j.\n"),
> 	  PROMPT_FIX, 0 },
> 
> +	/* Too many inodes in the filesystem */
> +	{ PR_0_INODE_COUNT_BIG,
> +	  N_("@S would have too many inodes (%N).\n"),
> +	  PROMPT_NONE, PR_AFTER_CODE, PR_0_SB_CORRUPT },
> +
> 	{ PR_0_HURD_CLEAR_FILETYPE,
> 	  N_("The Hurd does not support the filetype feature.\n"),
> 	  PROMPT_CLEAR, 0 },
> diff --git a/e2fsck/problem.h b/e2fsck/problem.h
> index 482d111a0f2c..b6bca668bb1b 100644
> --- a/e2fsck/problem.h
> +++ b/e2fsck/problem.h
> @@ -282,6 +282,8 @@ struct problem_context {
> /* Invalid quota inode number */
> #define PR_0_INVALID_QUOTA_INO			0x00004F
> 
> +/* Inode count in the superblock incorrect */
> +#define PR_0_INODE_COUNT_BIG			0x000050
> 
> /*
>  * Pass 1 errors
> diff --git a/e2fsck/super.c b/e2fsck/super.c
> index 9c0e0e7b35b4..60a19a7e2767 100644
> --- a/e2fsck/super.c
> +++ b/e2fsck/super.c
> @@ -646,6 +646,22 @@ void check_super_block(e2fsck_t ctx)
> 	check_super_value(ctx, "desc_size",
> 			  sb->s_desc_size, MAX_CHECK | LOG2_CHECK, 0,
> 			  EXT2_MAX_DESC_SIZE);
> +
> +	should_be = (__u64)sb->s_inodes_per_group * fs->group_desc_count;
> +	if (should_be > ~0U) {
> +		pctx.num = should_be;
> +		fix_problem(ctx, PR_0_INODE_COUNT_BIG, &pctx);
> +		ctx->flags |= E2F_FLAG_ABORT;
> +		return;
> +	}
> +	if (sb->s_inodes_count != should_be) {
> +		pctx.ino = sb->s_inodes_count;
> +		pctx.ino2 = should_be;
> +		if (fix_problem(ctx, PR_0_INODE_COUNT_WRONG, &pctx)) {
> +			sb->s_inodes_count = should_be;
> +			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,
> @@ -683,17 +699,6 @@ void check_super_block(e2fsck_t ctx)
> 		return;
> 	}
> 
> -	should_be = (blk64_t)sb->s_inodes_per_group * fs->group_desc_count;
> -	if (should_be > UINT_MAX)
> -		should_be = UINT_MAX;
> -	if (sb->s_inodes_count != should_be) {
> -		pctx.ino = sb->s_inodes_count;
> -		pctx.ino2 = should_be;
> -		if (fix_problem(ctx, PR_0_INODE_COUNT_WRONG, &pctx)) {
> -			sb->s_inodes_count = should_be;
> -			ext2fs_mark_super_dirty(fs);
> -		}
> -	}
> 	if (EXT2_INODE_SIZE(sb) > EXT2_GOOD_OLD_INODE_SIZE) {
> 		unsigned min =
> 			sizeof(((struct ext2_inode_large *) 0)->i_extra_isize) +
> --
> 2.13.7
> 


Cheers, Andreas
Jan Kara June 13, 2018, 12:11 p.m. | #2
On Tue 12-06-18 13:04:00, Andreas Dilger wrote:
> On Jun 12, 2018, at 3:53 AM, Jan Kara <jack@suse.cz> wrote:
> > 
> > When s_inodes_count would overflow given number of groups and inodes per
> > group, we cannot currently fix the breakage in e2fsck as that requires
> > trimming number of groups or inodes per group which both means data &
> > inode migration etc. Just trimming sb->s_inodes_count is not enough as
> > kernel's inode allocation code is not able to handle filesystems where
> > not all inodes in the last group are usable. So don't pretend we can fix
> > s_inodes_count overflow by just trimming the s_inodes_count value.
> > 
> > When s_inodes_count is just wrong but will not overflow, let's fix it.
> > Also move this check before we use s_inodes_count for checking
> > s_first_ino.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> I guess that still leaves an open issue of what a user can do if the
> superblock (correctly or incorrectly) has groups * inodes_per_group
> that overflows 2^32, since it seems e2fsck would abort in that case.

Yes, e2fsck would abort as in the case of other non-trivial superblock
corruptions. My thinking was that in that case user can use
debugfs (hence my following debugfs changes) to fix the filesystem.

> I'd think that one option would be to reduce the group count until
> the number of inodes < 2^32, which might lose some files at the end
> of the filesystem, but more likely would not lose anything since the
> kernel would have written those inodes into the wrong place anyway.
> 
> The existing code takes the approach of truncating the last group,
> which at least makes the filesystem usable again, though it risks
> corrupting inode #1 (EXT2_BAD_INO) on older kernels should the last
> inode be allocated.  That probably isn't a huge danger in the end.

Actually currently e2fsck never succeeded in even opening the filesystem
with corrupted s_inodes_count so it could not fix anything. So my change is
at least an improvement that it reports what's wrong and fixes the "easily
fixable" corruptions.

Reducing number of block groups is doable (although libext2fs code is not
quite prepared for that so it may need some fixups). I was just a bit
worried that if the s_inodes_count corruption is not due to resize2fs bug,
reducing the group count may actually be a wrong thing to do and could
cause serious damage to the filesystem. What do people think?

								Honza
Theodore Y. Ts'o June 19, 2018, 2:59 p.m. | #3
On Wed, Jun 13, 2018 at 02:11:05PM +0200, Jan Kara wrote:
> 
> Actually currently e2fsck never succeeded in even opening the filesystem
> with corrupted s_inodes_count so it could not fix anything. So my change is
> at least an improvement that it reports what's wrong and fixes the "easily
> fixable" corruptions.
> 
> Reducing number of block groups is doable (although libext2fs code is not
> quite prepared for that so it may need some fixups). I was just a bit
> worried that if the s_inodes_count corruption is not due to resize2fs bug,
> reducing the group count may actually be a wrong thing to do and could
> cause serious damage to the filesystem. What do people think?

The primary principle should be "first, do no harm".  It's probably
going to be easier to get e2undo with resize2fs working well and by
default than trying to figure out how to fix the s_inodes_count
corruption case.  And it will have a better bang for the buck, since
future bugs may not involve s_inodes_count at all.

So that's probably a better investment of developer time, in my view.

       	    	    	    		      - Ted
Theodore Y. Ts'o June 19, 2018, 3:25 p.m. | #4
On Tue, Jun 12, 2018 at 11:53:24AM +0200, Jan Kara wrote:
> When s_inodes_count would overflow given number of groups and inodes per
> group, we cannot currently fix the breakage in e2fsck as that requires
> trimming number of groups or inodes per group which both means data &
> inode migration etc. Just trimming sb->s_inodes_count is not enough as
> kernel's inode allocation code is not able to handle filesystems where
> not all inodes in the last group are usable. So don't pretend we can fix
> s_inodes_count overflow by just trimming the s_inodes_count value.
> 
> When s_inodes_count is just wrong but will not overflow, let's fix it.
> Also move this check before we use s_inodes_count for checking
> s_first_ino.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Applied with the following fix-up patch.  (The entries in the problem
array must be in problem_code sort order.)

					- Ted

diff --git a/e2fsck/problem.c b/e2fsck/problem.c
index 8de558edc..efe98c920 100644
--- a/e2fsck/problem.c
+++ b/e2fsck/problem.c
@@ -184,11 +184,6 @@ static struct e2fsck_problem problem_table[] = {
 	  N_("@i count in @S is %i, @s %j.\n"),
 	  PROMPT_FIX, 0 },
 
-	/* Too many inodes in the filesystem */
-	{ PR_0_INODE_COUNT_BIG,
-	  N_("@S would have too many inodes (%N).\n"),
-	  PROMPT_NONE, PR_AFTER_CODE, PR_0_SB_CORRUPT },
-
 	{ PR_0_HURD_CLEAR_FILETYPE,
 	  N_("The Hurd does not support the filetype feature.\n"),
 	  PROMPT_CLEAR, 0 },
@@ -498,6 +493,11 @@ static struct e2fsck_problem problem_table[] = {
 	  N_("Invalid %U @q @i %i.  "),
 	  PROMPT_FIX, 0 },
 
+	/* Too many inodes in the filesystem */
+	{ PR_0_INODE_COUNT_BIG,
+	  N_("@S would have too many inodes (%N).\n"),
+	  PROMPT_NONE, PR_AFTER_CODE, PR_0_SB_CORRUPT },
+
 	/* Pass 1 errors */
 
 	/* Pass 1: Checking inodes, blocks, and sizes */
Jan Kara June 20, 2018, 7:59 a.m. | #5
On Tue 19-06-18 11:25:35, Theodore Y. Ts'o wrote:
> On Tue, Jun 12, 2018 at 11:53:24AM +0200, Jan Kara wrote:
> > When s_inodes_count would overflow given number of groups and inodes per
> > group, we cannot currently fix the breakage in e2fsck as that requires
> > trimming number of groups or inodes per group which both means data &
> > inode migration etc. Just trimming sb->s_inodes_count is not enough as
> > kernel's inode allocation code is not able to handle filesystems where
> > not all inodes in the last group are usable. So don't pretend we can fix
> > s_inodes_count overflow by just trimming the s_inodes_count value.
> > 
> > When s_inodes_count is just wrong but will not overflow, let's fix it.
> > Also move this check before we use s_inodes_count for checking
> > s_first_ino.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Applied with the following fix-up patch.  (The entries in the problem
> array must be in problem_code sort order.)

Thanks for fixing that up!

								Honza

> diff --git a/e2fsck/problem.c b/e2fsck/problem.c
> index 8de558edc..efe98c920 100644
> --- a/e2fsck/problem.c
> +++ b/e2fsck/problem.c
> @@ -184,11 +184,6 @@ static struct e2fsck_problem problem_table[] = {
>  	  N_("@i count in @S is %i, @s %j.\n"),
>  	  PROMPT_FIX, 0 },
>  
> -	/* Too many inodes in the filesystem */
> -	{ PR_0_INODE_COUNT_BIG,
> -	  N_("@S would have too many inodes (%N).\n"),
> -	  PROMPT_NONE, PR_AFTER_CODE, PR_0_SB_CORRUPT },
> -
>  	{ PR_0_HURD_CLEAR_FILETYPE,
>  	  N_("The Hurd does not support the filetype feature.\n"),
>  	  PROMPT_CLEAR, 0 },
> @@ -498,6 +493,11 @@ static struct e2fsck_problem problem_table[] = {
>  	  N_("Invalid %U @q @i %i.  "),
>  	  PROMPT_FIX, 0 },
>  
> +	/* Too many inodes in the filesystem */
> +	{ PR_0_INODE_COUNT_BIG,
> +	  N_("@S would have too many inodes (%N).\n"),
> +	  PROMPT_NONE, PR_AFTER_CODE, PR_0_SB_CORRUPT },
> +
>  	/* Pass 1 errors */
>  
>  	/* Pass 1: Checking inodes, blocks, and sizes */

Patch

diff --git a/e2fsck/problem.c b/e2fsck/problem.c
index edc9d51fcf31..8de558edc5dd 100644
--- a/e2fsck/problem.c
+++ b/e2fsck/problem.c
@@ -184,6 +184,11 @@  static struct e2fsck_problem problem_table[] = {
 	  N_("@i count in @S is %i, @s %j.\n"),
 	  PROMPT_FIX, 0 },
 
+	/* Too many inodes in the filesystem */
+	{ PR_0_INODE_COUNT_BIG,
+	  N_("@S would have too many inodes (%N).\n"),
+	  PROMPT_NONE, PR_AFTER_CODE, PR_0_SB_CORRUPT },
+
 	{ PR_0_HURD_CLEAR_FILETYPE,
 	  N_("The Hurd does not support the filetype feature.\n"),
 	  PROMPT_CLEAR, 0 },
diff --git a/e2fsck/problem.h b/e2fsck/problem.h
index 482d111a0f2c..b6bca668bb1b 100644
--- a/e2fsck/problem.h
+++ b/e2fsck/problem.h
@@ -282,6 +282,8 @@  struct problem_context {
 /* Invalid quota inode number */
 #define PR_0_INVALID_QUOTA_INO			0x00004F
 
+/* Inode count in the superblock incorrect */
+#define PR_0_INODE_COUNT_BIG			0x000050
 
 /*
  * Pass 1 errors
diff --git a/e2fsck/super.c b/e2fsck/super.c
index 9c0e0e7b35b4..60a19a7e2767 100644
--- a/e2fsck/super.c
+++ b/e2fsck/super.c
@@ -646,6 +646,22 @@  void check_super_block(e2fsck_t ctx)
 	check_super_value(ctx, "desc_size",
 			  sb->s_desc_size, MAX_CHECK | LOG2_CHECK, 0,
 			  EXT2_MAX_DESC_SIZE);
+
+	should_be = (__u64)sb->s_inodes_per_group * fs->group_desc_count;
+	if (should_be > ~0U) {
+		pctx.num = should_be;
+		fix_problem(ctx, PR_0_INODE_COUNT_BIG, &pctx);
+		ctx->flags |= E2F_FLAG_ABORT;
+		return;
+	}
+	if (sb->s_inodes_count != should_be) {
+		pctx.ino = sb->s_inodes_count;
+		pctx.ino2 = should_be;
+		if (fix_problem(ctx, PR_0_INODE_COUNT_WRONG, &pctx)) {
+			sb->s_inodes_count = should_be;
+			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,
@@ -683,17 +699,6 @@  void check_super_block(e2fsck_t ctx)
 		return;
 	}
 
-	should_be = (blk64_t)sb->s_inodes_per_group * fs->group_desc_count;
-	if (should_be > UINT_MAX)
-		should_be = UINT_MAX;
-	if (sb->s_inodes_count != should_be) {
-		pctx.ino = sb->s_inodes_count;
-		pctx.ino2 = should_be;
-		if (fix_problem(ctx, PR_0_INODE_COUNT_WRONG, &pctx)) {
-			sb->s_inodes_count = should_be;
-			ext2fs_mark_super_dirty(fs);
-		}
-	}
 	if (EXT2_INODE_SIZE(sb) > EXT2_GOOD_OLD_INODE_SIZE) {
 		unsigned min =
 			sizeof(((struct ext2_inode_large *) 0)->i_extra_isize) +