[06/10] e2fsck: Handle s_inodes_count corruption properly

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. So don't pretend we can fix it 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 to 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(-)

On May 30, 2018, at 6:51 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. So don't pretend we can fix it 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 to checking
> s_first_ino.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Some comments inline.

> ---
> 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..00872b5ce5dc 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 = (blk64_t)sb->s_inodes_per_group * fs->group_desc_count;

I don't think "blk64_t" is quite the right type here either, that is
intended for 64-bit block numbers, while you are computing a 64-bit
inode number...

> +	if (should_be > UINT_MAX) {
> +		pctx.num = should_be;
> +		fix_problem(ctx, PR_0_INODE_COUNT_BIG, &pctx);
> +		ctx->flags |= E2F_FLAG_ABORT;
> +		return;
> +	}

I think this should also handle the case hit by the original bug reporter,
"should_be == 2^32".  That would likely be the common case hit here, since
the number of inodes per group is typically a power of two value and
resizing the filesystem too much would hit exactly 2^32.  Something like:

	if (should_be == (unsigned long long)UINT_MAX + 1) {
		should_be = UINT_MAX;
	} else if (should_be > UINT_MAX) {
		:
		:

This matches the behaviour in mke2fs that converts 2^32 inodes to 2^32-1.
Otherwise, it is possible for a valid filesystem formatted by mke2fs with
2^32-1 inodes cannot be checked by e2fsck.

Also, in patch 01/10 it showed "~0U" for the maximum inode count is used
in ext2fs/initialize.c, but this is using "UINT_MAX".  While they are likely
the same, it would be good to be consistent.

Cheers, Andreas

> +	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.6
> 


Cheers, Andreas

On Sun 03-06-18 22:50:39, Andreas Dilger wrote:
> On May 30, 2018, at 6:51 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. So don't pretend we can fix it 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 to checking
> > s_first_ino.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Some comments inline.
> 
> > ---
> > 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..00872b5ce5dc 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 = (blk64_t)sb->s_inodes_per_group * fs->group_desc_count;
> 
> I don't think "blk64_t" is quite the right type here either, that is
> intended for 64-bit block numbers, while you are computing a 64-bit
> inode number...

Right. I've copied this from the old e2fsck code but I agree it's better to
fix this up when moving the code.

> > +	if (should_be > UINT_MAX) {
> > +		pctx.num = should_be;
> > +		fix_problem(ctx, PR_0_INODE_COUNT_BIG, &pctx);
> > +		ctx->flags |= E2F_FLAG_ABORT;
> > +		return;
> > +	}
> 
> I think this should also handle the case hit by the original bug reporter,
> "should_be == 2^32".  That would likely be the common case hit here, since
> the number of inodes per group is typically a power of two value and
> resizing the filesystem too much would hit exactly 2^32.  Something like:
> 
> 	if (should_be == (unsigned long long)UINT_MAX + 1) {
> 		should_be = UINT_MAX;
> 	} else if (should_be > UINT_MAX) {
> 		:
> 		:
> 
> This matches the behaviour in mke2fs that converts 2^32 inodes to 2^32-1.
> Otherwise, it is possible for a valid filesystem formatted by mke2fs with
> 2^32-1 inodes cannot be checked by e2fsck.

Originally I wanted to do something like this as well. The problem with
setting number of inodes to 2^32-1 when it should really be 2^32 is that
the kernel is not really able to handle such filesystems safely. In
particular the code in fs/ext4/ialloc.c will happily allocate the inode
with number 2^32 which immediately overflows to 0 and bad things happen
after that. We could fix up the code there but older kernels won't be able
to handle it anyway. We could mark the last inode as allocated but that
would require more fixups in other places so in the end I've decided the
problems simply are not worth the additional possibility of having
filesystem larger by 1 block group.

I'll check mke2fs code and fix it up if the code there still trims inode
count of 2^32 to 2^32-1.

> Also, in patch 01/10 it showed "~0U" for the maximum inode count is used
> in ext2fs/initialize.c, but this is using "UINT_MAX".  While they are likely
> the same, it would be good to be consistent.

Agreed, I'll change that. Thanks for review!

								Honza

On Wed 06-06-18 11:17:39, Jan Kara wrote:
> On Sun 03-06-18 22:50:39, Andreas Dilger wrote:
> > On May 30, 2018, at 6:51 AM, Jan Kara <jack@suse.cz> wrote:
> > > +	if (should_be > UINT_MAX) {
> > > +		pctx.num = should_be;
> > > +		fix_problem(ctx, PR_0_INODE_COUNT_BIG, &pctx);
> > > +		ctx->flags |= E2F_FLAG_ABORT;
> > > +		return;
> > > +	}
> > 
> > I think this should also handle the case hit by the original bug reporter,
> > "should_be == 2^32".  That would likely be the common case hit here, since
> > the number of inodes per group is typically a power of two value and
> > resizing the filesystem too much would hit exactly 2^32.  Something like:
> > 
> > 	if (should_be == (unsigned long long)UINT_MAX + 1) {
> > 		should_be = UINT_MAX;
> > 	} else if (should_be > UINT_MAX) {
> > 		:
> > 		:
> > 
> > This matches the behaviour in mke2fs that converts 2^32 inodes to 2^32-1.
> > Otherwise, it is possible for a valid filesystem formatted by mke2fs with
> > 2^32-1 inodes cannot be checked by e2fsck.
> 
> Originally I wanted to do something like this as well. The problem with
> setting number of inodes to 2^32-1 when it should really be 2^32 is that
> the kernel is not really able to handle such filesystems safely. In
> particular the code in fs/ext4/ialloc.c will happily allocate the inode
> with number 2^32 which immediately overflows to 0 and bad things happen
> after that. We could fix up the code there but older kernels won't be able
> to handle it anyway. We could mark the last inode as allocated but that
> would require more fixups in other places so in the end I've decided the
> problems simply are not worth the additional possibility of having
> filesystem larger by 1 block group.
> 
> I'll check mke2fs code and fix it up if the code there still trims inode
> count of 2^32 to 2^32-1.

So I've checked what mke2fs (lib/ext2fs/initialize.c) does and although we
do trim the inode count to 2^32-1 there, this is just an initial estimate
which is later rounded to have equal number of inodes per group and to have
full inode blocks and in the end we check that the number of inodes fits in
32 bits (and reduce number of inodes per group if not) so overall mke2fs
cannot generate filesystem where number of usable inodes in the last group
would be different from the number of usable inodes in other groups.

I've fixed all the other issues you've mentioned.

								Honza