Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 23 Mar 2011 09:45:37 -0400
From:      John Baldwin <jhb@freebsd.org>
To:        Julian Elischer <julian@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Edward Tomasz Napierala <trasz@freebsd.org>
Subject:   Re: svn commit: r219727 - head/sys/vm
Message-ID:  <201103230945.37726.jhb@freebsd.org>
In-Reply-To: <4D831A82.7070308@freebsd.org>
References:  <201103180647.p2I6lNCB051745@svn.freebsd.org> <4D831A82.7070308@freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Friday, March 18, 2011 4:40:34 am Julian Elischer wrote:
> On 3/17/11 11:47 PM, Edward Tomasz Napierala wrote:
> > Author: trasz
> > Date: Fri Mar 18 06:47:23 2011
> > New Revision: 219727
> > URL: http://svn.freebsd.org/changeset/base/219727
> >
> > Log:
> >    In vm_daemon(), when iterating over all processes in the system, skip those
> >    which are not yet fully initialized (i.e. ones with p_state == PRS_NEW).
> >    Without it, we could panic in _thread_lock_flags().
> >
> >    Note that there may be other instances of FOREACH_PROC_IN_SYSTEM() that
> >    require similar fix.
> 
> In the past each process was only put on the process list after it was 
> fully set up.
> Did someone change that recently?  that would be "A Bad Thing" (TM).

Err, no, that has never been true.  The reason it has to go on the list
immediately is to reserve the PID against concurrent fork()s.

Hmm, the locking of prs_state is a bit busted it seems.  Both the PROC_LOCK()
and PROC_SLOCK() are supposed to be held when it is written to, but
PROC_LOCK() is missing in fork1() when moving the state to PRS_NORMAL.

Also, this commit should check against PRS_NORMAL after acquiring the proc
lock, not before.

Hmm, I did an audit of prs_state and there is lots of locking rot. :(  Below
is a set of fixes I will commit once it has been tested some:

Index: kern/kern_thread.c
===================================================================
--- kern/kern_thread.c	(revision 219902)
+++ kern/kern_thread.c	(working copy)
@@ -981,7 +981,9 @@
 				td = NULL;
 				break;
 			}
+			PROC_LOCK(td->td_proc);
 			if (td->td_proc->p_state == PRS_NEW) {
+				PROC_UNLOCK(td->td_proc);
 				td = NULL;
 				break;
 			}
@@ -990,12 +992,10 @@
 					LIST_REMOVE(td, td_hash);
 					LIST_INSERT_HEAD(TIDHASH(td->td_tid),
 						td, td_hash);
-					PROC_LOCK(td->td_proc);
 					rw_wunlock(&tidhash_lock);
 					return (td);
 				}
 			}
-			PROC_LOCK(td->td_proc);
 			break;
 		}
 		run++;
Index: kern/kern_proc.c
===================================================================
--- kern/kern_proc.c	(revision 219902)
+++ kern/kern_proc.c	(working copy)
@@ -291,11 +291,11 @@
 	sx_slock(&allproc_lock);
 	LIST_FOREACH(p, PIDHASH(pid), p_hash)
 		if (p->p_pid == pid) {
+			PROC_LOCK(p);
 			if (p->p_state == PRS_NEW) {
+				PROC_UNLOCK(p);
 				p = NULL;
-				break;
 			}
-			PROC_LOCK(p);
 			break;
 		}
 	sx_sunlock(&allproc_lock);
@@ -756,7 +756,6 @@
 		kp->ki_sigcatch = ps->ps_sigcatch;
 		mtx_unlock(&ps->ps_mtx);
 	}
