Nick Dokos wrote:
Following Andreas's suggestion, I found that ext2fs_set_gdt_csum()
was the culprit: it used an ext2_group_desc pointer to access/set
fields and it did that directly, not through access functions.
I'm testing the patch now, but it'll take a while, so again I'm sending
it out for comments and to possibly try out. Even if it checks out, it
will need some additional care once the flags situation settles down.
cool, thanks.
yep this would explain the bug (any direct access would I guess)
With this on top of my patch stack, my testcase passes, until I allocate
something high enough that I run into yet another 32-bit overflow
somewhere ;)
Ted, want me to collate these into a proper patch series? I have one
more direct access fix as well. (and then things like ext2ed & e2imgge
etc all need a thorough going-over too....)
We really must make these structs opaque or something or it will be
endless pain...
Minor comments below.
Thanks,
Nick
From 6a3b83cda1bcd1e3594515ee888f175bf5cc7906 Mon Sep 17 00:00:00 2001
From: Nick Dokos <nicholas.dokos@xxxxxx>
Date: Wed, 2 Sep 2009 23:00:23 -0400
Subject: [PATCH] Fix ext2fs_set_gdt_csum() to use access functions.
Replace all field accesses with calls to access functions.
Most importantly, get rid of the mis-declared group descriptor
pointer which caused the wrong fields to be updated.
Not quite sure what you mean by this? It worked ok for the "old" size ...
Signed-off-by: Nick Dokos <nicholas.dokos@xxxxxx>
---
lib/ext2fs/csum.c | 30 +++++++++++++++++-------------
1 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/lib/ext2fs/csum.c b/lib/ext2fs/csum.c
index a0f25e3..def3ddd 100644
--- a/lib/ext2fs/csum.c
+++ b/lib/ext2fs/csum.c
@@ -105,7 +105,6 @@ static __u32 find_last_inode_ingrp(ext2fs_inode_bitmap bitmap,
errcode_t ext2fs_set_gdt_csum(ext2_filsys fs)
{
struct ext2_super_block *sb = fs->super;
- struct ext2_group_desc *bg = fs->group_desc;
int dirty = 0;
dgrp_t i;
@@ -116,27 +115,32 @@ errcode_t ext2fs_set_gdt_csum(ext2_filsys fs)
EXT4_FEATURE_RO_COMPAT_GDT_CSUM))
return 0;
- for (i = 0; i < fs->group_desc_count; i++, bg++) {
- int old_csum = bg->bg_checksum;
- int old_unused = bg->bg_itable_unused;
- int old_flags = bg->bg_flags;
+ for (i = 0; i < fs->group_desc_count; i++) {
+ unsigned int old_csum = ext2fs_bg_checksum(fs, i);
+ int old_unused = ext2fs_bg_itable_unused(fs, i);
whitespace problem here, use tabs
+ unsigned int old_flags = ext2fs_bg_flags(fs, i);
+ int old_free_inodes_count = ext2fs_bg_free_inodes_count(fs, i);
ditto
also, strictly, csum & flags are 16 bits, unused is (today) 64 bits ....
+
extra blank line?
- if (bg->bg_free_inodes_count == sb->s_inodes_per_group) {
- bg->bg_flags |= EXT2_BG_INODE_UNINIT;
- bg->bg_itable_unused = sb->s_inodes_per_group;
+ if (old_free_inodes_count == sb->s_inodes_per_group) {
+ ext2fs_bg_flags_set(fs, i, old_flags | EXT2_BG_INODE_UNINIT);
better this I think (though I guess it may change ...):
+ ext2fs_bg_flag_set(fs, i, EXT2_BG_INODE_UNINIT);
+ ext2fs_bg_itable_unused_set(fs, i, sb->s_inodes_per_group);
} else {
- bg->bg_flags &= ~EXT2_BG_INODE_UNINIT;
- bg->bg_itable_unused = sb->s_inodes_per_group -
+ int unused = sb->s_inodes_per_group -
find_last_inode_ingrp(fs->inode_map,
sb->s_inodes_per_group,i);
+
+ ext2fs_bg_flags_set(fs, i, old_flags & ~EXT2_BG_INODE_UNINIT);
and this:
+ ext2fs_flag_clear(fs, i, EXT2_BG_INODE_UNINIT);
+
+ ext2fs_bg_itable_unused_set(fs, i, unused);
}
ext2fs_group_desc_csum_set(fs, i);
- if (old_flags != bg->bg_flags)
+ if (old_flags != ext2fs_bg_flags(fs, i))
dirty = 1;
- if (old_unused != bg->bg_itable_unused)
+ if (old_unused != ext2fs_bg_itable_unused(fs, i))
dirty = 1;
- if (old_csum != bg->bg_checksum)
+ if (old_csum != ext2fs_bg_checksum(fs, i))
dirty = 1;
}
if (dirty)
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
