Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Mingming Cao]

On Fri, 2006-08-11 at 17:13 -0500, Eric Sandeen wrote:
> I figure before we get too fired up about 48 bits and ext4 let's fix 32 bits on ext3 :)
> 
> I need to do some actual IO testing now, but this gets things mounting for a 16T ext3 filesystem.
> (patched up e2fsprogs is needed too, I'll send that off the kernel list)
> 
> This patch fixes these issues in the kernel:
> 
> o sbi->s_groups_count overflows in ext3_fill_super()
> 
> 	sbi->s_groups_count = (le32_to_cpu(es->s_blocks_count) -
> 			       le32_to_cpu(es->s_first_data_block) +
> 			       EXT3_BLOCKS_PER_GROUP(sb) - 1) /
> 			      EXT3_BLOCKS_PER_GROUP(sb);
> 
> at 16T, s_blocks_count is already maxed out; adding EXT3_BLOCKS_PER_GROUP(sb) overflows it and 
> groups_count comes out to 0.  Not really what we want, and causes a failed mount.
> 
> Feel free to check my math (actually, please do!), but changing it this way should work & 
> avoid the overflow:
> 
> (A + B - 1)/B changed to: ((A - 1)/B) + 1
> 
> o ext3_check_descriptors() overflows range checks

Looks correct to me.:)

Mingming

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Eric Sandeen]

Andrew Morton wrote:
> On Fri, 11 Aug 2006 17:13:14 -0500
> Eric Sandeen <esandeen@redhat.com> wrote:
> 
>> (a similar patch could be done for ext2; does anyone in their right mind use ext2 at 16T?
> 
> well, a bug's a bug.  People might want to ue 16TB ext2 for comparative
> performance testing, or because they get their jollies from running fsck or
> something.

ext2 and ext3 have seemingly already diverged a bit, but I suppose no reason to 
let it go further.

>> I'll send an ext2 patch doing the same thing if that's warranted)
> 
> please, when you have nothing better to do ;)

Will do :)

-Eric

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Johann Lombardi]

