Skip to contentSkip to conversation

mm/get_user_pages_fast fixes, cleanups

PATCH In review Michael S. Tsirkin 3 participants 9 messages Apr 5, 2018 – Apr 10, 2018

Subsystems (9)
Stable branch Memory management MIPS MM core +5 more subsystems
lore
Share & tools
lore

Michael S. Tsirkin

author
#0 lore raw

v2 cover mm/get_user_pages_fast fixes, cleanups

Turns out get_user_pages_fast and __get_user_pages_fast return different values on error when given a single page: __get_user_pages_fast returns
0. get_user_pages_fast returns either 0 or an error.

Callers of get_user_pages_fast expect an error so fix it up to return an error consistently.

Stress the difference between get_user_pages_fast and __get_user_pages_fast to make sure callers aren't confused.

Changes from v1:
limit code changes to get_user_pages_fast. document __get_user_pages_fast
fix a bug in caller

Michael S. Tsirkin (3):
mm/gup_benchmark: handle gup failures
gup: return -EFAULT on access_ok failure
mm/gup: document return value

 arch/mips/mm/gup.c  | 2 ++
 arch/s390/mm/gup.c  | 2 ++
 arch/sh/mm/gup.c    | 2 ++
 arch/sparc/mm/gup.c | 4 ++++
 mm/gup.c            | 9 +++++++--
 mm/gup_benchmark.c  | 5 +++--
 mm/util.c           | 6 ++++--
 7 files changed, 24 insertions(+), 6 deletions(-)
Signature · 1 line
MST
author
#1 lore raw

PATCH v2 1/3 mm/gup_benchmark: handle gup failures

__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 <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Huang Ying <[email protected]>
Cc Jonathan Corbet <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Thorsten Leemhuis <[email protected]>
Cc [email protected]
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 mm/gup_benchmark.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
mm/gup_benchmark.c +3 -2
index 5c8e2ab..d743035 100644
@@ -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();
Signature · 1 line
MST
#2 lore raw

Re: [PATCH v2 1/3] mm/gup_benchmark: handle gup failures

