On May 30, 2018, at 6:51 AM, Jan Kara <jack@xxxxxxx> 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@xxxxxxx>
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
Attachment:
signature.asc
Description: Message signed with OpenPGP
