[PATCH] Fix ext2fs_set_gdt_csum() to use access functions.

[PATCH] Fix ext2fs_set_gdt_csum() to use access functions.

[Nick Dokos]

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.

Thanks,
Nick

>From 6a3b83cda1bcd1e3594515ee888f175bf5cc7906 Mon Sep 17 00:00:00 2001
From: Nick Dokos <nicholas.dokos@hp.com>
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.

Signed-off-by: Nick Dokos <nicholas.dokos@hp.com>
---
 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);
+		unsigned int old_flags = ext2fs_bg_flags(fs, i);
+                int old_free_inodes_count = ext2fs_bg_free_inodes_count(fs, i);
+                
 
-		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);
+			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);
+			
+			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)
-- 
1.6.0.6

Re: [PATCH] Fix ext2fs_set_gdt_csum() to use access functions.

[Eric Sandeen]

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@hp.com>
> 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@hp.com>
> ---
>  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)

Re: [PATCH] Fix ext2fs_set_gdt_csum() to use access functions.

[Nick Dokos]

Eric Sandeen <sandeen@redhat.com> wrote:

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

Yes, it worked fine for the old ext2_group_desc structure, but it has no
hope of working with the ext4_group_desc: the sizes are different, so
doing bg++ gets it pointing somewhere within the first descriptor, not
to the beginning of the second. That's how it ended up modifying
reserved fields.

Sorry about the whitespace problems.  As for the flags, I didn't worry
too much about how to set them: I figured that once the dust settles,
we'll follow the proper convention.

Thanks for looking it over and testing too! We'll see whether Justin's
problem disappears.

Nick

Re: [PATCH] Fix ext2fs_set_gdt_csum() to use access functions.

[Eric Sandeen]

Nick Dokos wrote:
> Eric Sandeen <sandeen@redhat.com> wrote:
> 
>>> 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 ...
>>
> 
> Yes, it worked fine for the old ext2_group_desc structure, but it has no
> hope of working with the ext4_group_desc: the sizes are different, so
> doing bg++ gets it pointing somewhere within the first descriptor, not
> to the beginning of the second. That's how it ended up modifying
> reserved fields.

right.  Ok, I was just confused when you said "mis-declared" - thought 
you meant something was wrong with how bg was set or initialized.

> Sorry about the whitespace problems.  As for the flags, I didn't worry
> too much about how to set them: I figured that once the dust settles,
> we'll follow the proper convention.

sounds good.

Thanks,
-Eric

> Thanks for looking it over and testing too! We'll see whether Justin's
> problem disappears.
> 
> Nick

Re: [PATCH] Fix ext2fs_set_gdt_csum() to use access functions.

[Justin Maggard]

On Wed, Sep 2, 2009 at 10:11 PM, Nick Dokos<nicholas.dokos@hp.com> wrote:
>
> Thanks for looking it over and testing too! We'll see whether Justin's
> problem disappears.

Yes, it did!  Thanks everyone for taking the time to look into this
and get out a fix!

-Justin

Re: [PATCH] Fix ext2fs_set_gdt_csum() to use access functions.

[Andreas Dilger]

On Sep 02, 2009  23:21 -0400, Nick Dokos wrote:
> 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.
> 
>  errcode_t ext2fs_set_gdt_csum(ext2_filsys fs)
>  {
>  	struct ext2_super_block *sb = fs->super;
> -	struct ext2_group_desc *bg = fs->group_desc;

Given the danger of ongoing direct access to fs->group_desc (including
potentially from external applications) I think the only safe way of
doing this is to have an opaque fs->group_desc structure that cannot
be dereferenced outside of the library.

As a potential compatibility measure, we might consider fs->group_desc
to be valid for 32-bit filesystems, and leave it NULL for 64-bit
filesystems and use a second (opaque) fs->group_desc64 pointer for
access to filesystems with INCOMPAT_64BIT set.


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

Re: [PATCH] Fix ext2fs_set_gdt_csum() to use access functions.

[Nick Dokos]

Forgot to copy the list...

------- Forwarded Message

Date:    Thu, 03 Sep 2009 01:20:00 -0400
From:    Nick Dokos <nicholas.dokos@hp.com>
To:      Eric Sandeen <sandeen@redhat.com>
cc:      nicholas.dokos@hp.com
Subject: Re: [PATCH] Fix ext2fs_set_gdt_csum() to use access functions.

> Eric Sandeen <sandeen@redhat.com> wrote:
> 
> ...
> right.  Ok, I was just confused when you said "mis-declared" - thought 
> you meant something was wrong with how bg was set or initialized.
> 
Oh, I see: yes, "mis-declared" is not right - it should not exist
at all!-) I'll rephrase it somehow.

Thanks,
Nick


------- End of Forwarded Message