linux-ext4 - Re: [RFC PATCH 04/17] Implement 64-bit "bitarray" bmap ops

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] [thread-next>] [day] [month] [year] [list]

Message-id: <20081112204756.GO16005@webber.adilger.int>
Date:	Wed, 12 Nov 2008 13:47:56 -0700
From:	Andreas Dilger <adilger@....com>
To:	Valerie Aurora Henson <vaurora@...hat.com>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: [RFC PATCH 04/17] Implement 64-bit "bitarray" bmap ops

On Nov 11, 2008  19:42 -0800, Valerie Aurora Henson wrote:
> +/*
> + * Private data for bit array implementation of bitmap ops.
> + * Currently, this is just a pointer to our big flat hunk of memory,
> + * exactly equivalent to the old-skool char * bitmap member.
> + */
> +
> +struct ext2fs_ba_private_struct {
> +	char *bitarray;
> +};

Since we're going to the expense of allocating a 1-pointer data structure,
we may as well make it useful by adding a magic value in there that can
be verified later and catch code bugs or corruption.

> +static errcode_t ba_new_bmap(ext2_filsys fs, ext2fs_generic_bitmap64 bitmap)
>  {
> +	bp = (ext2fs_ba_private) bitmap->private;

Then use a simple accessor function ba_private_to_bitarray() to verify the
pointer magic and return the bitarray pointer directly.  That would remove
the direct use of "ext2fs_ba_private" in the majority of the code.

>  static errcode_t ba_copy_bmap(ext2fs_generic_bitmap64 src,
> +			      ext2fs_generic_bitmap64 dest)
>  {
> +	size = (size_t) (((src->real_end - src->start) / 8) + 1);
> +	memcpy (dest_bp->bitarray, src_bp->bitarray, size);

Would it also be worthwhile to store the size of the bitarray in the
ba_private_struct for verification?

> -	errcode_t (*new_bmap)(ext2_filsys fs, ext2fs_generic_bitmap64 *bmap);
> +	errcode_t (*new_bmap)(ext2_filsys fs, ext2fs_generic_bitmap64 bmap);

As a general rule, I dislike types that are pointers, as it isn't clear
from looking at this code if you are passing "bmap" by reference or a
copy.

> @@ -162,37 +163,53 @@ errcode_t ext2fs_copy_generic_bmap(ext2fs_generic_bitmap64 src,
>  					  ext2fs_generic_bitmap64 *dest)
>  {
>  	if (!EXT2FS_IS_64_BITMAP(src))
> -		return;
> +		return EINVAL;

Is this worth a better error than "EINVAL"?  In general, anything that
returns "errcode_t" can have a very specific error return value as
defined in lib/ext2fs/ext2_err.et.in.  This is true of all of these
functions that return EINVAL.

> +__u64 ext2fs_get_generic_bmap_start(ext2fs_generic_bitmap64 bitmap)
> +{
> +	if (!bitmap)
> +		return EINVAL;
> +
> +	if (EXT2FS_IS_32_BITMAP(bitmap)) {
> +		return ext2fs_get_generic_bitmap_start((ext2fs_generic_bitmap)
> +						       bitmap);
> +
> +	}
> +
> +	if (!EXT2FS_IS_64_BITMAP(bitmap))
> +		return EINVAL;
> +
> +	return bitmap->start;
> +}

Hmm, how do you distinguish between EINVAL and an actual start value here?

> +void ext2fs_clear_generic_bmap(ext2fs_generic_bitmap64 bitmap)
> +{
> +	if (EXT2FS_IS_32_BITMAP(bitmap)) {
> +		ext2fs_clear_generic_bitmap((ext2fs_generic_bitmap) bitmap);
> +		return;
> +	}
> +
> +	bitmap->bitmap_ops->clear_bmap (bitmap);
> +}

To be "fail safe" this should probably prefer to believe there is a 32-bit
bitmap (i.e. what is used in all existing applications/deployments) instead
of a 64-bit bitmap.  Failing that, is there a reason the 32-bit bitmap
cannot register a ->clear_bmap() method itself, and this code can never
fail?

> +int ext2fs_test_block_bitmap_range2(ext2fs_block_bitmap64 bmap,
> +				    blk64_t block, int num)
> +{
> +	int	i;
> +
> +	if (!bmap)
> +		return EINVAL;
> +
> +	if (EXT2FS_IS_32_BITMAP(bmap)) {
> +		if ((block+num) & ~0xffffffffULL) {
> +			ext2fs_warn_bitmap2((ext2fs_generic_bitmap) bmap,
> +					    EXT2FS_UNMARK_ERROR, 0xffffffff);
> +			return EINVAL;
> +		}
> +		return ext2fs_test_block_bitmap_range((ext2fs_generic_bitmap) bmap,
> +						      block, num);
> +	}
> +
> +	if (!EXT2FS_IS_64_BITMAP(bmap))
> +		return EINVAL;

Similarly, I don't see how the caller of this code can distinguish between
EINVAL and (what is expected to be a boolean) whether the entire bitmap
range is clear or not.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
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