block: fix get_max_segment_size() overflow on 32bit arch
diff mbox series

Message ID 20200108012526.26731-1-ming.lei@redhat.com
State New
Headers show
Series
  • block: fix get_max_segment_size() overflow on 32bit arch
Related show

Commit Message

Ming Lei Jan. 8, 2020, 1:25 a.m. UTC
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(-)

Comments

Damien Le Moal Jan. 8, 2020, 2:06 a.m. UTC | #1
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));
>  }
>  
>
Jens Axboe Jan. 8, 2020, 2:34 a.m. UTC | #2
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.
Damien Le Moal Jan. 8, 2020, 2:38 a.m. UTC | #3
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.
Ming Lei Jan. 8, 2020, 2:38 a.m. UTC | #4
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
Ming Lei Jan. 8, 2020, 2:47 a.m. UTC | #5
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
Guenter Roeck Jan. 8, 2020, 2:59 a.m. UTC | #6
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
Damien Le Moal Jan. 8, 2020, 3:02 a.m. UTC | #7
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
>

Patch
diff mbox series

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));
 }