[PATCH 0/3] userfaultfd non-cooperative further update for 4.11 merge window

[PATCH 0/3] userfaultfd non-cooperative further update for 4.11 merge window

[Andrea Arcangeli]

Hello,

unfortunately I noticed one relevant bug in userfaultfd_exit while
doing more testing. I've been doing testing before and this was also
tested by kbuild bot and exercised by the selftest, but this bug never
reproduced before.

I dropped userfaultfd_exit as result. I dropped it because of
implementation difficulty in receiving signals in __mmput and because
I think -ENOSPC as result from the background UFFDIO_COPY should be
enough already.

Before I decided to remove userfaultfd_exit, I noticed
userfaultfd_exit wasn't exercised by the selftest and when I tried to
exercise it, after moving it to a more correct place in __mmput where
it would make more sense and where the vma list is stable, it resulted
in the event_wait_completion in D state. So then I added the second
patch to be sure even if we call userfaultfd_event_wait_completion too
late during task exit(), we won't risk to generate tasks in D
state. The same check exists in handle_userfault() for the same
reason, except it makes a difference there, while here is just a
robustness check and it's run under WARN_ON_ONCE.

While looking at the userfaultfd_event_wait_completion() function I
looked back at its callers too while at it and I think it's not ok to
stop executing dup_fctx on the fcs list because we relay on
userfaultfd_event_wait_completion to execute
userfaultfd_ctx_put(fctx->orig) which is paired against
userfaultfd_ctx_get(fctx->orig) in dup_userfault just before
list_add(fcs). This change only takes care of fctx->orig but this area
also needs further review looking for similar problems in fctx->new.

The only patch that is urgent is the first because it's an use after
free during a SMP race condition that affects all processes if
CONFIG_USERFAULTFD=y. Very hard to reproduce though and probably
impossible without SLUB poisoning enabled.

Mike and Pavel please review, thanks!
Andrea

Andrea Arcangeli (3):
  userfaultfd: non-cooperative: rollback userfaultfd_exit
  userfaultfd: non-cooperative: robustness check
  userfaultfd: non-cooperative: release all ctx in
    dup_userfaultfd_complete

 fs/userfaultfd.c                 | 47 +++++++---------------------------------
 include/linux/userfaultfd_k.h    |  6 -----
 include/uapi/linux/userfaultfd.h |  5 +----
 kernel/exit.c                    |  1 -
 4 files changed, 9 insertions(+), 50 deletions(-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/3] userfaultfd: non-cooperative: rollback userfaultfd_exit

[Andrea Arcangeli]

I once reproduced this oops with the userfaultfd selftest, it's not
easily reproducible and it requires SLUB poisoning to reproduce.

general protection fault: 0000 [#1] SMP
Modules linked in:
CPU: 2 PID: 18421 Comm: userfaultfd Tainted: G               ------------ T 3.10.0+ #15
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.1-0-g8891697-prebuilt.qemu-project.org 04/01/2014
task: ffff8801f83b9440 ti: ffff8801f833c000 task.ti: ffff8801f833c000
RIP: 0010:[<ffffffff81451299>]  [<ffffffff81451299>] userfaultfd_exit+0x29/0xa0
RSP: 0018:ffff8801f833fe80  EFLAGS: 00010202
RAX: ffff8801f833ffd8 RBX: 6b6b6b6b6b6b6b6b RCX: ffff8801f83b9440
RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8800baf18600
RBP: ffff8801f833fee8 R08: 0000000000000000 R09: 0000000000000001
R10: 0000000000000000 R11: ffffffff8127ceb3 R12: 0000000000000000
R13: ffff8800baf186b0 R14: ffff8801f83b99f8 R15: 00007faed746c700
FS:  0000000000000000(0000) GS:ffff88023fc80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00007faf0966f028 CR3: 0000000001bc6000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Stack:
 ffffffff8127cec6 0000000000000001 0000000000000286 ffff8801f833fed0
 0000000000000286 ffff8801f83b99f8 ffff8800baf18600 ffff8800baf186b0
 ffff8801f83b99f8 ffff8801f83b99f8 ffff8801f833fee8 ffff8801f83b9440
Call Trace:
 [<ffffffff8127cec6>] ? do_exit+0x276/0xd10
 [<ffffffff8127cee7>] do_exit+0x297/0xd10
 [<ffffffff8137d113>] ? context_tracking_user_exit+0x13/0x20
 [<ffffffff8121c5ed>] ? syscall_trace_enter+0x13d/0x300
 [<ffffffff8127eda7>] SyS_exit+0x17/0x20
 [<ffffffff8183edeb>] tracesys+0xdd/0xe2
Code: 00 00 66 66 66 66 90 55 48 89 e5 41 54 53 48 83 ec 58 48 8b 1f 48 85 db 75 11 eb 73 66 0f 1f 44 00 00 48 8b 5b 10 48 85 db 74 64 <4c> 8b a3 b8 00 00 00 4d 85 e4 74 eb 41 f6 84 24 2c 01 00 00 80
RIP  [<ffffffff81451299>] userfaultfd_exit+0x29/0xa0
 RSP <ffff8801f833fe80>
---[ end trace 9fecd6dcb442846a ]---

In the debugger I located the "mm" pointer in the stack and walking
mm->mmap->vm_next through the end shows the vma->vm_next list is fully
consistent and it is null terminated list as expected. So this has to
be an SMP race condition where userfaultfd_exit was running while the
vma list was being modified by another CPU.

When userfaultfd_exit() run one of the ->vm_next pointers pointed to
SLAB_POISON (RBX is the vma pointer and is 0x6b6b..).

The reason is that it's not running in __mmput but while there are
still other threads running and it's not holding the mmap_sem (it
can't as it has to wait the even to be received by the manager). So
this is an use after free that was happening for all processes.

One more implementation problem aside from the race condition:
userfaultfd_exit has really to check a flag in mm->flags before
walking the vma or it's going to slowdown the exit() path for regular
tasks.

One more implementation problem: at that point signals can't be
delivered so it would also create a task in D state if the manager
doesn't read the event.

The major design issue: it overall looks superfluous as the manager
can check for -ENOSPC in the background transfer:

	if (mmget_not_zero(ctx->mm)) {
[..]
	} else {
		return -ENOSPC;
	}

It's safer to roll it back and re-introduce it later if at all.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 fs/userfaultfd.c                 | 28 ----------------------------
 include/linux/userfaultfd_k.h    |  6 ------
 include/uapi/linux/userfaultfd.h |  5 +----
 kernel/exit.c                    |  1 -
 4 files changed, 1 insertion(+), 39 deletions(-)

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index e6e0a61..52733a7 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -774,34 +774,6 @@ void userfaultfd_unmap_complete(struct mm_struct *mm, struct list_head *uf)
 	}
 }
 
