lkml.org 
[lkml]   [2017]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [Intel-gfx] [PATCH v4] lib/scatterlist: Avoid potential scatterlist entry overflow
From
Date

Hi,

On 11/01/2017 23:59, Andy Shevchenko wrote:
> On Wed, Jan 11, 2017 at 1:58 PM, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>> Since the scatterlist length field is an unsigned int, make
>> sure that sg_alloc_table_from_pages does not overflow it while
>> coallescing pages to a single entry.
>
>
>> /*
>> + * Since the above length field is an unsigned int, below we define the maximum
>> + * lenght in bytes that can be stored in one scatterlist entry.
>
> length
>
>> + */
>> +#define SCATTERLIST_MAX_SEGMENT (0xfffff000)
>
> Shouldn't be calculated from PAGE_SIZE (PAGE bits, etc)?

Yep, and at the same time I would potentially change the name to be
consistent with the other defines in the file as Joonas suggested.
Something like:

#define SG_MAX_SEGMENT (UINT_MAX & PAGE_MASK)

But would need a better name since SG_MAX_SEGMENT*S* already exists and
means something else. If we can't come up with a better name then leave
it as SCATTERLIST_MAX_SEGMENT?

>> --- a/lib/scatterlist.c
>> +++ b/lib/scatterlist.c
>
>> @@ -402,9 +403,16 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,
>>
>> /* compute number of contiguous chunks */
>> chunks = 1;
>> - for (i = 1; i < n_pages; ++i)
>> - if (page_to_pfn(pages[i]) != page_to_pfn(pages[i - 1]) + 1)
>> + seg_len = PAGE_SIZE;
>> + for (i = 1; i < n_pages; ++i) {
>> + if (seg_len >= max_segment ||
>> + page_to_pfn(pages[i]) != page_to_pfn(pages[i - 1]) + 1) {
>> ++chunks;
>> + seg_len = PAGE_SIZE;
>> + } else {
>> + seg_len += PAGE_SIZE;
>> + }
>> + }
>
> Wouldn't be following looks more readable?
>
> seg_len = 0;
> // Are compilers so stupid doing calculation per iteration in for-conditional?
> // for (i = 0; i + 1 < n_pages; i++) ?

I didn't get what you meant here?

> for (i = 1; i < n_pages; ++i) {
> seg_len += PAGE_SIZE;
> if (seg_len >= max_segment || page_to_pfn(pages[i]) !=
> page_to_pfn(pages[i - 1]) + 1) {
> ++chunks;
> seg_len = PAGE_SIZE;
> }
> }

Tried it in my unit tester but it doesn't work for all scenarios, guess
there is a subtle bug somewhere. I don't find it that unreadable so
would prefer to leave it since it works.

> Perhaps while() or do-while() will increase readability even more, but
> I didn't check.
>
>> /* look for the end of the current chunk */
>> + seg_len = PAGE_SIZE;
>> for (j = cur_page + 1; j < n_pages; ++j)
>> - if (page_to_pfn(pages[j]) !=
>> + if (seg_len >= max_segment ||
>> + page_to_pfn(pages[j]) !=
>> page_to_pfn(pages[j - 1]) + 1)
>> break;
>> + else
>> + seg_len += PAGE_SIZE;
>
> Something similar here (didn't you get warning from checkpath about
> curly braces?).

It didn't, but agreed that I should have added them. Will do.

Regards,

Tvrtko

\
 
 \ /
  Last update: 2017-01-13 09:38    [W:0.126 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site