Skip site navigation (1)Skip section navigation (2) 
Date:      Sat, 17 Aug 2019 18:19:49 +0000 (UTC)
From:      Mateusz Guzik <mjg@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r351175 - head/sys/kern
Message-ID:  <201908171819.x7HIJnAF089691@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: mjg
Date: Sat Aug 17 18:19:49 2019
New Revision: 351175
URL: https://svnweb.freebsd.org/changeset/base/351175

Log:
  fork: rework locking around do_fork
  
  - move allproc lock into the func, it is of no use prior to it
  - the code would lock p1 and p2 while holding allproc to partially
  construct it after it gets added to the list. instead we can do the
  work prior to adding anything.
  - protect lastpid with procid_lock
  
  As a side effect we do less work with allproc held.
  
  Sponsored by:	The FreeBSD Foundation

Modified:
  head/sys/kern/kern_fork.c

Modified: head/sys/kern/kern_fork.c
==============================================================================
--- head/sys/kern/kern_fork.c	Sat Aug 17 17:56:43 2019	(r351174)
+++ head/sys/kern/kern_fork.c	Sat Aug 17 18:19:49 2019	(r351175)
@@ -246,17 +246,24 @@ static int
 fork_findpid(int flags)
 {
 	pid_t result;
-	int trypid;
+	int trypid, random;
 
+	/*
+	 * Avoid calling arc4random with procid_lock held.
+	 */
+	random = 0;
+	if (__predict_false(randompid))
+		random = arc4random() % randompid;
+
+	mtx_lock(&procid_lock);
+
 	trypid = lastpid + 1;
 	if (flags & RFHIGHPID) {
 		if (trypid < 10)
 			trypid = 10;
 	} else {
-		if (randompid)
-			trypid += arc4random() % randompid;
+		trypid += random;
 	}
-	mtx_lock(&procid_lock);
 retry:
 	if (trypid >= pid_max)
 		trypid = 2;
@@ -341,33 +348,16 @@ do_fork(struct thread *td, struct fork_req *fr, struct
     struct vmspace *vm2, struct file *fp_procdesc)
 {
 	struct proc *p1, *pptr;
-	int trypid;
 	struct filedesc *fd;
 	struct filedesc_to_leader *fdtol;
 	struct sigacts *newsigacts;
 
-	sx_assert(&allproc_lock, SX_XLOCKED);
-
 	p1 = td->td_proc;
 
-	trypid = fork_findpid(fr->fr_flags);
-	p2->p_state = PRS_NEW;		/* protect against others */
-	p2->p_pid = trypid;
-	AUDIT_ARG_PID(p2->p_pid);
-	LIST_INSERT_HEAD(&allproc, p2, p_list);
-	allproc_gen++;
-	sx_xlock(PIDHASHLOCK(p2->p_pid));
-	LIST_INSERT_HEAD(PIDHASH(p2->p_pid), p2, p_hash);
-	sx_xunlock(PIDHASHLOCK(p2->p_pid));
-	PROC_LOCK(p2);
 	PROC_LOCK(p1);
-
-	sx_xunlock(&allproc_lock);
-
 	bcopy(&p1->p_startcopy, &p2->p_startcopy,
 	    __rangeof(struct proc, p_startcopy, p_endcopy));
 	pargs_hold(p2->p_args);
-
 	PROC_UNLOCK(p1);
 
 	bzero(&p2->p_startzero,
@@ -376,8 +366,19 @@ do_fork(struct thread *td, struct fork_req *fr, struct
 	/* Tell the prison that we exist. */
 	prison_proc_hold(p2->p_ucred->cr_prison);
 
-	PROC_UNLOCK(p2);
+	p2->p_state = PRS_NEW;		/* protect against others */
+	p2->p_pid = fork_findpid(fr->fr_flags);
+	AUDIT_ARG_PID(p2->p_pid);
 
+	sx_xlock(&allproc_lock);
+	LIST_INSERT_HEAD(&allproc, p2, p_list);
+	allproc_gen++;
+	sx_xunlock(&allproc_lock);
+
+	sx_xlock(PIDHASHLOCK(p2->p_pid));
+	LIST_INSERT_HEAD(PIDHASH(p2->p_pid), p2, p_hash);
+	sx_xunlock(PIDHASHLOCK(p2->p_pid));
+
 	tidhash_add(td2);
 
 	/*
@@ -969,8 +970,6 @@ fork1(struct thread *td, struct fork_req *fr)
 	newproc->p_klist = knlist_alloc(&newproc->p_mtx);
 	STAILQ_INIT(&newproc->p_ktr);
 
-	sx_xlock(&allproc_lock);
-
 	/*
 	 * Increment the count of procs running with this uid. Don't allow
 	 * a nonprivileged user to exceed their current limit.
@@ -986,7 +985,6 @@ fork1(struct thread *td, struct fork_req *fr)
 	return (0);
 fail0:
 	error = EAGAIN;
-	sx_xunlock(&allproc_lock);
 #ifdef MAC
 	mac_proc_destroy(newproc);
 #endif

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201908171819.x7HIJnAF089691>