Re: general protection fault in get_futex_key

[Peter Zijlstra]

On Wed, Apr 08, 2020 at 03:36:45PM +0200, Thomas Gleixner wrote:

> This means that
> 
>      get_user_pages_fast(address, 1, FOLL_WRITE, &page)
> 
> returned 0, which is breaking the interface:
> 
>  * 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.
> 
> nr_pages is clearly 1. So if the call fails, the number of pinned pages
> is 0 and it should return a proper error number. It did before.
> 
> From a quick look at the commit in question I assume it's the
> 
> +               if (fatal_signal_pending(current))
> +                       break;
> 
> which can cause that to happen.

Yep, that's broken. It wants the below, just like all the other error
paths in that loop.

---
Subject: mm/gup: Fix broken fatal_signal_pending() test

Just like all the other error returns in this loop, we need to set
pages_done to the error value when it is zero.

Fixes: 4426e945df58 ("mm/gup: allow VM_FAULT_RETRY for multiple times")
Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
---
diff --git a/mm/gup.c b/mm/gup.c
index afce0bc47e70..14bce270b249 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1326,8 +1326,11 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk,
 		 * start trying again otherwise it can loop forever.
 		 */
 
-		if (fatal_signal_pending(current))
+		if (fatal_signal_pending(current)) {
+			if (!pages_done)
+				pages_done = -ERESTARTSYS;
 			break;
+		}
 
 		ret = down_read_killable(&mm->mmap_sem);
 		if (ret) {