git: 9246b3090cbc - main - fork: Suspend other threads if both RFPROC and RFMEM are not set

[Mark Johnston]

The branch main has been updated by markj:

URL: https://cgit.FreeBSD.org/src/commit/?id=9246b3090cbc82c54350391601b9acef2aa9a625

commit 9246b3090cbc82c54350391601b9acef2aa9a625
Author:     Mark Johnston <markj at FreeBSD.org>
AuthorDate: 2021-05-13 12:33:23 +0000
Commit:     Mark Johnston <markj at FreeBSD.org>
CommitDate: 2021-05-13 12:33:23 +0000

    fork: Suspend other threads if both RFPROC and RFMEM are not set
    
    Otherwise, a multithreaded parent process may trigger races in
    vm_forkproc() if one thread calls rfork() with RFMEM set and another
    calls rfork() without RFMEM.
    
    Also simplify vm_forkproc() a bit, vmspace_unshare() already checks to
    see if the address space is shared.
    
    Reported by:    syzbot+0aa7c2bec74c4066c36f at syzkaller.appspotmail.com
    Reported by:    syzbot+ea84cb06937afeae609d at syzkaller.appspotmail.com
    Reviewed by:    kib
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D30220
---
 sys/kern/kern_fork.c | 13 +++++++++----
 sys/vm/vm_glue.c     |  8 +++-----
 sys/vm/vm_map.c      |  4 ++++
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index 2a092b192878..0d0659b432fe 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -313,8 +313,13 @@ fork_norfproc(struct thread *td, int flags)
 	    ("fork_norfproc called with RFPROC set"));
 	p1 = td->td_proc;
 
-	if (((p1->p_flag & (P_HADTHREADS|P_SYSTEM)) == P_HADTHREADS) &&
-	    (flags & (RFCFDG | RFFDG))) {
+	/*
+	 * Quiesce other threads if necessary.  If RFMEM is not specified we
+	 * must ensure that other threads do not concurrently create a second
+	 * process sharing the vmspace, see vmspace_unshare().
+	 */
+	if ((p1->p_flag & (P_HADTHREADS | P_SYSTEM)) == P_HADTHREADS &&
+	    ((flags & (RFCFDG | RFFDG)) != 0 || (flags & RFMEM) == 0)) {
 		PROC_LOCK(p1);
 		if (thread_single(p1, SINGLE_BOUNDARY)) {
 			PROC_UNLOCK(p1);
@@ -350,8 +355,8 @@ fork_norfproc(struct thread *td, int flags)
 	}
 
 fail:
-	if (((p1->p_flag & (P_HADTHREADS|P_SYSTEM)) == P_HADTHREADS) &&
-	    (flags & (RFCFDG | RFFDG))) {
+	if ((p1->p_flag & (P_HADTHREADS | P_SYSTEM)) == P_HADTHREADS &&
+	    ((flags & (RFCFDG | RFFDG)) != 0 || (flags & RFMEM) == 0)) {
 		PROC_LOCK(p1);
 		thread_single_end(p1, SINGLE_BOUNDARY);
 		PROC_UNLOCK(p1);
diff --git a/sys/vm/vm_glue.c b/sys/vm/vm_glue.c
index 6facf744456c..7cfb08246f9e 100644
--- a/sys/vm/vm_glue.c
+++ b/sys/vm/vm_glue.c
@@ -553,11 +553,9 @@ vm_forkproc(struct thread *td, struct proc *p2, struct thread *td2,
 		 * COW locally.
 		 */
 		if ((flags & RFMEM) == 0) {
-			if (refcount_load(&p1->p_vmspace->vm_refcnt) > 1) {
-				error = vmspace_unshare(p1);
-				if (error)
-					return (error);
-			}
+			error = vmspace_unshare(p1);
+			if (error)
+				return (error);
 		}
 		cpu_fork(td, p2, td2, flags);
 		return (0);
diff --git a/sys/vm/vm_map.c b/sys/vm/vm_map.c
index d870fe3507fd..1ac4ccf72f11 100644
--- a/sys/vm/vm_map.c
+++ b/sys/vm/vm_map.c
@@ -4867,6 +4867,10 @@ vmspace_unshare(struct proc *p)
 	struct vmspace *newvmspace;
 	vm_ooffset_t fork_charge;
 
+	/*
+	 * The caller is responsible for ensuring that the reference count
+	 * cannot concurrently transition 1 -> 2.
+	 */
 	if (refcount_load(&oldvmspace->vm_refcnt) == 1)
 		return (0);
 	fork_charge = 0;