linux-ext4 - Re: [PATCH 2/3] e2fsprogs: move code computing old_desc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20120123165633.GC4439@thunk.org>
Date:	Mon, 23 Jan 2012 11:56:33 -0500
From:	Ted Ts'o <tytso@....edu>
To:	Yongqiang Yang <xiaoqiangnk@...il.com>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: [PATCH 2/3] e2fsprogs: move code computing old_desc_blocks to
 a function

On Wed, Jan 04, 2012 at 07:02:11PM +0800, Yongqiang Yang wrote:
> diff --git a/lib/ext2fs/blknum.c b/lib/ext2fs/blknum.c
> index 33da7d6..f1d5156 100644
> --- a/lib/ext2fs/blknum.c
> +++ b/lib/ext2fs/blknum.c
> @@ -98,6 +98,20 @@ blk64_t ext2fs_blocks_count(struct ext2_super_block *super)
>  }
>  
>  /*
> + * Return the old desc blocks count
> + */
> +blk64_t ext2fs_old_desc_blocks_count(ext2_filsys fs)
> +{
> +	blk64_t old_desc_blocks;
> +
> +	old_desc_blocks = fs->desc_blocks + fs->super->s_reserved_gdt_blocks;
> +	if (fs->super->s_feature_incompat & EXT2_FEATURE_INCOMPAT_META_BG &&
> +	    old_desc_blocks > fs->super->s_first_meta_bg)
> +		old_desc_blocks = fs->super->s_first_meta_bg;
> +	return old_desc_blocks;

I'd prefer if you don't refactor code and change what it does at the
same time.  That makes it harder later to figure out whether a bug is
introduced by the refactorization, or by the change in the
calculation.  It's especially bad when there is no mention of the
change of the calculation in the commit description.

As I mentioned in my comments for your 3/3 patch, s_first_meta_bg
should *always* be less than old_desc_blocks, since it describes the
meta-blockgroup number where we start using the meta-blockgroup
layout.  Since there the size of the meta-blockgroup is defined to be
the number of block groups descriptors that fit in a block, by
definition the meta_bg must be less than or equal to old_desc_blocks.
If it isn't the superblock must be corrupt.

I think it's much better to fix this by adding a check to open and
initialization functions.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html