>  static int ext3_check_descriptors (struct super_block * sb)
>  {
>  	struct ext3_sb_info *sbi = EXT3_SB(sb);
> -	ext3_fsblk_t block = le32_to_cpu(sbi->s_es->s_first_data_block);
> +	ext3_fsblk_t first_block = le32_to_cpu(sbi->s_es->s_first_data_block);
> +	ext3_fsblk_t last_block;
>  	struct ext3_group_desc * gdp = NULL;
>  	int desc_block = 0;
>  	int i;
> @@ -1141,12 +1142,16 @@ static int ext3_check_descriptors (struc
>  
>  	for (i = 0; i < sbi->s_groups_count; i++)
>  	{
> +		if (i == sbi->s_groups_count - 1)
> +			last_block = sb->s_blocks_count - 1;
> +		else
> +			last = first + (EXT3_BLOCKS_PER_GROUP(sb) - 1);

I think it should be:
last_block = first_block + ...

Johann

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Andreas Dilger]

On Aug 11, 2006  15:29 -0700, Mingming Cao wrote:
> On Fri, 2006-08-11 at 17:13 -0500, Eric Sandeen wrote:
> > I figure before we get too fired up about 48 bits and ext4 let's fix 32 bits on ext3 :)
> > 
> > I need to do some actual IO testing now, but this gets things mounting for a 16T ext3 filesystem.
> > (patched up e2fsprogs is needed too, I'll send that off the kernel list)
> > 
> > This patch fixes these issues in the kernel:
> > 
> > o sbi->s_groups_count overflows in ext3_fill_super()
> > 
> > 	sbi->s_groups_count = (le32_to_cpu(es->s_blocks_count) -
> > 			       le32_to_cpu(es->s_first_data_block) +
> > 			       EXT3_BLOCKS_PER_GROUP(sb) - 1) /
> > 			      EXT3_BLOCKS_PER_GROUP(sb);
> > 
> > at 16T, s_blocks_count is already maxed out; adding EXT3_BLOCKS_PER_GROUP(sb) overflows it and 
> > groups_count comes out to 0.  Not really what we want, and causes a failed mount.
> > 
> > Feel free to check my math (actually, please do!), but changing it this way should work & 
> > avoid the overflow:
> > 
> > (A + B - 1)/B changed to: ((A - 1)/B) + 1
> > 
> > o ext3_check_descriptors() overflows range checks

This is generally not safe because of underflow, but it also isn't possible
to have a 0-block filesystem so it should be OK.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[sho]

Hi,

>I figure before we get too fired up about 48 bits and ext4 let's fix 32 bits on ext3 :)
>
>I need to do some actual IO testing now, but this gets things mounting for a 16T ext3 
>filesystem.
>(patched up e2fsprogs is needed too, I'll send that off the kernel list)
>
>This patch fixes these issues in the kernel:
> 
> o sbi->s_groups_count overflows in ext3_fill_super()
> 
> sbi->s_groups_count = (le32_to_cpu(es->s_blocks_count) -
>        le32_to_cpu(es->s_first_data_block) +
>        EXT3_BLOCKS_PER_GROUP(sb) - 1) /
>       EXT3_BLOCKS_PER_GROUP(sb);
> 
> at 16T, s_blocks_count is already maxed out; adding EXT3_BLOCKS_PER_GROUP(sb) overflows it and 
> groups_count comes out to 0.  Not really what we want, and causes a failed mount.
> 
> Feel free to check my math (actually, please do!), but changing it this way should work & 
> avoid the overflow:
> 
> (A + B - 1)/B changed to: ((A - 1)/B) + 1
> 
> o ext3_check_descriptors() overflows range checks

I had done the same work for both ext3 and ext2,
and sent the patches to ext2-devel three months ago.
If you need, you can get them from the following URL.

The introduction for my patch set:
http://marc.theaimsgroup.com/?l=ext2-devel&m=114856080926308&w=2

The patch which fixes overflow problem on ext3:
http://marc.theaimsgroup.com/?l=ext2-devel&m=114856129220863&w=2

The patch which fixes overflow problem on ext2:
http://marc.theaimsgroup.com/?l=ext2-devel&m=114856135120693&w=2

I have reviewed your patch and found other place which might
cause overflow as below.  If group_first_block is the first block of
the last group, overflow will occur.  This has already been fixed
in my patch.

o ext3_try_to_allocate_with_rsv() in fs/ext3/balloc.c
	if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
		    || (my_rsv->rsv_end < group_first_block))
			BUG();

Cheers, Takashi

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Eric Sandeen]

sho@tnes.nec.co.jp wrote:
> Hi,
> 
> I had done the same work for both ext3 and ext2,
> and sent the patches to ext2-devel three months ago.
> If you need, you can get them from the following URL.

Thank you, I did not know that anyone else had done this work (I guess I should 
have searched...!)

Thanks for the pointer, I will compare my patches to yours.  I guess it can be 
considered an independent review. :)

> I have reviewed your patch and found other place which might
> cause overflow as below.  If group_first_block is the first block of
> the last group, overflow will occur.  This has already been fixed
> in my patch.

I had that patch locally as well, but had not yet sent it out.

Thanks,
-Eric

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Mingming Cao]

sho@tnes.nec.co.jp wrote:

> I have reviewed your patch and found other place which might
> cause overflow as below.  If group_first_block is the first block of
> the last group, overflow will occur.  This has already been fixed
> in my patch.
> 
> o ext3_try_to_allocate_with_rsv() in fs/ext3/balloc.c
> 	if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
> 		    || (my_rsv->rsv_end < group_first_block))
> 			BUG();
> 

Yes, this isn't being addressed in the current 2.6.18-rc4 kernel. I 
think this is better than casting to unsigned long long:

