From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751241AbdAMIhP (ORCPT ); Fri, 13 Jan 2017 03:37:15 -0500 Received: from mga14.intel.com ([192.55.52.115]:39295 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751146AbdAMIhO (ORCPT ); Fri, 13 Jan 2017 03:37:14 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,220,1477983600"; d="scan'208";a="53384528" Subject: Re: [Intel-gfx] [PATCH v4] lib/scatterlist: Avoid potential scatterlist entry overflow To: Andy Shevchenko , Tvrtko Ursulin References: <1484125238-2539-2-git-send-email-tvrtko.ursulin@linux.intel.com> <1484135939-20988-1-git-send-email-tvrtko.ursulin@linux.intel.com> Cc: Masahiro Yamada , Intel-gfx@lists.freedesktop.org, "linux-kernel@vger.kernel.org" , Joonas Lahtinen From: Tvrtko Ursulin Message-ID: <05250677-7414-a4bd-8645-96da6a8b2df6@linux.intel.com> Date: Fri, 13 Jan 2017 08:37:10 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 11/01/2017 23:59, Andy Shevchenko wrote: > On Wed, Jan 11, 2017 at 1:58 PM, Tvrtko Ursulin 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