[PATCH] ext4: fix wrong size computation in ext4_mb_normalize_request()

[PATCH] ext4: fix wrong size computation in ext4_mb_normalize_request()

[Xiaoguang Wang]

As the member fe_len defined in struct ext4_free_extent is expressed as
number of clusters, the variable "size" computation is wrong, we need to
first translate it to block number, then to bytes.

Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
---
 fs/ext4/mballoc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 7f72f50..9a543b5 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3076,7 +3076,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
 		size = 8 * 1024 * 1024;
 	} else {
 		start_off = (loff_t)ac->ac_o_ex.fe_logical << bsbits;
-		size	  = ac->ac_o_ex.fe_len << bsbits;
+		size	  = EXT4_C2B(EXT4_SB(ac->ac_sb),
+				     ac->ac_o_ex.fe_len) << bsbits;
 	}
 	size = size >> bsbits;
 	start = start_off >> bsbits;
-- 
1.8.2.1

Re: [PATCH] ext4: fix wrong size computation in ext4_mb_normalize_request()

[Lukáš Czerner]

On Thu, 10 Jul 2014, Xiaoguang Wang wrote:

> Date: Thu, 10 Jul 2014 13:40:36 +0800
> From: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> To: linux-ext4@vger.kernel.org
> Cc: tytso@mit.edu, mlombard@redhat.com,
>     Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> Subject: [PATCH] ext4: fix wrong size computation in
>     ext4_mb_normalize_request()
> 
> As the member fe_len defined in struct ext4_free_extent is expressed as
> number of clusters, the variable "size" computation is wrong, we need to
> first translate it to block number, then to bytes.
> 
> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> ---
>  fs/ext4/mballoc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 7f72f50..9a543b5 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3076,7 +3076,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>  		size = 8 * 1024 * 1024;
>  	} else {
>  		start_off = (loff_t)ac->ac_o_ex.fe_logical << bsbits;
> -		size	  = ac->ac_o_ex.fe_len << bsbits;
> +		size	  = EXT4_C2B(EXT4_SB(ac->ac_sb),
> +				     ac->ac_o_ex.fe_len) << bsbits;

There is a patch out there up for more discussion which rewrites a
whole bunch of code in ext4_mb_normalize_request(). But more
importantly this fix, while correct is not going to change anything
since this condition will never be run.

Btw, I take back the correct part since it seems that there is a
a possibility of overflow. This should be better.

		size	  = (loff_t)EXT4_C2B(EXT4_SB(ac->ac_sb),
					     ac->ac_o_ex.fe_len) << bsbits;

Thanks!
-Lukas

>  	}
>  	size = size >> bsbits;
>  	start = start_off >> bsbits;
> 

Re: [PATCH] ext4: fix wrong size computation in ext4_mb_normalize_request()

[Xiaoguang Wang]

Hi,

On 07/10/2014 05:25 PM, Lukáš Czerner wrote:
> On Thu, 10 Jul 2014, Xiaoguang Wang wrote:
> 
>> Date: Thu, 10 Jul 2014 13:40:36 +0800
>> From: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
>> To: linux-ext4@vger.kernel.org
>> Cc: tytso@mit.edu, mlombard@redhat.com,
>>     Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
>> Subject: [PATCH] ext4: fix wrong size computation in
>>     ext4_mb_normalize_request()
>>
>> As the member fe_len defined in struct ext4_free_extent is expressed as
>> number of clusters, the variable "size" computation is wrong, we need to
>> first translate it to block number, then to bytes.
>>
>> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
>> ---
>>  fs/ext4/mballoc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index 7f72f50..9a543b5 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -3076,7 +3076,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>>  		size = 8 * 1024 * 1024;
>>  	} else {
>>  		start_off = (loff_t)ac->ac_o_ex.fe_logical << bsbits;
>> -		size	  = ac->ac_o_ex.fe_len << bsbits;
>> +		size	  = EXT4_C2B(EXT4_SB(ac->ac_sb),
>> +				     ac->ac_o_ex.fe_len) << bsbits;
> 
> There is a patch out there up for more discussion which rewrites a
> whole bunch of code in ext4_mb_normalize_request(). But more
> importantly this fix, while correct is not going to change anything
> since this condition will never be run.

Yeah, I have read your patch and related discussions, indeed this condition
would never be run, thanks for your explanation. But given that currently your
patch is not merged, still this code is wrong and should be fixed.
I'm new to ext4:) Whether should I continue to send a v2 version to fix this issue? thanks!

Regards,
Xiaoguang Wang
> 
> Btw, I take back the correct part since it seems that there is a
> a possibility of overflow. This should be better.
> 
> 		size	  = (loff_t)EXT4_C2B(EXT4_SB(ac->ac_sb),
> 					     ac->ac_o_ex.fe_len) << bsbits;
> 
> Thanks!
> -Lukas
> 
>>  	}
>>  	size = size >> bsbits;
>>  	start = start_off >> bsbits;
>>
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: fix wrong size computation in ext4_mb_normalize_request()

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2675 bytes --]

On Fri, 11 Jul 2014, Xiaoguang Wang wrote:

