Re: [PATCH] Fix oops in mballoc caused by a variable overflow

[Valerie Clement <valerie.clement@bull.net>]

Aneesh Kumar K.V wrote:
> What about this  ? I guess we will overflow 
> start = start << bsbits;
> 

Hi Aneesh,
your patch below doesn't fix the issue, because as start_off is also 
loff_t, start_off = ac->ac_o_ex.fe_logical << bsbits  also overflows.

> I guess start should be of type loff_t. Patch below
> 
> -aneesh
> 
> ext4: Fix overflow in ext4_mb_normalize_request
> 
> From: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> 
> kernel BUG at fs/ext4/mballoc.c:3148!
> 
> The BUG_ON is:
> BUG_ON(size <= 0 || size >= EXT4_BLOCKS_PER_GROUP(ac->ac_sb));
> 
> where the value of "size" is 4293920768.
> 
> This is due to the overflow of the variable "start" in the
> ext4_mb_normalize_request() function.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> 
>  fs/ext4/mballoc.c |   21 ++++++++-------------
>  1 files changed, 8 insertions(+), 13 deletions(-)
> 
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index d8cd81e..d8a2db8 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2998,7 +2998,7 @@ static void ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>  	int bsbits, max;
>  	ext4_lblk_t end;
>  	struct list_head *cur;
> -	loff_t size, orig_size;
> +	loff_t size, orig_size, start_off;
>  	ext4_lblk_t start, orig_start;
>  	struct ext4_inode_info *ei = EXT4_I(ac->ac_inode);
>  
> @@ -3039,7 +3039,7 @@ static void ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>  
>  	/* first, try to predict filesize */
>  	/* XXX: should this table be tunable? */
> -	start = 0;
> +	start_off = 0;
>  	if (size <= 16 * 1024) {
>  		size = 16 * 1024;
>  	} else if (size <= 32 * 1024) {
> @@ -3055,26 +3055,21 @@ static void ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>  	} else if (size <= 1024 * 1024) {
>  		size = 1024 * 1024;
>  	} else if (NRL_CHECK_SIZE(size, 4 * 1024 * 1024, max, bsbits)) {
> -		start = ac->ac_o_ex.fe_logical << bsbits;
> -		start = (start / (1024 * 1024)) * (1024 * 1024);
> +		start_off = (ac->ac_o_ex.fe_logical >> (20 - bsbits)) << 20;
>  		size = 1024 * 1024;
>  	} else if (NRL_CHECK_SIZE(size, 8 * 1024 * 1024, max, bsbits)) {
> -		start = ac->ac_o_ex.fe_logical << bsbits;
> -		start = (start / (4 * (1024 * 1024))) * 4 * (1024 * 1024);
> +		start_off = (ac->ac_o_ex.fe_logical >> (22 - bsbits)) << 22;
>  		size = 4 * 1024 * 1024;
>  	} else if (NRL_CHECK_SIZE(ac->ac_o_ex.fe_len,
>  					(8<<20)>>bsbits, max, bsbits)) {
> -		start = ac->ac_o_ex.fe_logical;
> -		start = start << bsbits;
> -		start = (start / (8 * (1024 * 1024))) * 8 * (1024 * 1024);
> +		start_off = (ac->ac_o_ex.fe_logical >> (23 - bsbits)) << 23;
>  		size = 8 * 1024 * 1024;
>  	} else {
> -		start = ac->ac_o_ex.fe_logical;
> -		start = start << bsbits;
> -		size = ac->ac_o_ex.fe_len << bsbits;
> +		start_off = ac->ac_o_ex.fe_logical << bsbits;
> +		size	  = ac->ac_o_ex.fe_len << bsbits;
>  	}
>  	orig_size = size = size >> bsbits;
> -	orig_start = start = start >> bsbits;
> +	orig_start = start = start_off >> bsbits;
>  
>  	/* don't cover already allocated blocks in selected range */
>  	if (ar->pleft && start <= ar->lleft) {
>