linux-ext4 - Re: [PATCH] e2fsprogs: New bitmap and inode table allocation for FLEX

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20080324134650.GA13621@mit.edu>
Date:	Mon, 24 Mar 2008 09:46:50 -0400
From:	Theodore Tso <tytso@....EDU>
To:	"Jose R. Santos" <jrs@...ibm.com>
Cc:	linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] e2fsprogs: New bitmap and inode table allocation for
	FLEX_BG v2

I'm starting to audit this patch, and have a bunch of questions and
observations.

On Wed, Feb 13, 2008 at 08:47:50PM -0600, Jose R. Santos wrote:
> +void ext2fs_bgd_set_flex_meta_flag(ext2_filsys fs, blk_t block)
> +{
> +	dgrp_t	group;
> +
> +	group = ext2fs_group_of_blk(fs, block);
> +	if (!(fs->group_desc[group].bg_flags & EXT2_BG_FLEX_METADATA))
> +		fs->group_desc[group].bg_flags |= EXT2_BG_FLEX_METADATA;
> +}

This function is used nowhere else but in lib/ext2fs/alloc_tables.c,
and it's not declared in lib/ext2fs/ext2fs.h.  So I've renamed it to
bgd_set_flex_meta_flag() and declared it static, to make it clear that
it's a private function.

The other question which immediately comes to mind is *why* we need to
set this flag in the first place.  The kernel doesn't use it, and
there doesn't seem to be any reason why needs to be an on-disk flag at
all.  It seems to be used as a way of communicating to mke2fs about
whether or not we can safely set the EXT2_BG_BLOCK_UNINIT flag.  

This turns out to be a kludge whose short comings show other problems.
The real problem is that most of the libext2fs isn't BLOCK_UNINIT
aware.  So for example, if debugfs is used to write a file into the
filesystem, and the block group doesn't have an initialized bitmap,
the Wrong Thing will happen.  More to the point, if you use mke2fs to
a 1k blocksize filesystem, and the journal is bigger than 16 megs, (or
with a 4k blocksize filesystem, if the journal is bigger than 512
megs), you could easily end up allocating the journal into a block
group with BG_BLOCK_UNINIT.  Oops.

This wasn't that much of a big deal since up until now lazy_bg was
only used for debugging really big filesystems, and not much else.  It
was a quick hack for debugging purposes only.  But given that
uninititalized blockgroups are intended for more general use, we have
to make sure all of these corner cases are handled correctly.

Just looking at it quickly, it seems like the right thing to do is
split setup_lazy_bg() into two parts.  The first part sets
EXT2_BG_BLOCK_UNINIT for all block groups, and then we modify the
block allocation functions in lib/ext2fs to clear the BLOCK_UNINIT
flag --- and then later on, we update the bg_free_blocks_count and
s_free_blocks_count for the lazy_bg case.  

This needs more study though, and there is a similar issue, although
not quite so serious about making sure all of libext2fs is
INODE_UNINIT aware.

> +/*
> + * This routine searches for free blocks that can allocate a full
> + * group of bitmaps or inode tables for a flexbg group.  Returns the
> + * block number with a correct offset were the bitmaps and inode
> + * tables can be allocated continously and in order.
> + */
> +blk_t ext2fs_flexbg_offset(ext2_filsys fs, dgrp_t group, blk_t start_blk,
> +			   ext2fs_block_bitmap bmap, int offset, int size)

See above comments about no one using this feature but
lib/ext2fs/alloc_tables.c.  Is there reason why this function isn't
declared static?  (And if it is renamed static, better to remove the
ext2fs_ prefix, to make it clear it isn't a globally visible ext2fs
library function.)

> diff --git a/lib/ext2fs/closefs.c b/lib/ext2fs/closefs.c
> index a523c8e..83d7cc4 100644
> --- a/lib/ext2fs/closefs.c
> +++ b/lib/ext2fs/closefs.c
> @@ -56,6 +56,7 @@ int ext2fs_super_and_bgd_loc(ext2_filsys fs,
>  	unsigned int meta_bg, meta_bg_size;
>  	blk_t	numblocks, old_desc_blocks;
>  	int	has_super;
> +	unsigned int flex_bg_size = 1 << fs->super->s_log_groups_per_flex;
>  
>  	group_block = ext2fs_group_first_block(fs, group);
>  

The function doesn't use this new variable; so it should be just
deleted and removed.

						- 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