> Date: Fri, 11 Jul 2014 11:41:08 +0800
> From: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org, tytso@mit.edu, mlombard@redhat.com
> Subject: Re: [PATCH] ext4: fix wrong size computation in
>     ext4_mb_normalize_request()
> 
> Hi,
> 
> On 07/10/2014 05:25 PM, Lukáš Czerner wrote:
> > On Thu, 10 Jul 2014, Xiaoguang Wang wrote:
> > 
> >> Date: Thu, 10 Jul 2014 13:40:36 +0800
> >> From: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> >> To: linux-ext4@vger.kernel.org
> >> Cc: tytso@mit.edu, mlombard@redhat.com,
> >>     Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> >> Subject: [PATCH] ext4: fix wrong size computation in
> >>     ext4_mb_normalize_request()
> >>
> >> As the member fe_len defined in struct ext4_free_extent is expressed as
> >> number of clusters, the variable "size" computation is wrong, we need to
> >> first translate it to block number, then to bytes.
> >>
> >> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> >> ---
> >>  fs/ext4/mballoc.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> >> index 7f72f50..9a543b5 100644
> >> --- a/fs/ext4/mballoc.c
> >> +++ b/fs/ext4/mballoc.c
> >> @@ -3076,7 +3076,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
> >>  		size = 8 * 1024 * 1024;
> >>  	} else {
> >>  		start_off = (loff_t)ac->ac_o_ex.fe_logical << bsbits;
> >> -		size	  = ac->ac_o_ex.fe_len << bsbits;
> >> +		size	  = EXT4_C2B(EXT4_SB(ac->ac_sb),
> >> +				     ac->ac_o_ex.fe_len) << bsbits;
> > 
> > There is a patch out there up for more discussion which rewrites a
> > whole bunch of code in ext4_mb_normalize_request(). But more
> > importantly this fix, while correct is not going to change anything
> > since this condition will never be run.
> 
> Yeah, I have read your patch and related discussions, indeed this condition
> would never be run, thanks for your explanation. But given that currently your
> patch is not merged, still this code is wrong and should be fixed.
> I'm new to ext4:) Whether should I continue to send a v2 version to fix this issue? thanks!

Please do, it's still worth fixing.

Thanks!
-Lukas

> 
> Regards,
> Xiaoguang Wang
> > 
> > Btw, I take back the correct part since it seems that there is a
> > a possibility of overflow. This should be better.
> > 
> > 		size	  = (loff_t)EXT4_C2B(EXT4_SB(ac->ac_sb),
> > 					     ac->ac_o_ex.fe_len) << bsbits;
> > 
> > Thanks!
> > -Lukas
> > 
> >>  	}
> >>  	size = size >> bsbits;
> >>  	start = start_off >> bsbits;
> >>
> > .
> > 
> 
> 

[PATCH v2] ext4: fix wrong size computation in ext4_mb_normalize_request()

[Xiaoguang Wang]

As the member fe_len defined in struct ext4_free_extent is expressed as
number of clusters, the variable "size" computation is wrong, we need to
first translate fe_len to block number, then to bytes.

Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
---
 fs/ext4/mballoc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 7f72f50..975a35e 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3076,7 +3076,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
 		size = 8 * 1024 * 1024;
 	} else {
 		start_off = (loff_t)ac->ac_o_ex.fe_logical << bsbits;
-		size	  = ac->ac_o_ex.fe_len << bsbits;
+		size	  = (loff_t)EXT4_C2B(EXT4_SB(ac->ac_sb),
+				     ac->ac_o_ex.fe_len) << bsbits;
 	}
 	size = size >> bsbits;
 	start = start_off >> bsbits;
-- 
1.8.2.1

Re: [PATCH v2] ext4: fix wrong size computation in ext4_mb_normalize_request()

[Lukáš Czerner]

On Fri, 11 Jul 2014, Xiaoguang Wang wrote:

> Date: Fri, 11 Jul 2014 16:34:20 +0800
> From: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> To: linux-ext4@vger.kernel.org
> Cc: tytso@mit.edu, lczerner@redhat.com,
>     Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> Subject: [PATCH v2] ext4: fix wrong size computation in
>     ext4_mb_normalize_request()
> 
> As the member fe_len defined in struct ext4_free_extent is expressed as
> number of clusters, the variable "size" computation is wrong, we need to
> first translate fe_len to block number, then to bytes.
> 
> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> ---
>  fs/ext4/mballoc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 7f72f50..975a35e 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3076,7 +3076,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>  		size = 8 * 1024 * 1024;
>  	} else {
>  		start_off = (loff_t)ac->ac_o_ex.fe_logical << bsbits;
> -		size	  = ac->ac_o_ex.fe_len << bsbits;
> +		size	  = (loff_t)EXT4_C2B(EXT4_SB(ac->ac_sb),
> +				     ac->ac_o_ex.fe_len) << bsbits;

The indentation is wrong, otherwise it looks good.

Thanks!

Reviewed-by: Lukas Czerner <lczerner@redhat.com>


>  	}
>  	size = size >> bsbits;
>  	start = start_off >> bsbits;
> 

Re: [PATCH v2] ext4: fix wrong size computation in ext4_mb_normalize_request()

[Theodore Ts'o]

On Fri, Jul 11, 2014 at 04:34:20PM +0800, Xiaoguang Wang wrote:
> As the member fe_len defined in struct ext4_free_extent is expressed as
> number of clusters, the variable "size" computation is wrong, we need to
> first translate fe_len to block number, then to bytes.
> 
> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>

Thanks, applied.

						- Ted