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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tvrtko Ursulin Subject: Re: [PATCH v4] lib/scatterlist: Avoid potential scatterlist entry overflow Date: Fri, 13 Jan 2017 08:37:10 +0000 Message-ID: <05250677-7414-a4bd-8645-96da6a8b2df6@linux.intel.com> References: <1484125238-2539-2-git-send-email-tvrtko.ursulin@linux.intel.com> <1484135939-20988-1-git-send-email-tvrtko.ursulin@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2618C6ED5C for ; Fri, 13 Jan 2017 08:37:14 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Andy Shevchenko , Tvrtko Ursulin Cc: Masahiro Yamada , Intel-gfx@lists.freedesktop.org, "linux-kernel@vger.kernel.org" List-Id: intel-gfx@lists.freedesktop.org CkhpLAoKT24gMTEvMDEvMjAxNyAyMzo1OSwgQW5keSBTaGV2Y2hlbmtvIHdyb3RlOgo+IE9uIFdl ZCwgSmFuIDExLCAyMDE3IGF0IDE6NTggUE0sIFR2cnRrbyBVcnN1bGluIDx0dXJzdWxpbkB1cnN1 bGluLm5ldD4gd3JvdGU6Cj4+IFNpbmNlIHRoZSBzY2F0dGVybGlzdCBsZW5ndGggZmllbGQgaXMg YW4gdW5zaWduZWQgaW50LCBtYWtlCj4+IHN1cmUgdGhhdCBzZ19hbGxvY190YWJsZV9mcm9tX3Bh Z2VzIGRvZXMgbm90IG92ZXJmbG93IGl0IHdoaWxlCj4+IGNvYWxsZXNjaW5nIHBhZ2VzIHRvIGEg c2luZ2xlIGVudHJ5Lgo+Cj4KPj4gIC8qCj4+ICsgKiBTaW5jZSB0aGUgYWJvdmUgbGVuZ3RoIGZp ZWxkIGlzIGFuIHVuc2lnbmVkIGludCwgYmVsb3cgd2UgZGVmaW5lIHRoZSBtYXhpbXVtCj4+ICsg KiBsZW5naHQgaW4gYnl0ZXMgdGhhdCBjYW4gYmUgc3RvcmVkIGluIG9uZSBzY2F0dGVybGlzdCBl bnRyeS4KPgo+IGxlbmd0aAo+Cj4+ICsgKi8KPj4gKyNkZWZpbmUgU0NBVFRFUkxJU1RfTUFYX1NF R01FTlQgKDB4ZmZmZmYwMDApCj4KPiBTaG91bGRuJ3QgYmUgY2FsY3VsYXRlZCBmcm9tIFBBR0Vf U0laRSAoUEFHRSBiaXRzLCBldGMpPwoKWWVwLCBhbmQgYXQgdGhlIHNhbWUgdGltZSBJIHdvdWxk IHBvdGVudGlhbGx5IGNoYW5nZSB0aGUgbmFtZSB0byBiZSAKY29uc2lzdGVudCB3aXRoIHRoZSBv dGhlciBkZWZpbmVzIGluIHRoZSBmaWxlIGFzIEpvb25hcyBzdWdnZXN0ZWQuIApTb21ldGhpbmcg bGlrZToKCiNkZWZpbmUgU0dfTUFYX1NFR01FTlQgKFVJTlRfTUFYICYgUEFHRV9NQVNLKQoKQnV0 IHdvdWxkIG5lZWQgYSBiZXR0ZXIgbmFtZSBzaW5jZSBTR19NQVhfU0VHTUVOVCpTKiBhbHJlYWR5 IGV4aXN0cyBhbmQgCm1lYW5zIHNvbWV0aGluZyBlbHNlLiBJZiB3ZSBjYW4ndCBjb21lIHVwIHdp dGggYSBiZXR0ZXIgbmFtZSB0aGVuIGxlYXZlIAppdCBhcyBTQ0FUVEVSTElTVF9NQVhfU0VHTUVO VD8KCj4+IC0tLSBhL2xpYi9zY2F0dGVybGlzdC5jCj4+ICsrKyBiL2xpYi9zY2F0dGVybGlzdC5j Cj4KPj4gQEAgLTQwMiw5ICs0MDMsMTYgQEAgaW50IHNnX2FsbG9jX3RhYmxlX2Zyb21fcGFnZXMo c3RydWN0IHNnX3RhYmxlICpzZ3QsCj4+Cj4+ICAgICAgICAgLyogY29tcHV0ZSBudW1iZXIgb2Yg Y29udGlndW91cyBjaHVua3MgKi8KPj4gICAgICAgICBjaHVua3MgPSAxOwo+PiAtICAgICAgIGZv ciAoaSA9IDE7IGkgPCBuX3BhZ2VzOyArK2kpCj4+IC0gICAgICAgICAgICAgICBpZiAocGFnZV90 b19wZm4ocGFnZXNbaV0pICE9IHBhZ2VfdG9fcGZuKHBhZ2VzW2kgLSAxXSkgKyAxKQo+PiArICAg ICAgIHNlZ19sZW4gPSBQQUdFX1NJWkU7Cj4+ICsgICAgICAgZm9yIChpID0gMTsgaSA8IG5fcGFn ZXM7ICsraSkgewo+PiArICAgICAgICAgICAgICAgaWYgKHNlZ19sZW4gPj0gbWF4X3NlZ21lbnQg fHwKPj4gKyAgICAgICAgICAgICAgICAgICBwYWdlX3RvX3BmbihwYWdlc1tpXSkgIT0gcGFnZV90 b19wZm4ocGFnZXNbaSAtIDFdKSArIDEpIHsKPj4gICAgICAgICAgICAgICAgICAgICAgICAgKytj aHVua3M7Cj4+ICsgICAgICAgICAgICAgICAgICAgICAgIHNlZ19sZW4gPSBQQUdFX1NJWkU7Cj4+ ICsgICAgICAgICAgICAgICB9IGVsc2Ugewo+PiArICAgICAgICAgICAgICAgICAgICAgICBzZWdf bGVuICs9IFBBR0VfU0laRTsKPj4gKyAgICAgICAgICAgICAgIH0KPj4gKyAgICAgICB9Cj4KPiBX b3VsZG4ndCBiZSBmb2xsb3dpbmcgbG9va3MgbW9yZSByZWFkYWJsZT8KPgo+IHNlZ19sZW4gPSAw Owo+IC8vIEFyZSBjb21waWxlcnMgc28gc3R1cGlkIGRvaW5nIGNhbGN1bGF0aW9uIHBlciBpdGVy YXRpb24gaW4gZm9yLWNvbmRpdGlvbmFsPwo+IC8vIGZvciAoaSA9IDA7IGkgKyAxIDwgbl9wYWdl czsgaSsrKSA/CgpJIGRpZG4ndCBnZXQgd2hhdCB5b3UgbWVhbnQgaGVyZT8KCj4gZm9yIChpID0g MTsgaSA8IG5fcGFnZXM7ICsraSkgewo+ICAgc2VnX2xlbiArPSBQQUdFX1NJWkU7Cj4gICBpZiAo c2VnX2xlbiA+PSBtYXhfc2VnbWVudCB8fCBwYWdlX3RvX3BmbihwYWdlc1tpXSkgIT0KPiBwYWdl X3RvX3BmbihwYWdlc1tpIC0gMV0pICsgMSkgewo+ICAgICArK2NodW5rczsKPiAgICAgc2VnX2xl biA9IFBBR0VfU0laRTsKPiAgIH0KPiB9CgpUcmllZCBpdCBpbiBteSB1bml0IHRlc3RlciBidXQg aXQgZG9lc24ndCB3b3JrIGZvciBhbGwgc2NlbmFyaW9zLCBndWVzcyAKdGhlcmUgaXMgYSBzdWJ0 bGUgYnVnIHNvbWV3aGVyZS4gSSBkb24ndCBmaW5kIGl0IHRoYXQgdW5yZWFkYWJsZSBzbyAKd291 bGQgcHJlZmVyIHRvIGxlYXZlIGl0IHNpbmNlIGl0IHdvcmtzLgoKPiBQZXJoYXBzIHdoaWxlKCkg b3IgZG8td2hpbGUoKSB3aWxsIGluY3JlYXNlIHJlYWRhYmlsaXR5IGV2ZW4gbW9yZSwgYnV0Cj4g SSBkaWRuJ3QgY2hlY2suCj4KPj4gICAgICAgICAgICAgICAgIC8qIGxvb2sgZm9yIHRoZSBlbmQg b2YgdGhlIGN1cnJlbnQgY2h1bmsgKi8KPj4gKyAgICAgICAgICAgICAgIHNlZ19sZW4gPSBQQUdF X1NJWkU7Cj4+ICAgICAgICAgICAgICAgICBmb3IgKGogPSBjdXJfcGFnZSArIDE7IGogPCBuX3Bh Z2VzOyArK2opCj4+IC0gICAgICAgICAgICAgICAgICAgICAgIGlmIChwYWdlX3RvX3BmbihwYWdl c1tqXSkgIT0KPj4gKyAgICAgICAgICAgICAgICAgICAgICAgaWYgKHNlZ19sZW4gPj0gbWF4X3Nl Z21lbnQgfHwKPj4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgIHBhZ2VfdG9fcGZuKHBhZ2Vz W2pdKSAhPQo+PiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgcGFnZV90b19wZm4ocGFnZXNb aiAtIDFdKSArIDEpCj4+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgYnJlYWs7Cj4+ ICsgICAgICAgICAgICAgICAgICAgICAgIGVsc2UKPj4gKyAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICBzZWdfbGVuICs9IFBBR0VfU0laRTsKPgo+IFNvbWV0aGluZyBzaW1pbGFyIGhlcmUg KGRpZG4ndCB5b3UgZ2V0IHdhcm5pbmcgZnJvbSBjaGVja3BhdGggYWJvdXQKPiBjdXJseSBicmFj ZXM/KS4KCkl0IGRpZG4ndCwgYnV0IGFncmVlZCB0aGF0IEkgc2hvdWxkIGhhdmUgYWRkZWQgdGhl bS4gV2lsbCBkby4KClJlZ2FyZHMsCgpUdnJ0a28KX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlz dHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4v bGlzdGluZm8vaW50ZWwtZ2Z4Cg==