linux-ext4 - Re: [PATCH] uninitialized block groups

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: <20070828201300.GW5377@schatzie.adilger.int>
Date:	Tue, 28 Aug 2007 14:13:00 -0600
From:	Andreas Dilger <adilger@...sterfs.com>
To:	Avantika Mathur <mathur@...ux.vnet.ibm.com>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: [PATCH] uninitialized block groups

On Aug 22, 2007  22:41 -0700, Avantika Mathur wrote:
> From: Andreas Dilger <adilger@...sterfs.com>
> Signed-off-by: Avantika Mathur <mathur@...ibm.com>

Please change this to a Signed-off-by: for me.

> +unsigned ext4_init_block_bitmap(struct super_block *sb, struct buffer_head *bh,
> +				int block_group, struct ext4_group_desc *gdp)
> +{
> +		/* Set bits for block and inode bitmaps, and inode table */
> +		ext4_set_bit(le32_to_cpu(gdp->bg_block_bitmap) - start,
> +			     bh->b_data);
> +		ext4_set_bit(le32_to_cpu(gdp->bg_inode_bitmap) - start,
> +			     bh->b_data);

These need to be wrapped as ext4_{block,inode}_bitmap(sb, gdp).

One minor thing I never got around to implementing here is to handle the
last group is marked BLOGK_UNINIT.  This shouldn't happen in real life,
because e2fsprogs will never create such a filesystem, but using the
mark_bitmap_end() code (which should be moved to a common location, it
is already duplicated from resize.c) as in ext4_init_inode_bitmap() is
pretty trivial.

That could be done as a separate patch to avoid delaying this one as
it does not affect any normal operation.

> @@ -116,10 +186,22 @@ read_block_bitmap(struct super_block *sb
> -	bh = sb_bread(sb, ext4_block_bitmap(sb, desc));
> +	if (desc->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
> +		bh = sb_getblk(sb, le32_to_cpu(desc->bg_block_bitmap));

Needs to stay as ext4_block_bitmap(sb, desc) as it was before.

> +	} else {
> +		bh = sb_bread(sb, le32_to_cpu(desc->bg_block_bitmap));
> +	}

As above.

> +unsigned ext4_init_inode_bitmap(struct super_block *sb,
> +				struct buffer_head *bh, int block_group,
> +				struct ext4_group_desc *gdp)
> +{
> +	/* If checksum is bad mark all blocks and inodes use to prevent
> +	 * allocation, essentially implementing a per-group read-only flag. */

s/use/used/

> -	bh = sb_bread(sb, ext4_inode_bitmap(sb, desc));
> +	if (desc->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT)) {
> +		bh = sb_getblk(sb, le32_to_cpu(desc->bg_inode_bitmap));

Needs to stay as ext4_inode_bitmap(sb, desc)

> +	} else {
> +		bh = sb_bread(sb, le32_to_cpu(desc->bg_inode_bitmap));
> +	}

As above.

> +__le16 ext4_group_desc_csum(struct ext4_sb_info *sbi, __u32 block_group,
> +			    struct ext4_group_desc *gdp)
> +{
> +			crc = crc16(crc, (__u8 *)gdp + offset,
> +				    le16_to_cpu(sbi->s_es->s_desc_size)
> +					- offset);

Please put '-' operator at end of the previous line.

> +int ext4_group_desc_csum_verify(struct ext4_sb_info *sbi, __u32 block_group,
> +				struct ext4_group_desc *gdp)
> +{
> +	if ((sbi->s_es->s_feature_ro_compat &
> +	     cpu_to_le32(EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) &&
> +	     (gdp->bg_checksum != ext4_group_desc_csum(sbi, block_group, gdp)))

This line should be indented to the left one space, since it is a different
condition than "(sbi->s_es ...)".

>  struct ext4_group_desc
>  {
>  	__u16	bg_flags;

Can you please add a comment for this	/* EXT4_BG_flags (INODE_UNINIT, etc) */

> -	__u32	bg_reserved[3];
> +	__u32	bg_reserved[2];

Likewise, for the above			/* Likely block/inode bitmap checksum */

> +	__le16  bg_itable_unused;       /* Unused inodes count */
> +	__le16  bg_checksum;            /* crc16(sb_uuid+group+desc) */

The above are using spaces instead of tabs for alignment of the comments?

> +#define EXT4_BG_INODE_UNINIT  0x0001 /* Inode table/bitmap not in use */
> +#define EXT4_BG_BLOCK_UNINIT  0x0002 /* Block bitmap not in use */
> +#define EXT4_BG_INODE_ZEROED  0x0004 /* On-disk itable initialized to zero */

Likewise, the above have spaces instead of tabs for alignment.

> @@ -734,6 +740,7 @@ static inline int ext4_valid_inum(struct
>  #define EXT4_FEATURE_RO_COMPAT_BTREE_DIR	0x0004
> +#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM		0x0010
>  #define EXT4_FEATURE_RO_COMPAT_DIR_NLINK	0x0020
>  #define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE	0x0040
>  
> @@ -755,6 +762,7 @@ static inline int ext4_valid_inum(struct
>  					 EXT4_FEATURE_RO_COMPAT_LARGE_FILE| \
>  					 EXT4_FEATURE_RO_COMPAT_DIR_NLINK | \
>  					 EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE | \
> +					 EXT4_FEATURE_RO_COMPAT_GDT_CSUM| \

This should probably be put before DIR_NLINK to keep numerical order.

I think this is ready to be added to the patch queue.  Ted, if you could
merge in the uninit groups patches to e2fsprogs that would be much
appreciated (which incidentally fixes the LAZY_BG-with-RESIZE_INODE support
and is needed for Jose's FLEX_BG too).

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, 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