[PATCH] block: fix get_max_segment_size() overflow on 32bit arch

[PATCH] block: fix get_max_segment_size() overflow on 32bit arch

[Ming Lei]

Commit 429120f3df2d starts to take account of segment's start dma address
when computing max segment size, and data type of 'unsigned long'
is used to do that. However, the segment mask may be 0xffffffff, so
the figured out segment size may be overflowed because DMA address can
be 64bit on 32bit arch.

Fixes the issue by using 'unsigned long long' to compute max segment
size.

Fixes: 429120f3df2d ("block: fix splitting segments on boundary masks")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-merge.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 347782a24a35..b0fcc72594cb 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -159,12 +159,12 @@ static inline unsigned get_max_io_size(struct request_queue *q,
 
 static inline unsigned get_max_segment_size(const struct request_queue *q,
 					    struct page *start_page,
-					    unsigned long offset)
+					    unsigned long long offset)
 {
 	unsigned long mask = queue_segment_boundary(q);
 
 	offset = mask & (page_to_phys(start_page) + offset);
-	return min_t(unsigned long, mask - offset + 1,
+	return min_t(unsigned long long, mask - offset + 1,
 		     queue_max_segment_size(q));
 }
 
-- 
2.20.1

Re: [PATCH] block: fix get_max_segment_size() overflow on 32bit arch

[Damien Le Moal]

On 2020/01/08 10:25, Ming Lei wrote:
> Commit 429120f3df2d starts to take account of segment's start dma address
> when computing max segment size, and data type of 'unsigned long'
> is used to do that. However, the segment mask may be 0xffffffff, so
> the figured out segment size may be overflowed because DMA address can
> be 64bit on 32bit arch.
> 
> Fixes the issue by using 'unsigned long long' to compute max segment
> size.
> 
> Fixes: 429120f3df2d ("block: fix splitting segments on boundary masks")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-merge.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 347782a24a35..b0fcc72594cb 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -159,12 +159,12 @@ static inline unsigned get_max_io_size(struct request_queue *q,
>  
>  static inline unsigned get_max_segment_size(const struct request_queue *q,
>  					    struct page *start_page,
> -					    unsigned long offset)
> +					    unsigned long long offset)
>  {
>  	unsigned long mask = queue_segment_boundary(q);
>  
>  	offset = mask & (page_to_phys(start_page) + offset);

Shouldn't mask be an unsigned long long too for this to give the
expected correct result ?

> -	return min_t(unsigned long, mask - offset + 1,
> +	return min_t(unsigned long long, mask - offset + 1,
>  		     queue_max_segment_size(q));
>  }
>  
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: fix get_max_segment_size() overflow on 32bit arch

[Jens Axboe]

On 1/7/20 7:06 PM, Damien Le Moal wrote:
> On 2020/01/08 10:25, Ming Lei wrote:
>> Commit 429120f3df2d starts to take account of segment's start dma address
>> when computing max segment size, and data type of 'unsigned long'
>> is used to do that. However, the segment mask may be 0xffffffff, so
>> the figured out segment size may be overflowed because DMA address can
>> be 64bit on 32bit arch.
>>
>> Fixes the issue by using 'unsigned long long' to compute max segment
>> size.
>>
>> Fixes: 429120f3df2d ("block: fix splitting segments on boundary masks")
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>> ---
>>  block/blk-merge.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index 347782a24a35..b0fcc72594cb 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -159,12 +159,12 @@ static inline unsigned get_max_io_size(struct request_queue *q,
>>  
>>  static inline unsigned get_max_segment_size(const struct request_queue *q,
>>  					    struct page *start_page,
>> -					    unsigned long offset)
>> +					    unsigned long long offset)
>>  {
>>  	unsigned long mask = queue_segment_boundary(q);
>>  
>>  	offset = mask & (page_to_phys(start_page) + offset);
> 
> Shouldn't mask be an unsigned long long too for this to give the
> expected correct result ?

Don't think so, and the seg boundary is a ulong to begin with as well.

-- 
Jens Axboe

Re: [PATCH] block: fix get_max_segment_size() overflow on 32bit arch

[Damien Le Moal]

On 2020/01/08 11:34, Jens Axboe wrote:
> On 1/7/20 7:06 PM, Damien Le Moal wrote:
>> On 2020/01/08 10:25, Ming Lei wrote:
>>> Commit 429120f3df2d starts to take account of segment's start dma address
>>> when computing max segment size, and data type of 'unsigned long'
>>> is used to do that. However, the segment mask may be 0xffffffff, so
>>> the figured out segment size may be overflowed because DMA address can
>>> be 64bit on 32bit arch.
>>>
>>> Fixes the issue by using 'unsigned long long' to compute max segment
>>> size.
>>>
>>> Fixes: 429120f3df2d ("block: fix splitting segments on boundary masks")
>>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>>  block/blk-merge.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>>> index 347782a24a35..b0fcc72594cb 100644
>>> --- a/block/blk-merge.c
>>> +++ b/block/blk-merge.c
>>> @@ -159,12 +159,12 @@ static inline unsigned get_max_io_size(struct request_queue *q,
>>>  
>>>  static inline unsigned get_max_segment_size(const struct request_queue *q,
>>>  					    struct page *start_page,
>>> -					    unsigned long offset)
>>> +					    unsigned long long offset)
>>>  {
>>>  	unsigned long mask = queue_segment_boundary(q);
>>>  
>>>  	offset = mask & (page_to_phys(start_page) + offset);
>>
>> Shouldn't mask be an unsigned long long too for this to give the
>> expected correct result ?
> 
> Don't think so, and the seg boundary is a ulong to begin with as well.
> 

I was referring to 32bits arch were ulong is 32bits. So we would have

offset = 32bits & 64bits;

with the patch applied. But I am not sure how gcc handles that and if
this can be a problem.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: fix get_max_segment_size() overflow on 32bit arch

[Ming Lei]

On Wed, Jan 08, 2020 at 02:06:25AM +0000, Damien Le Moal wrote:
> On 2020/01/08 10:25, Ming Lei wrote:
> > Commit 429120f3df2d starts to take account of segment's start dma address
> > when computing max segment size, and data type of 'unsigned long'
> > is used to do that. However, the segment mask may be 0xffffffff, so
> > the figured out segment size may be overflowed because DMA address can
> > be 64bit on 32bit arch.
> > 
> > Fixes the issue by using 'unsigned long long' to compute max segment
> > size.
> > 
> > Fixes: 429120f3df2d ("block: fix splitting segments on boundary masks")
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Tested-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-merge.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > index 347782a24a35..b0fcc72594cb 100644
> > --- a/block/blk-merge.c
> > +++ b/block/blk-merge.c
> > @@ -159,12 +159,12 @@ static inline unsigned get_max_io_size(struct request_queue *q,
> >  
> >  static inline unsigned get_max_segment_size(const struct request_queue *q,
> >  					    struct page *start_page,
> > -					    unsigned long offset)
> > +					    unsigned long long offset)
> >  {
> >  	unsigned long mask = queue_segment_boundary(q);
> >  
> >  	offset = mask & (page_to_phys(start_page) + offset);
> 
> Shouldn't mask be an unsigned long long too for this to give the
> expected correct result ?

queue_segment_boundary() always returns 'unsigned long', so far not
see issue related with that.


Thanks,
Ming

Re: [PATCH] block: fix get_max_segment_size() overflow on 32bit arch

[Ming Lei]

On Wed, Jan 08, 2020 at 02:38:02AM +0000, Damien Le Moal wrote:
> On 2020/01/08 11:34, Jens Axboe wrote:
> > On 1/7/20 7:06 PM, Damien Le Moal wrote:
> >> On 2020/01/08 10:25, Ming Lei wrote:
> >>> Commit 429120f3df2d starts to take account of segment's start dma address
> >>> when computing max segment size, and data type of 'unsigned long'
> >>> is used to do that. However, the segment mask may be 0xffffffff, so
> >>> the figured out segment size may be overflowed because DMA address can
> >>> be 64bit on 32bit arch.
> >>>
> >>> Fixes the issue by using 'unsigned long long' to compute max segment
> >>> size.
> >>>
> >>> Fixes: 429120f3df2d ("block: fix splitting segments on boundary masks")
> >>> Reported-by: Guenter Roeck <linux@roeck-us.net>
> >>> Tested-by: Guenter Roeck <linux@roeck-us.net>
> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >>> ---
> >>>  block/blk-merge.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/block/blk-merge.c b/block/blk-merge.c
> >>> index 347782a24a35..b0fcc72594cb 100644
> >>> --- a/block/blk-merge.c
> >>> +++ b/block/blk-merge.c
> >>> @@ -159,12 +159,12 @@ static inline unsigned get_max_io_size(struct request_queue *q,
> >>>  
> >>>  static inline unsigned get_max_segment_size(const struct request_queue *q,
> >>>  					    struct page *start_page,
> >>> -					    unsigned long offset)
> >>> +					    unsigned long long offset)
> >>>  {
> >>>  	unsigned long mask = queue_segment_boundary(q);
> >>>  
> >>>  	offset = mask & (page_to_phys(start_page) + offset);
> >>
> >> Shouldn't mask be an unsigned long long too for this to give the
> >> expected correct result ?
> > 
> > Don't think so, and the seg boundary is a ulong to begin with as well.
> > 
> 
> I was referring to 32bits arch were ulong is 32bits. So we would have
> 
> offset = 32bits & 64bits;
> 
> with the patch applied. But I am not sure how gcc handles that and if
> this can be a problem.

Both are 'unsigned', so C compiler will do zero extension for 32bits.


Thanks,
Ming

Re: [PATCH] block: fix get_max_segment_size() overflow on 32bit arch

[Guenter Roeck]

On 1/7/20 6:38 PM, Damien Le Moal wrote:
> On 2020/01/08 11:34, Jens Axboe wrote:
>> On 1/7/20 7:06 PM, Damien Le Moal wrote:
>>> On 2020/01/08 10:25, Ming Lei wrote:
>>>> Commit 429120f3df2d starts to take account of segment's start dma address
>>>> when computing max segment size, and data type of 'unsigned long'
>>>> is used to do that. However, the segment mask may be 0xffffffff, so
>>>> the figured out segment size may be overflowed because DMA address can
>>>> be 64bit on 32bit arch.
>>>>
>>>> Fixes the issue by using 'unsigned long long' to compute max segment
>>>> size.
>>>>
>>>> Fixes: 429120f3df2d ("block: fix splitting segments on boundary masks")
>>>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>>>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>> ---
>>>>   block/blk-merge.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>>>> index 347782a24a35..b0fcc72594cb 100644
>>>> --- a/block/blk-merge.c
>>>> +++ b/block/blk-merge.c
>>>> @@ -159,12 +159,12 @@ static inline unsigned get_max_io_size(struct request_queue *q,
>>>>   
>>>>   static inline unsigned get_max_segment_size(const struct request_queue *q,
>>>>   					    struct page *start_page,
>>>> -					    unsigned long offset)
>>>> +					    unsigned long long offset)
>>>>   {
>>>>   	unsigned long mask = queue_segment_boundary(q);
>>>>   
>>>>   	offset = mask & (page_to_phys(start_page) + offset);
>>>
>>> Shouldn't mask be an unsigned long long too for this to give the
>>> expected correct result ?
>>
>> Don't think so, and the seg boundary is a ulong to begin with as well.
>>
> 
> I was referring to 32bits arch were ulong is 32bits. So we would have
> 
> offset = 32bits & 64bits;
> 
> with the patch applied. But I am not sure how gcc handles that and if
> this can be a problem.
> 

Type extension is well defined in the C standard.

The underlying problem here is that mask is 0xffffffff, and
page_to_phys(start_page) as well as offset are sometimes 0.
In this situation, mask - offset + 1 is 0 if offset is a 32 bit
variable, and 0x100000000 if offset is a 64 bit variable.
In the first case, this results in a wrong maximum segment
size of 0.

Guenter

Re: [PATCH] block: fix get_max_segment_size() overflow on 32bit arch

[Damien Le Moal]

On 2020/01/08 11:59, Guenter Roeck wrote:
> On 1/7/20 6:38 PM, Damien Le Moal wrote:
>> On 2020/01/08 11:34, Jens Axboe wrote:
>>> On 1/7/20 7:06 PM, Damien Le Moal wrote:
>>>> On 2020/01/08 10:25, Ming Lei wrote:
>>>>> Commit 429120f3df2d starts to take account of segment's start dma address
>>>>> when computing max segment size, and data type of 'unsigned long'
>>>>> is used to do that. However, the segment mask may be 0xffffffff, so
>>>>> the figured out segment size may be overflowed because DMA address can
>>>>> be 64bit on 32bit arch.
>>>>>
>>>>> Fixes the issue by using 'unsigned long long' to compute max segment
>>>>> size.
>>>>>
>>>>> Fixes: 429120f3df2d ("block: fix splitting segments on boundary masks")
>>>>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>>>>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>>> ---
>>>>>   block/blk-merge.c | 4 ++--
>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>>>>> index 347782a24a35..b0fcc72594cb 100644
>>>>> --- a/block/blk-merge.c
>>>>> +++ b/block/blk-merge.c
>>>>> @@ -159,12 +159,12 @@ static inline unsigned get_max_io_size(struct request_queue *q,
>>>>>   
>>>>>   static inline unsigned get_max_segment_size(const struct request_queue *q,
>>>>>   					    struct page *start_page,
>>>>> -					    unsigned long offset)
>>>>> +					    unsigned long long offset)
>>>>>   {
>>>>>   	unsigned long mask = queue_segment_boundary(q);
>>>>>   
>>>>>   	offset = mask & (page_to_phys(start_page) + offset);
>>>>
>>>> Shouldn't mask be an unsigned long long too for this to give the
>>>> expected correct result ?
>>>
>>> Don't think so, and the seg boundary is a ulong to begin with as well.
>>>
>>
>> I was referring to 32bits arch were ulong is 32bits. So we would have
>>
>> offset = 32bits & 64bits;
>>
>> with the patch applied. But I am not sure how gcc handles that and if
>> this can be a problem.
>>
> 
> Type extension is well defined in the C standard.
> 
> The underlying problem here is that mask is 0xffffffff, and
> page_to_phys(start_page) as well as offset are sometimes 0.
> In this situation, mask - offset + 1 is 0 if offset is a 32 bit
> variable, and 0x100000000 if offset is a 64 bit variable.
> In the first case, this results in a wrong maximum segment
> size of 0.

OK. Thanks for the clarification.

> 
> Guenter
> 


-- 
Damien Le Moal
Western Digital Research