linux-ext4 - Re: s_first_meta_bg treatment incompatibility between kernel and e2fsprogs

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: <20091115192355.GD4323@mit.edu>
Date:	Sun, 15 Nov 2009 14:23:55 -0500
From:	Theodore Tso <tytso@....edu>
To:	Damien Guibouret <damien.guibouret@...tition-saving.com>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: s_first_meta_bg treatment incompatibility between kernel and
	e2fsprogs

On Sun, Nov 15, 2009 at 11:28:13AM +0100, Damien Guibouret wrote:
> I've open a kernel bug since:  
> http://bugzilla.kernel.org/show_bug.cgi?id=14601
> with a proposed patch (little different from yours but it is matter of  
> taste :)

For future references, patches are less likely to slip through the
cracks if they are sent to the linux-ext4 mailing list as opposed to
having a BZ bug opened.  (Yeah, I know, that's unusual).  The reason
for that is that patches are tracked via patchwork, here:

	http://patchwork.ozlabs.org/project/linux-ext4

Basically, anything that looks like a patch which is sent to
linux-ext4 gets snagged by patchwork, and it's a good place to look
for stuff that hasn't yet been merged.  In some cases there are good
reasons why a patch has been kept out, and in other cases patches have
been merged or definitely rejected and I don't get to getting that
status updated in patchwork, but overall I've found it to work very
well.

As far as the matter of taste issue is concerned, I think we already
have too many static functions with a single caller, and it actually
makes the code harder to understand.  So adding yet another simple
static function I think is a bad thing, not a good thing.

> And I think there is some other places where kernel should be fixed when  
> it uses s_gdb_count (but here my knowledge of the sources are not deep  
> enough to be sure on what shall be performed).

I've looked through the other areas, and the one place where I see a
problem is in the block validity checks in ext4_iget() for the
extended attribute block and in block_validity.c.  The former can and
should be fixed to use the latter.

Here's the fix that I plan to be using.   Comments, anyone?

							- Ted

ext4: fix block validity checks so they work correctly with meta_bg

The block validity checks used by ext4_data_block_valid() wasn't
correctly written to check file systems with the meta_bg feature.  Fix
this.

Signed-off-by: "Theodore Ts'o" <tytso@....edu>
---
 fs/ext4/block_validity.c |    2 +-
 fs/ext4/inode.c          |    5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
index 50784ef..dc79b75 100644
--- a/fs/ext4/block_validity.c
+++ b/fs/ext4/block_validity.c
@@ -160,7 +160,7 @@ int ext4_setup_system_zone(struct super_block *sb)
 		if (ext4_bg_has_super(sb, i) &&
 		    ((i < 5) || ((i % flex_size) == 0)))
 			add_system_zone(sbi, ext4_group_first_block_no(sb, i),
-					sbi->s_gdb_count + 1);
+					ext4_bg_num_gdb(sb, i) + 1);
 		gdp = ext4_get_group_desc(sb, i, NULL);
 		ret = add_system_zone(sbi, ext4_block_bitmap(sb, gdp), 1);
 		if (ret)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index b5cdb88..c62ca93 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4886,10 +4886,7 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)

 	ret = 0;
 	if (ei->i_file_acl &&
-	    ((ei->i_file_acl <
-	      (le32_to_cpu(EXT4_SB(sb)->s_es->s_first_data_block) +
-	       EXT4_SB(sb)->s_gdb_count)) ||
-	     (ei->i_file_acl >= ext4_blocks_count(EXT4_SB(sb)->s_es)))) {
+	    !ext4_data_block_valid(EXT4_SB(sb), ei->i_file_acl, 1)) {
 		ext4_error(sb, __func__,
 			   "bad extended attribute block %llu in inode #%lu",
 			   ei->i_file_acl, inode->i_ino);
--
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