| Message ID | 20200108012526.26731-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: fix get_max_segment_size() overflow on 32bit arch | expand |
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)); > } > >
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.
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.
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
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
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
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 >
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)); }
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(-)