Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751779AbdG1KyQ convert rfc822-to-8bit (ORCPT ); Fri, 28 Jul 2017 06:54:16 -0400 Received: from mail.fireflyinternet.com ([109.228.58.192]:65383 "EHLO fireflyinternet.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751700AbdG1KyP (ORCPT ); Fri, 28 Jul 2017 06:54:15 -0400 X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138; Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT To: Tvrtko Ursulin , Intel-gfx@lists.freedesktop.org From: Chris Wilson In-Reply-To: <20170727090504.15812-3-tvrtko.ursulin@linux.intel.com> Cc: linux-kernel@vger.kernel.org, "Masahiro Yamada" , "Andy Shevchenko" , "Ben Widawsky" References: <20170727090504.15812-1-tvrtko.ursulin@linux.intel.com> <20170727090504.15812-3-tvrtko.ursulin@linux.intel.com> Message-ID: <150123923145.15632.18091147973239693301@mail.alporthouse.com> User-Agent: alot/0.3.6 Subject: Re: [Intel-gfx] [PATCH 2/4] lib/scatterlist: Avoid potential scatterlist entry overflow Date: Fri, 28 Jul 2017 11:53:51 +0100 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4418 Lines: 112 Quoting Tvrtko Ursulin (2017-07-27 10:05:02) > From: Tvrtko Ursulin > > 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. > > v2: Drop reference to future use. Use UINT_MAX. > v3: max_segment must be page aligned. > v4: Do not rely on compiler to optimise out the rounddown. > (Joonas Lahtinen) > v5: Simplified loops and use post-increments rather than > pre-increments. Use PAGE_MASK and fix comment typo. > (Andy Shevchenko) > > Signed-off-by: Tvrtko Ursulin > Cc: Masahiro Yamada > Cc: linux-kernel@vger.kernel.org > Reviewed-by: Chris Wilson (v2) > Cc: Joonas Lahtinen > Cc: Andy Shevchenko > --- > include/linux/scatterlist.h | 6 ++++++ > lib/scatterlist.c | 31 ++++++++++++++++++++----------- > 2 files changed, 26 insertions(+), 11 deletions(-) > > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h > index 205aefb4ed93..6dd2ddbc6230 100644 > --- a/include/linux/scatterlist.h > +++ b/include/linux/scatterlist.h > @@ -21,6 +21,12 @@ struct scatterlist { > }; > > /* > + * Since the above length field is an unsigned int, below we define the maximum > + * length in bytes that can be stored in one scatterlist entry. > + */ > +#define SCATTERLIST_MAX_SEGMENT (UINT_MAX & PAGE_MASK) > + > +/* > * These macros should be used after a dma_map_sg call has been done > * to get bus addresses of each of the SG entries and their lengths. > * You should only work with the number of sg entries dma_map_sg > diff --git a/lib/scatterlist.c b/lib/scatterlist.c > index dee0c5004e2f..7b2e74da2c44 100644 > --- a/lib/scatterlist.c > +++ b/lib/scatterlist.c > @@ -394,17 +394,22 @@ int sg_alloc_table_from_pages(struct sg_table *sgt, > unsigned int offset, unsigned long size, > gfp_t gfp_mask) > { > - unsigned int chunks; > - unsigned int i; > - unsigned int cur_page; > + const unsigned int max_segment = SCATTERLIST_MAX_SEGMENT; > + unsigned int chunks, cur_page, seg_len, i; > int ret; > struct scatterlist *s; > > /* 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) > - ++chunks; > + seg_len = 0; > + 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 = 0; > + } > + } Ok. Took a moment to realise that it works correctly for a chunk on last page. > ret = sg_alloc_table(sgt, chunks, gfp_mask); > if (unlikely(ret)) > @@ -413,17 +418,21 @@ int sg_alloc_table_from_pages(struct sg_table *sgt, > /* merging chunks and putting them into the scatterlist */ > cur_page = 0; > for_each_sg(sgt->sgl, s, sgt->orig_nents, i) { > - unsigned long chunk_size; > - unsigned int j; > + unsigned int j, chunk_size; > > /* look for the end of the current chunk */ > - for (j = cur_page + 1; j < n_pages; ++j) > - if (page_to_pfn(pages[j]) != > + seg_len = 0; > + for (j = cur_page + 1; j < n_pages; j++) { > + seg_len += PAGE_SIZE; > + if (seg_len >= max_segment || > + page_to_pfn(pages[j]) != > page_to_pfn(pages[j - 1]) + 1) > break; > + } Ok. > > chunk_size = ((j - cur_page) << PAGE_SHIFT) - offset; > - sg_set_page(s, pages[cur_page], min(size, chunk_size), offset); > + sg_set_page(s, pages[cur_page], > + min_t(unsigned long, size, chunk_size), offset); > size -= chunk_size; > offset = 0; > cur_page = j; Reviewed-by: Chris Wilson -Chris