mm/gup_benchmark.c static int __gup_benchmark_ioctl(unsigned int cmd, · line 45

Quoted 5 lines · Michael S. Tsirkin
                nr = get_user_pages_fast(addr, nr, gup->flags & 1, pages + i);
mm/gup_benchmark.c +2 -1
-               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

author
#3 lore raw

Re: [PATCH v2 1/3] mm/gup_benchmark: handle gup failures

Quoted 9 lines · Linus Torvalds

mm/gup_benchmark.c static int __gup_benchmark_ioctl(unsigned int cmd, · line 45

Quoted 5 lines · Michael S. Tsirkin
                nr = get_user_pages_fast(addr, nr, gup->flags & 1, pages + i);
mm/gup_benchmark.c +2 -1
-               i += nr;
+               if (nr > 0)
+                       i += nr;

Can we just make this robust while at it, and just make it

Quoted 7 lines
        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;

maintainer
#4 lore raw

Re: [PATCH v2 1/3] mm/gup_benchmark: handle gup failures

Quoted 16 lines · On Sun, 8 Apr 2018 06:12:13 +0300 "Michael S. Tsirkin" <[email protected]> wrote:
Quoted 15 lines · Linus Torvalds

Review

Quoted 5 lines · Michael S. Tsirkin
                nr = get_user_pages_fast(addr, nr, gup->flags & 1, pages + i);
(quoted diff) +2 -1
-               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.

Quoted 9 lines
Quoted 1 line

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;

It does help readability to have the thing bail out as soon as we see something go bad. This?

mm/gup_benchmark.c renamed from mm/gup_benchmark.c~mm-gup_benchmark-handle-gup-failures-fix +3 -2
@@ -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();

_

author
#5 lore raw

PATCH v2 2/3 gup: return -EFAULT on access_ok failure

In reply to Michael S. Tsirkin

get_user_pages_fast is supposed to be a faster drop-in equivalent of get_user_pages. As such, callers expect it to return a negative return code when passed an invalid address, and never expect it to return 0 when passed a positive number of pages, since
its documentation says:

* Returns number of pages pinned. This may be fewer than the number
* requested. If nr_pages is 0 or negative, returns 0. If no pages
* were pinned, returns -errno.

When get_user_pages_fast fall back on get_user_pages this is exactly what happens. Unfortunately the implementation is inconsistent: it returns 0 if passed a kernel address, confusing callers: for example, the following is pretty common but does not appear to do the right thing with a kernel address:

        ret = get_user_pages_fast(addr, 1, writeable, &page);
        if (ret < 0)
                return ret;

Change get_user_pages_fast to return -EFAULT when supplied a kernel address to make it match expectations.

All caller have been audited for consistency with the documented semantics.

Lightly tested.

Cc Kirill A. Shutemov <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Huang Ying <[email protected]>
Cc Jonathan Corbet <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Thorsten Leemhuis <[email protected]>
Cc [email protected]
Fixes 5b65c4677a57 ("mm, x86/mm: Fix performance regression in get_user_pages_fast()")
Reported-by [email protected]
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 mm/gup.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
mm/gup.c +4 -1
index 6afae32..8f3a064 100644
@@ -1806,9 +1806,12 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write,
 	len = (unsigned long) nr_pages << PAGE_SHIFT;
 	end = start + len;
 
+	if (nr_pages <= 0)
+		return 0;
+
 	if (unlikely(!access_ok(write ? VERIFY_WRITE : VERIFY_READ,
 					(void __user *)start, len)))
-		return 0;
+		return -EFAULT;
 
 	if (gup_fast_permitted(start, nr_pages, write)) {
 		local_irq_disable();
Signature · 1 line
MST
author
#6 lore raw

PATCH v2 3/3 mm/gup: document return value

In reply to Michael S. Tsirkin

__get_user_pages_fast handles errors differently from
get_user_pages_fast: the former always returns the number of pages pinned, the later might return a negative error code.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 arch/mips/mm/gup.c  | 2 ++
 arch/s390/mm/gup.c  | 2 ++
 arch/sh/mm/gup.c    | 2 ++
 arch/sparc/mm/gup.c | 4 ++++
 mm/gup.c            | 4 +++-
 mm/util.c           | 6 ++++--
 6 files changed, 17 insertions(+), 3 deletions(-)
arch/mips/mm/gup.c +2 -0
index 1e4658e..5a4875ca 100644
@@ -178,6 +178,8 @@ static int gup_pud_range(pgd_t pgd, unsigned long addr, unsigned long end,
 /*
  * Like get_user_pages_fast() except its IRQ-safe in that it won't fall
  * back to the regular GUP.
+ * Note a difference with get_user_pages_fast: this always returns the
+ * number of pages pinned, 0 if no pages were pinned.
  */
 int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 			  struct page **pages)
arch/s390/mm/gup.c +2 -0
index 05c8abd..2809d11 100644
@@ -220,6 +220,8 @@ static inline int gup_p4d_range(pgd_t *pgdp, pgd_t pgd, unsigned long addr,
 /*
  * Like get_user_pages_fast() except its IRQ-safe in that it won't fall
  * back to the regular GUP.
+ * Note a difference with get_user_pages_fast: this always returns the
+ * number of pages pinned, 0 if no pages were pinned.
  */
 int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 			  struct page **pages)
arch/sh/mm/gup.c +2 -0
index 8045b5b..56c86ca 100644
@@ -160,6 +160,8 @@ static int gup_pud_range(pgd_t pgd, unsigned long addr, unsigned long end,
 /*
  * Like get_user_pages_fast() except its IRQ-safe in that it won't fall
  * back to the regular GUP.
+ * Note a difference with get_user_pages_fast: this always returns the
+ * number of pages pinned, 0 if no pages were pinned.
  */
 int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 			  struct page **pages)
arch/sparc/mm/gup.c +4 -0
index 5335ba3..ca3eb69 100644
@@ -192,6 +192,10 @@ static int gup_pud_range(pgd_t pgd, unsigned long addr, unsigned long end,
 	return 1;
 }
 
+/*
+ * Note a difference with get_user_pages_fast: this always returns the
+ * number of pages pinned, 0 if no pages were pinned.
+ */
 int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 			  struct page **pages)
 {
mm/gup.c +3 -1
index 8f3a064..5cb5bb1 100644
@@ -1740,7 +1740,9 @@ bool gup_fast_permitted(unsigned long start, int nr_pages, int write)
 
 /*
  * Like get_user_pages_fast() except it's IRQ-safe in that it won't fall back to
- * the regular GUP. It will only return non-negative values.
+ * the regular GUP.
+ * Note a difference with get_user_pages_fast: this always returns the
+ * number of pages pinned, 0 if no pages were pinned.
  */
 int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 			  struct page **pages)
mm/util.c +4 -2
index c125050..db2f005 100644
@@ -297,8 +297,10 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
 /*
  * Like get_user_pages_fast() except its IRQ-safe in that it won't fall
  * back to the regular GUP.
- * If the architecture not support this function, simply return with no
- * page pinned
+ * Note a difference with get_user_pages_fast: this always returns the
+ * number of pages pinned, 0 if no pages were pinned.
+ * If the architecture does not support this function, simply return with no
+ * pages pinned.
  */
 int __weak __get_user_pages_fast(unsigned long start,
 				 int nr_pages, int write, struct page **pages)
Signature · 1 line
MST
maintainer
#7 lore raw
In reply to Michael S. Tsirkin
Quoted 10 lines · On Fri, 6 Apr 2018 00:03:48 +0300 "Michael S. Tsirkin" <[email protected]> wrote:

Turns out get_user_pages_fast and __get_user_pages_fast return different values on error when given a single page: __get_user_pages_fast returns
0. get_user_pages_fast returns either 0 or an error.

Callers of get_user_pages_fast expect an error so fix it up to return an error consistently.

Stress the difference between get_user_pages_fast and __get_user_pages_fast to make sure callers aren't confused.

A term which is missing from all these changelogs is "vhost" :(

This patchset fixes a user-affecting bug, does it not? If so, please fully describe that bug so that we can decide which kernel version(s) need the patchset.

And yes, this return value asymmetry is sad. Did you scope out what would be needed to fix up the callers so we can avoid this?

author
#8 lore raw
Quoted 6 lines · Andrew Morton
Quoted 4 lines · On Fri, 6 Apr 2018 00:03:48 +0300 "Michael S. Tsirkin" <[email protected]> wrote:

Turns out get_user_pages_fast and __get_user_pages_fast return different values on error when given a single page: __get_user_pages_fast returns
0. get_user_pages_fast returns either 0 or an error.

Quoted 8 lines
Quoted 6 lines

Callers of get_user_pages_fast expect an error so fix it up to return an error consistently.

Stress the difference between get_user_pages_fast and __get_user_pages_fast to make sure callers aren't confused.

A term which is missing from all these changelogs is "vhost" :(

vhost has a BUG_ON for unexpected handling so it catches the bug, but it's not the only site affected.

Quoted 3 lines

This patchset fixes a user-affecting bug, does it not? If so, please fully describe that bug so that we can decide which kernel version(s) need the patchset.

OK, I'll try to write up something.

Quoted 2 lines

And yes, this return value asymmetry is sad. Did you scope out what would be needed to fix up the callers so we can avoid this?

Yes - there is a very small number of callers for __get_user_pages_fast so it's easy to teach them all to treat any value <=0 as 0. There seems to be some opposition to changing the API,
I'd like to look into this after the bugfix patchset is merged.

Signature · 1 line
MST