-void userfaultfd_exit(struct mm_struct *mm)
-{
-	struct vm_area_struct *vma = mm->mmap;
-
-	/*
-	 * We can do the vma walk without locking because the caller
-	 * (exit_mm) knows it now has exclusive access
-	 */
-	while (vma) {
-		struct userfaultfd_ctx *ctx = vma->vm_userfaultfd_ctx.ctx;
-
-		if (ctx && (ctx->features & UFFD_FEATURE_EVENT_EXIT)) {
-			struct userfaultfd_wait_queue ewq;
-
-			userfaultfd_ctx_get(ctx);
-
-			msg_init(&ewq.msg);
-			ewq.msg.event = UFFD_EVENT_EXIT;
-
-			userfaultfd_event_wait_completion(ctx, &ewq);
-
-			ctx->features &= ~UFFD_FEATURE_EVENT_EXIT;
-		}
-
-		vma = vma->vm_next;
-	}
-}
-
 static int userfaultfd_release(struct inode *inode, struct file *file)
 {
 	struct userfaultfd_ctx *ctx = file->private_data;
diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
index 0468548..f2b79bf 100644
--- a/include/linux/userfaultfd_k.h
+++ b/include/linux/userfaultfd_k.h
@@ -72,8 +72,6 @@ extern int userfaultfd_unmap_prep(struct vm_area_struct *vma,
 extern void userfaultfd_unmap_complete(struct mm_struct *mm,
 				       struct list_head *uf);
 
-extern void userfaultfd_exit(struct mm_struct *mm);
-
 #else /* CONFIG_USERFAULTFD */
 
 /* mm helpers */
@@ -139,10 +137,6 @@ static inline void userfaultfd_unmap_complete(struct mm_struct *mm,
 {
 }
 
-static inline void userfaultfd_exit(struct mm_struct *mm)
-{
-}
-
 #endif /* CONFIG_USERFAULTFD */
 
 #endif /* _LINUX_USERFAULTFD_K_H */
diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h
index c055947..3b05953 100644
--- a/include/uapi/linux/userfaultfd.h
+++ b/include/uapi/linux/userfaultfd.h
@@ -18,8 +18,7 @@
  * means the userland is reading).
  */
 #define UFFD_API ((__u64)0xAA)
-#define UFFD_API_FEATURES (UFFD_FEATURE_EVENT_EXIT |		\
-			   UFFD_FEATURE_EVENT_FORK |		\
+#define UFFD_API_FEATURES (UFFD_FEATURE_EVENT_FORK |		\
 			   UFFD_FEATURE_EVENT_REMAP |		\
 			   UFFD_FEATURE_EVENT_REMOVE |	\
 			   UFFD_FEATURE_EVENT_UNMAP |		\
@@ -113,7 +112,6 @@ struct uffd_msg {
 #define UFFD_EVENT_REMAP	0x14
 #define UFFD_EVENT_REMOVE	0x15
 #define UFFD_EVENT_UNMAP	0x16
-#define UFFD_EVENT_EXIT		0x17
 
 /* flags for UFFD_EVENT_PAGEFAULT */
 #define UFFD_PAGEFAULT_FLAG_WRITE	(1<<0)	/* If this was a write fault */
@@ -163,7 +161,6 @@ struct uffdio_api {
 #define UFFD_FEATURE_MISSING_HUGETLBFS		(1<<4)
 #define UFFD_FEATURE_MISSING_SHMEM		(1<<5)
 #define UFFD_FEATURE_EVENT_UNMAP		(1<<6)
-#define UFFD_FEATURE_EVENT_EXIT			(1<<7)
 	__u64 features;
 
 	__u64 ioctls;
diff --git a/kernel/exit.c b/kernel/exit.c
index d038f98..a2e2fba 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -519,7 +519,6 @@ static void exit_mm(struct task_struct *tsk)
 	enter_lazy_tlb(mm, current);
 	task_unlock(tsk);
 	mm_update_next_owner(mm);
-	userfaultfd_exit(mm);
 	mmput(mm);
 	if (test_thread_flag(TIF_MEMDIE))
 		exit_oom_victim();

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/3] userfaultfd: non-cooperative: robustness check

[Andrea Arcangeli]

Similar to the handle_userfault() case, also make sure to never
attempt to send any event past the PF_EXITING point of no return.

This is purely a robustness check.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 fs/userfaultfd.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 52733a7..3d7c248 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -529,8 +529,13 @@ int handle_userfault(struct vm_fault *vmf, unsigned long reason)
 static int userfaultfd_event_wait_completion(struct userfaultfd_ctx *ctx,
 					     struct userfaultfd_wait_queue *ewq)
 {
-	int ret = 0;
+	int ret;
+
+	ret = -1;
+	if (WARN_ON_ONCE(current->flags & PF_EXITING))
+		goto out;
 
+	ret = 0;
 	ewq->ctx = ctx;
 	init_waitqueue_entry(&ewq->wq, current);
 
@@ -565,7 +570,7 @@ static int userfaultfd_event_wait_completion(struct userfaultfd_ctx *ctx,
 	 * ctx may go away after this if the userfault pseudo fd is
 	 * already released.
 	 */
-
+out:
 	userfaultfd_ctx_put(ctx);
 	return ret;
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/3] userfaultfd: non-cooperative: release all ctx in dup_userfaultfd_complete

[Andrea Arcangeli]

Don't stop running dup_fctx() even if
userfaultfd_event_wait_completion fails as it has to run
userfaultfd_ctx_put on all ctx to pair against the userfaultfd_ctx_get
that was run on all fctx->orig in dup_userfaultfd.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 fs/userfaultfd.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 3d7c248..0072f04 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -526,16 +526,12 @@ int handle_userfault(struct vm_fault *vmf, unsigned long reason)
 	return ret;
 }
 
-static int userfaultfd_event_wait_completion(struct userfaultfd_ctx *ctx,
-					     struct userfaultfd_wait_queue *ewq)
+static void userfaultfd_event_wait_completion(struct userfaultfd_ctx *ctx,
+					      struct userfaultfd_wait_queue *ewq)
 {
-	int ret;
-
-	ret = -1;
 	if (WARN_ON_ONCE(current->flags & PF_EXITING))
 		goto out;
 
-	ret = 0;
 	ewq->ctx = ctx;
 	init_waitqueue_entry(&ewq->wq, current);
 
@@ -551,7 +547,6 @@ static int userfaultfd_event_wait_completion(struct userfaultfd_ctx *ctx,
 			break;
 		if (ACCESS_ONCE(ctx->released) ||
 		    fatal_signal_pending(current)) {
-			ret = -1;
 			__remove_wait_queue(&ctx->event_wqh, &ewq->wq);
 			break;
 		}
@@ -572,7 +567,6 @@ static int userfaultfd_event_wait_completion(struct userfaultfd_ctx *ctx,
 	 */
 out:
 	userfaultfd_ctx_put(ctx);
-	return ret;
 }
 
 static void userfaultfd_event_complete(struct userfaultfd_ctx *ctx,
@@ -630,7 +624,7 @@ int dup_userfaultfd(struct vm_area_struct *vma, struct list_head *fcs)
 	return 0;
 }
 
-static int dup_fctx(struct userfaultfd_fork_ctx *fctx)
+static void dup_fctx(struct userfaultfd_fork_ctx *fctx)
 {
 	struct userfaultfd_ctx *ctx = fctx->orig;
 	struct userfaultfd_wait_queue ewq;
@@ -640,17 +634,15 @@ static int dup_fctx(struct userfaultfd_fork_ctx *fctx)
 	ewq.msg.event = UFFD_EVENT_FORK;
 	ewq.msg.arg.reserved.reserved1 = (unsigned long)fctx->new;
 
-	return userfaultfd_event_wait_completion(ctx, &ewq);
+	userfaultfd_event_wait_completion(ctx, &ewq);
 }
 
 void dup_userfaultfd_complete(struct list_head *fcs)
 {
-	int ret = 0;
 	struct userfaultfd_fork_ctx *fctx, *n;
 
 	list_for_each_entry_safe(fctx, n, fcs, list) {
-		if (!ret)
-			ret = dup_fctx(fctx);
+		dup_fctx(fctx);
 		list_del(&fctx->list);
 		kfree(fctx);
 	}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/3] userfaultfd non-cooperative further update for 4.11 merge window

[Mike Rapoport]

On Fri, 24 Feb 2017 19:19:54 +0100, Andrea Arcangeli wrote:
> Hello,
> 
> unfortunately I noticed one relevant bug in userfaultfd_exit while
> doing more testing. I've been doing testing before and this was also
> tested by kbuild bot and exercised by the selftest, but this bug never
> reproduced before.
> 
> I dropped userfaultfd_exit as result. I dropped it because of
> implementation difficulty in receiving signals in __mmput and because
> I think -ENOSPC as result from the background UFFDIO_COPY should be
> enough already.

The -ENOSPC from UFFDIO_COPY will be enough, I believe.

> Before I decided to remove userfaultfd_exit, I noticed
> userfaultfd_exit wasn't exercised by the selftest and when I tried to
> exercise it, after moving it to a more correct place in __mmput where
> it would make more sense and where the vma list is stable, it resulted
> in the event_wait_completion in D state. So then I added the second
> patch to be sure even if we call userfaultfd_event_wait_completion too
> late during task exit(), we won't risk to generate tasks in D
> state. The same check exists in handle_userfault() for the same
> reason, except it makes a difference there, while here is just a
> robustness check and it's run under WARN_ON_ONCE.
> 
> While looking at the userfaultfd_event_wait_completion() function I
> looked back at its callers too while at it and I think it's not ok to
> stop executing dup_fctx on the fcs list because we relay on
> userfaultfd_event_wait_completion to execute
> userfaultfd_ctx_put(fctx->orig) which is paired against
> userfaultfd_ctx_get(fctx->orig) in dup_userfault just before
> list_add(fcs). This change only takes care of fctx->orig but this area
> also needs further review looking for similar problems in fctx->new.

I'll take a look at fctx->new. 

> The only patch that is urgent is the first because it's an use after
> free during a SMP race condition that affects all processes if
> CONFIG_USERFAULTFD=y. Very hard to reproduce though and probably
> impossible without SLUB poisoning enabled.
> 
> Mike and Pavel please review, thanks!

Thanks for the fixes :)

> Andrea
> 
> Andrea Arcangeli (3):
>   userfaultfd: non-cooperative: rollback userfaultfd_exit
>   userfaultfd: non-cooperative: robustness check
>   userfaultfd: non-cooperative: release all ctx in dup_userfaultfd_complete

Acked-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
 
>  fs/userfaultfd.c                 | 47 +++++++---------------------------------
>  include/linux/userfaultfd_k.h    |  6 -----
>  include/uapi/linux/userfaultfd.h |  5 +----
>  kernel/exit.c                    |  1 -
>  4 files changed, 9 insertions(+), 50 deletions(-)

--
Sincerely yours,
Mike.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1.5/3] userfaultfd: documentation fixup after removal of UFFD_EVENT_EXIT

[Mike Rapoport]

Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
---
Hello Andrew,

It would be great if you can fold the patch below with the patch 1/3
(userfaultfd: non-cooperative: rollback userfaultfd_exit) 

 Documentation/vm/userfaultfd.txt | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/Documentation/vm/userfaultfd.txt b/Documentation/vm/userfaultfd.txt
index fe51a5a..d57e59c 100644
--- a/Documentation/vm/userfaultfd.txt
+++ b/Documentation/vm/userfaultfd.txt
@@ -172,10 +172,6 @@ the same read(2) protocol as for the page fault notifications. The
 manager has to explicitly enable these events by setting appropriate
 bits in uffdio_api.features passed to UFFDIO_API ioctl:
 
-UFFD_FEATURE_EVENT_EXIT - enable notification about exit() of the
-non-cooperative process. When the monitored process exits, the uffd
-manager will get UFFD_EVENT_EXIT.
-
 UFFD_FEATURE_EVENT_FORK - enable userfaultfd hooks for fork(). When
 this feature is enabled, the userfaultfd context of the parent process
 is duplicated into the newly created process. The manager receives
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>