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

[PATCH V2] 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 in case of zero physical
address on 32bit arch.

Fix the issue by returning queue_max_segment_size() directly when that
happens.

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>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
V2:
	- don't use 64bit offset, so that not adding overhead on 32bit
	physical address space

 block/blk-merge.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 347782a24a35..1534ed736363 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -164,8 +164,13 @@ static inline unsigned get_max_segment_size(const struct request_queue *q,
 	unsigned long mask = queue_segment_boundary(q);
 
 	offset = mask & (page_to_phys(start_page) + offset);
-	return min_t(unsigned long, mask - offset + 1,
-		     queue_max_segment_size(q));
+
+	/*
+	 * overflow may be triggered in case of zero page physical address
+	 * on 32bit arch, use queue's max segment size when that happens.
+	 */
+	return min_not_zero(mask - offset + 1,
+			(unsigned long)queue_max_segment_size(q));
 }
 
 /**
-- 
2.20.1

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

[Jens Axboe]

On 1/11/20 5:57 AM, 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 in case of zero physical
> address on 32bit arch.
> 
> Fix the issue by returning queue_max_segment_size() directly when that
> happens.

I still think this should use phys_addr_t, just in case the mask is
ever not 32-bit. The current types are a bit weird, tbh.

-- 
Jens Axboe

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

[Ming Lei]

On Sat, Jan 11, 2020 at 08:51:31AM -0700, Jens Axboe wrote:
> On 1/11/20 5:57 AM, 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 in case of zero physical
> > address on 32bit arch.
> > 
> > Fix the issue by returning queue_max_segment_size() directly when that
> > happens.
> 
> I still think this should use phys_addr_t, just in case the mask is
> ever not 32-bit. The current types are a bit weird, tbh.

I didn't use phys_addr_t because queue_segment_boundary() always
returns 'unsigned long', so using 'phys_addr_t' doesn't make any
difference because the following result can be held in 32bit always
no matter offset is 32bit or 64bit:

	mask & (page_to_phys(start_page) + offset)

BTW, 'seg_boundary_mask' is defined as 'unsigned long' since kernel
git tree was born. Given not see related report with 64bit phys_addr_t
on 32bit arch, I guess we may leave it alone.

However, if you think we need to convert 'seg_boundary_mask' into
phys_addr_t, the 'offset' parameter can be re-defined as phys_addr_t.


thanks,
Ming