linux-ext4 - Re: [PATCH] Ext4: 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 for Android: free password hash cracker in your pocket

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

Message-ID: <20070919164220.GJ32520@schatzie.adilger.int>
Date:	Wed, 19 Sep 2007 10:42:20 -0600
From:	Andreas Dilger <adilger@...sterfs.com>
To:	Valerie Clement <valerie.clement@...l.net>
Cc:	Avantika Mathur <mathur@...ux.vnet.ibm.com>,
	linux-ext4@...r.kernel.org, cmm@...ibm.com, kalpak@...sterfs.com,
	girish@...sterfs.com
Subject: Re: [PATCH] Ext4: Uninitialized Block Groups

On Sep 19, 2007  14:06 +0200, Valerie Clement wrote:
> I ran some tests with the uninit_groups feature enabled and got error messages
> when running e2fsck on my ext4 partition. e2fsck complains of an "invalid 
> unused inodes count" in some group descriptors.
> These errors occur when checking groups which have only one inode in use. The 
> "free inodes" count has been decremented by one in these groups but not the 
> "unused inodes" count.
> 
> Index: linux-2.6.23-rc6/fs/ext4/ialloc.c
> ===================================================================
> --- linux-2.6.23-rc6.orig/fs/ext4/ialloc.c	2007-09-19 11:31:01.000000000 +0200
> +++ linux-2.6.23-rc6/fs/ext4/ialloc.c	2007-09-19 11:31:41.000000000 +0200
> @@ -633,13 +633,10 @@ got:
>  	/* If we didn't allocate from within the initialized part of the inode
>  	 * table then we need to initialize up to this inode. */
>  	if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
> -		if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT)) {
> +		if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT))
>  			gdp->bg_flags &= cpu_to_le16(~EXT4_BG_INODE_UNINIT);
> -			free = EXT4_INODES_PER_GROUP(sb);
> -		} else {
> -			free = EXT4_INODES_PER_GROUP(sb) -
> +		free = EXT4_INODES_PER_GROUP(sb) -
>  				le16_to_cpu(gdp->bg_itable_unused);
> -		}

Hmm, this is indeed incorrect, but I'm not sure solution is the right one.
I guess in our testing we ran it for a long time and must have created
more than a single inode per group...

What about the following instead?  I think the assumption in the original
code is that "it's a new group, all the inodes are free", but that is not
correct - we want to make NONE of the inodes free initially so that
bt_itable_unused is recalculated below:

		if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT)) {
 			gdp->bg_flags &= cpu_to_le16(~EXT4_BG_INODE_UNINIT);
-			free = EXT4_INODES_PER_GROUP(sb);
+			free = 0;
		} else {
			free = EXT4_INODES_PER_GROUP(sb) -
 				le16_to_cpu(gdp->bg_itable_unused);
		}

		 if (ino > free)
                       gdp->bg_itable_unused =
                                cpu_to_le16(EXT3_INODES_PER_GROUP(sb) - ino);

Still a bit uneasy about off-by-one errors here though.  Is "ino" 0 or
1 for the first inode in the group.  We might need to have a -1 in the
bg_itable_unused calculation still.

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