- 	if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
+ 	if ((my_rsv->rsv_start > group_first_block - 1 + 
EXT3_BLOCKS_PER_GROUP(sb))


Thanks,

Mingming

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Eric Sandeen]

Mingming Cao wrote:

> Yes, this isn't being addressed in the current 2.6.18-rc4 kernel. I 
> think this is better than casting to unsigned long long:
> 
> -     if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
> +     if ((my_rsv->rsv_start > group_first_block - 1 + EXT3_BLOCKS_PER_GROUP(sb))

And there are a few more of these.  The patch I currently have in my stack follows.
(personally I think
	last = first + (count - 1)
is clearer than
	last = first - 1 + count
but that's just my opinion...)

Thanks,

-Eric

Signed-off-by: Eric Sandeen <esandeen@redhat.com>

Index: linux-2.6.17/fs/ext3/balloc.c
===================================================================
--- linux-2.6.17.orig/fs/ext3/balloc.c
+++ linux-2.6.17/fs/ext3/balloc.c
@@ -168,7 +168,7 @@ goal_in_my_reservation(struct ext3_reser
 	ext3_fsblk_t group_first_block, group_last_block;
 
 	group_first_block = ext3_group_first_block_no(sb, group);
-	group_last_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
+	group_last_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
 
 	if ((rsv->_rsv_start > group_last_block) ||
 	    (rsv->_rsv_end < group_first_block))
@@ -897,7 +897,7 @@ static int alloc_new_reservation(struct 
 	spinlock_t *rsv_lock = &EXT3_SB(sb)->s_rsv_window_lock;
 
 	group_first_block = ext3_group_first_block_no(sb, group);
-	group_end_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
+	group_end_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
 
 	if (grp_goal < 0)
 		start_block = group_first_block;
@@ -1132,7 +1132,7 @@ ext3_try_to_allocate_with_rsv(struct sup
 			try_to_extend_reservation(my_rsv, sb,
 					*count-my_rsv->rsv_end + grp_goal - 1);
 
-		if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
+		if ((my_rsv->rsv_start > group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1))
 		    || (my_rsv->rsv_end < group_first_block))
 			BUG();
 		ret = ext3_try_to_allocate(sb, handle, group, bitmap_bh, grp_goal,

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Mingming Cao]

On Fri, 2006-08-18 at 12:39 -0500, Eric Sandeen wrote:
> Mingming Cao wrote:
> 
> > Yes, this isn't being addressed in the current 2.6.18-rc4 kernel. I 
> > think this is better than casting to unsigned long long:
> > 
> > -     if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
> > +     if ((my_rsv->rsv_start > group_first_block - 1 + EXT3_BLOCKS_PER_GROUP(sb))
> 
> And there are a few more of these.  The patch I currently have in my stack follows.
> (personally I think
> 	last = first + (count - 1)
> is clearer than
> 	last = first - 1 + count
> but that's just my opinion...)
> 
> Thanks,
> 

ACK.
> -Eric
> 
> Signed-off-by: Eric Sandeen <esandeen@redhat.com>
> 
> Index: linux-2.6.17/fs/ext3/balloc.c
> ===================================================================
> --- linux-2.6.17.orig/fs/ext3/balloc.c
> +++ linux-2.6.17/fs/ext3/balloc.c
> @@ -168,7 +168,7 @@ goal_in_my_reservation(struct ext3_reser
>  	ext3_fsblk_t group_first_block, group_last_block;
>  
>  	group_first_block = ext3_group_first_block_no(sb, group);
> -	group_last_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
> +	group_last_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
>  
>  	if ((rsv->_rsv_start > group_last_block) ||
>  	    (rsv->_rsv_end < group_first_block))
> @@ -897,7 +897,7 @@ static int alloc_new_reservation(struct 
>  	spinlock_t *rsv_lock = &EXT3_SB(sb)->s_rsv_window_lock;
>  
>  	group_first_block = ext3_group_first_block_no(sb, group);
> -	group_end_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
> +	group_end_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
>  
>  	if (grp_goal < 0)
>  		start_block = group_first_block;
> @@ -1132,7 +1132,7 @@ ext3_try_to_allocate_with_rsv(struct sup
>  			try_to_extend_reservation(my_rsv, sb,
>  					*count-my_rsv->rsv_end + grp_goal - 1);
>  
> -		if ((my_rsv->rsv_start >= group_first_block + EXT3_BLOCKS_PER_GROUP(sb))
> +		if ((my_rsv->rsv_start > group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1))
>  		    || (my_rsv->rsv_end < group_first_block))
>  			BUG();
>  		ret = ext3_try_to_allocate(sb, handle, group, bitmap_bh, grp_goal,
> 
> 
> 
> -------------------------------------------------------------------------
> Using Tomcat but need to do more? Need to support web services, security?
> Get stuff done quickly with pre-integrated technology to make your job easier
> Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
> http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
> _______________________________________________
> Ext2-devel mailing list
> Ext2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ext2-devel

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Andreas Dilger]

On Aug 18, 2006  12:39 -0500, Eric Sandeen wrote:
> @@ -168,7 +168,7 @@ goal_in_my_reservation(struct ext3_reser
>  	ext3_fsblk_t group_first_block, group_last_block;
>  
>  	group_first_block = ext3_group_first_block_no(sb, group);
> -	group_last_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
> +	group_last_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
>  
>  	if ((rsv->_rsv_start > group_last_block) ||
>  	    (rsv->_rsv_end < group_first_block))
> @@ -897,7 +897,7 @@ static int alloc_new_reservation(struct 
>  	spinlock_t *rsv_lock = &EXT3_SB(sb)->s_rsv_window_lock;
>  
>  	group_first_block = ext3_group_first_block_no(sb, group);
> -	group_end_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
> +	group_end_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
>  
>  	if (grp_goal < 0)
>  		start_block = group_first_block;

I don't see how these can make a difference?  Surely, if the intermediate
sum overflows it will then underflow when "- 1" is done?  Not that I mind,
per-se, just curious why you think this fixes anything.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Re: [Ext2-devel] [PATCH] fix ext3 mounts at 16T

[Eric Sandeen]

Andreas Dilger wrote:
> On Aug 18, 2006  12:39 -0500, Eric Sandeen wrote:
>> @@ -168,7 +168,7 @@ goal_in_my_reservation(struct ext3_reser
>>  	ext3_fsblk_t group_first_block, group_last_block;
>>  
>>  	group_first_block = ext3_group_first_block_no(sb, group);
>> -	group_last_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
>> +	group_last_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
>>  
>>  	if ((rsv->_rsv_start > group_last_block) ||
>>  	    (rsv->_rsv_end < group_first_block))
>> @@ -897,7 +897,7 @@ static int alloc_new_reservation(struct 
>>  	spinlock_t *rsv_lock = &EXT3_SB(sb)->s_rsv_window_lock;
>>  
>>  	group_first_block = ext3_group_first_block_no(sb, group);
>> -	group_end_block = group_first_block + EXT3_BLOCKS_PER_GROUP(sb) - 1;
>> +	group_end_block = group_first_block + (EXT3_BLOCKS_PER_GROUP(sb) - 1);
>>  
>>  	if (grp_goal < 0)
>>  		start_block = group_first_block;
> 
> I don't see how these can make a difference?  Surely, if the intermediate
> sum overflows it will then underflow when "- 1" is done?  Not that I mind,
> per-se, just curious why you think this fixes anything.

Well, you're right, if it overflows then it will underflow again.  And I've not 
observed any actual failures, and I don't expect to.  But personally I guess I'd 
rather avoid the whole overflow in the first place... maybe I'm being silly.  :)

If you think it's unnecessary code churn then we can not make this change...

-Eric