-	PROC_SLOCK(p);
 	if (p->p_state != PRS_NEW &&
 	    p->p_state != PRS_ZOMBIE &&
 	    p->p_vmspace != NULL) {
@@ -782,12 +781,11 @@
 	kp->ki_swtime = (ticks - p->p_swtick) / hz;
 	kp->ki_pid = p->p_pid;
 	kp->ki_nice = p->p_nice;
-	rufetch(p, &kp->ki_rusage);
-	kp->ki_runtime = cputick2usec(p->p_rux.rux_runtime);
-	PROC_SUNLOCK(p);
 	kp->ki_start = p->p_stats->p_start;
 	timevaladd(&kp->ki_start, &boottime);
 	PROC_SLOCK(p);
+	rufetch(p, &kp->ki_rusage);
+	kp->ki_runtime = cputick2usec(p->p_rux.rux_runtime);
 	calcru(p, &kp->ki_rusage.ru_utime, &kp->ki_rusage.ru_stime);
 	PROC_SUNLOCK(p);
 	calccru(p, &kp->ki_childutime, &kp->ki_childstime);
@@ -1213,13 +1211,11 @@
 			/*
 			 * Skip embryonic processes.
 			 */
-			PROC_SLOCK(p);
+			PROC_LOCK(p);
 			if (p->p_state == PRS_NEW) {
-				PROC_SUNLOCK(p);
+				PROC_UNLOCK(p);
 				continue;
 			}
-			PROC_SUNLOCK(p);
-			PROC_LOCK(p);
 			KASSERT(p->p_ucred != NULL,
 			    ("process credential is NULL for non-NEW proc"));
 			/*
Index: kern/kern_descrip.c
===================================================================
--- kern/kern_descrip.c	(revision 219902)
+++ kern/kern_descrip.c	(working copy)
@@ -2634,9 +2634,11 @@
 	xf.xf_size = sizeof(xf);
 	sx_slock(&allproc_lock);
 	FOREACH_PROC_IN_SYSTEM(p) {
-		if (p->p_state == PRS_NEW)
+		PROC_LOCK(p);
+		if (p->p_state == PRS_NEW) {
+			PROC_UNLOCK(p);
 			continue;
-		PROC_LOCK(p);
+		}
 		if (p_cansee(req->td, p) != 0) {
 			PROC_UNLOCK(p);
 			continue;
Index: kern/kern_fork.c
===================================================================
--- kern/kern_fork.c	(revision 219902)
+++ kern/kern_fork.c	(working copy)
@@ -630,12 +630,13 @@
 	/*
 	 * Set the child start time and mark the process as being complete.
 	 */
+	PROC_LOCK(p2);
+	PROC_LOCK(p1);
 	microuptime(&p2->p_stats->p_start);
 	PROC_SLOCK(p2);
 	p2->p_state = PRS_NORMAL;
 	PROC_SUNLOCK(p2);
 
-	PROC_LOCK(p1);
 #ifdef KDTRACE_HOOKS
 	/*
 	 * Tell the DTrace fasttrap provider about the new process
@@ -643,11 +644,8 @@
 	 * p_state is PRS_NORMAL since the fasttrap module will use pfind()
 	 * later on.
 	 */
-	if (dtrace_fasttrap_fork) {
-		PROC_LOCK(p2);
+	if (dtrace_fasttrap_fork)
 		dtrace_fasttrap_fork(p1, p2);
-		PROC_UNLOCK(p2);
-	}
 #endif
 	if ((p1->p_flag & (P_TRACED | P_FOLLOWFORK)) == (P_TRACED |
 	    P_FOLLOWFORK)) {
@@ -660,12 +658,11 @@
 		 */
 		td->td_dbgflags |= TDB_FORK;
 		td->td_dbg_forked = p2->p_pid;
-		PROC_LOCK(p2);
 		td2->td_dbgflags |= TDB_STOPATFORK;
 		_PHOLD(p2);
 		p2_held = 1;
-		PROC_UNLOCK(p2);
 	}
+	PROC_UNLOCK(p2);
 	if ((flags & RFSTOPPED) == 0) {
 		/*
 		 * If RFSTOPPED not requested, make child runnable and
Index: fs/nfsclient/nfs_clport.c
===================================================================
--- fs/nfsclient/nfs_clport.c	(revision 219902)
+++ fs/nfsclient/nfs_clport.c	(working copy)
@@ -1102,11 +1102,11 @@
 
 	LIST_FOREACH(p, PIDHASH(pid), p_hash)
 		if (p->p_pid == pid) {
+			PROC_LOCK(p);
 			if (p->p_state == PRS_NEW) {
+				PROC_UNLOCK(p);
 				p = NULL;
-				break;
 			}
-			PROC_LOCK(p);
 			break;
 		}
 	return (p);
Index: vm/vm_pageout.c
===================================================================
--- vm/vm_pageout.c	(revision 219902)
+++ vm/vm_pageout.c	(working copy)
@@ -1281,14 +1281,13 @@
 	FOREACH_PROC_IN_SYSTEM(p) {
 		int breakout;
 
-		if (p->p_state != PRS_NORMAL)
-			continue;
 		if (PROC_TRYLOCK(p) == 0)
 			continue;
 		/*
 		 * If this is a system, protected or killed process, skip it.
 		 */
-		if ((p->p_flag & (P_INEXEC | P_PROTECTED | P_SYSTEM)) ||
+		if (p->p_state != PRS_NORMAL ||
+		    (p->p_flag & (P_INEXEC | P_PROTECTED | P_SYSTEM)) ||
 		    (p->p_pid == 1) || P_KILLED(p) ||
 		    ((p->p_pid < 48) && (swap_pager_avail != 0))) {
 			PROC_UNLOCK(p);
@@ -1651,14 +1650,13 @@
 		FOREACH_PROC_IN_SYSTEM(p) {
 			vm_pindex_t limit, size;
 
-			if (p->p_state != PRS_NORMAL)
-				continue;
 			/*
 			 * if this is a system process or if we have already
 			 * looked at this process, skip it.
 			 */
 			PROC_LOCK(p);
-			if (p->p_flag & (P_INEXEC | P_SYSTEM | P_WEXIT)) {
+			if (p->p_state != PRS_NORMAL ||
+			    p->p_flag & (P_INEXEC | P_SYSTEM | P_WEXIT)) {
 				PROC_UNLOCK(p);
 				continue;
 			}
Index: vm/vm_meter.c
===================================================================
--- vm/vm_meter.c	(revision 219902)
+++ vm/vm_meter.c	(working copy)
@@ -130,15 +130,12 @@
 		if (p->p_flag & P_SYSTEM)
 			continue;
 		PROC_LOCK(p);
-		PROC_SLOCK(p);
 		switch (p->p_state) {
 		case PRS_NEW:
-			PROC_SUNLOCK(p);
 			PROC_UNLOCK(p);
 			continue;
 			break;
 		default:
-			PROC_SUNLOCK(p);
 			FOREACH_THREAD_IN_PROC(p, td) {
 				thread_lock(td);
 				switch (td->td_state) {

-- 
John Baldwin



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