__gup_benchmark_ioctl does not handle the case where get_user_pages_fast fails:
- a negative return code will cause a buffer overrun - returning with partial success will cause use of uninitialized memory.
Cc: Kirill A. Shutemov kirill.shutemov@linux.intel.com Cc: Andrew Morton akpm@linux-foundation.org Cc: Huang Ying ying.huang@intel.com Cc: Jonathan Corbet corbet@lwn.net Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Peter Zijlstra peterz@infradead.org Cc: Thomas Gleixner tglx@linutronix.de Cc: Thorsten Leemhuis regressions@leemhuis.info Cc: stable@vger.kernel.org Signed-off-by: Michael S. Tsirkin mst@redhat.com --- mm/gup_benchmark.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c index 5c8e2ab..d743035 100644 --- a/mm/gup_benchmark.c +++ b/mm/gup_benchmark.c @@ -23,7 +23,7 @@ static int __gup_benchmark_ioctl(unsigned int cmd, struct page **pages;
nr_pages = gup->size / PAGE_SIZE; - pages = kvmalloc(sizeof(void *) * nr_pages, GFP_KERNEL); + pages = kvzalloc(sizeof(void *) * nr_pages, GFP_KERNEL); if (!pages) return -ENOMEM;
@@ -41,7 +41,8 @@ static int __gup_benchmark_ioctl(unsigned int cmd, }
nr = get_user_pages_fast(addr, nr, gup->flags & 1, pages + i); - i += nr; + if (nr > 0) + i += nr; } end_time = ktime_get();
On Thu, Apr 5, 2018 at 2:03 PM, Michael S. Tsirkin mst@redhat.com wrote:
nr = get_user_pages_fast(addr, nr, gup->flags & 1, pages + i);
i += nr;
if (nr > 0)i += nr;
Can we just make this robust while at it, and just make it
if (nr <= 0) break;
instead? Then it doesn't care about zero vs negative error, and wouldn't get stuck in an endless loop if it got zero.
Linus
On Sat, Apr 07, 2018 at 01:08:43PM -0700, Linus Torvalds wrote:
On Thu, Apr 5, 2018 at 2:03 PM, Michael S. Tsirkin mst@redhat.com wrote:
nr = get_user_pages_fast(addr, nr, gup->flags & 1, pages + i);
i += nr;
if (nr > 0)i += nr;Can we just make this robust while at it, and just make it
if (nr <= 0) break;instead? Then it doesn't care about zero vs negative error, and wouldn't get stuck in an endless loop if it got zero.
Linus
I don't mind though it alredy breaks out on the next cycle:
if (nr != gup->nr_pages_per_call) break;
the only issue is i getting corrupted when nr < 0;
On Sun, 8 Apr 2018 06:12:13 +0300 "Michael S. Tsirkin" mst@redhat.com wrote:
On Sat, Apr 07, 2018 at 01:08:43PM -0700, Linus Torvalds wrote:
On Thu, Apr 5, 2018 at 2:03 PM, Michael S. Tsirkin mst@redhat.com wrote:
nr = get_user_pages_fast(addr, nr, gup->flags & 1, pages + i);
i += nr;
if (nr > 0)i += nr;Can we just make this robust while at it, and just make it
if (nr <= 0) break;instead? Then it doesn't care about zero vs negative error, and wouldn't get stuck in an endless loop if it got zero.
LinusI don't mind though it alredy breaks out on the next cycle:
if (nr != gup->nr_pages_per_call) break;the only issue is i getting corrupted when nr < 0;
It does help readability to have the thing bail out as soon as we see something go bad. This?
--- a/mm/gup_benchmark.c~mm-gup_benchmark-handle-gup-failures-fix +++ a/mm/gup_benchmark.c @@ -41,8 +41,9 @@ static int __gup_benchmark_ioctl(unsigne }
nr = get_user_pages_fast(addr, nr, gup->flags & 1, pages + i); - if (nr > 0) - i += nr; + if (nr <= 0) + break; + i += nr; } end_time = ktime_get();
_
linux-stable-mirror@lists.linaro.org
-
Andrew Morton -
Linus Torvalds -
Michael S. Tsirkin