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
[email protected] 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
[email protected] 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
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 <[email protected]>
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,
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 <[email protected]>
>
> 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/ext2-devel
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.
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