Change the pfind() and zpfind() functions to lock the process that they

find before releasing the allproc lock and returning.

Reviewed by:	-smp, dfr, jake

11 files changed, 173 insertions, 115 deletions

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 90023ee..9a0a25c 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -486,8 +486,11 @@ fsetown(pgid, sigiop)
 		 * restrict FSETOWN to the current process or process
 		 * group for maximum safety.
 		 */
-		if (proc->p_session != curproc->p_session)
+		if (proc->p_session != curproc->p_session) {
+			PROC_UNLOCK(proc);
 			return (EPERM);
+		}
+		PROC_UNLOCK(proc);
 
 		pgrp = NULL;
 	} else /* if (pgid < 0) */ {
diff --git a/sys/kern/kern_event.c b/sys/kern/kern_event.c
index 2cf1de9..4156460 100644
--- a/sys/kern/kern_event.c
+++ b/sys/kern/kern_event.c
@@ -170,8 +170,10 @@ filt_procattach(struct knote *kn)
 	p = pfind(kn->kn_id);
 	if (p == NULL)
 		return (ESRCH);
-	if ((error = p_can(curproc, p, P_CAN_SEE, NULL)))
+	if ((error = p_can(curproc, p, P_CAN_SEE, NULL))) {
+		PROC_UNLOCK(p);
 		return (error);
+	}
 
 	kn->kn_ptr.p_proc = p;
 	kn->kn_flags |= EV_CLEAR;		/* automatically set */
@@ -185,7 +187,6 @@ filt_procattach(struct knote *kn)
 		kn->kn_flags &= ~EV_FLAG1;
 	}
 
-	PROC_LOCK(p);
 	SLIST_INSERT_HEAD(&p->p_klist, kn, kn_selnext);
 	PROC_UNLOCK(p);
 
diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c
index 3267b21..209210d 100644
--- a/sys/kern/kern_exit.c
+++ b/sys/kern/kern_exit.c
@@ -496,11 +496,10 @@ loop:
 					p->p_oppid = 0;
 					proc_reparent(p, t);
 					PROC_UNLOCK(p);
-					PROC_LOCK(t);
 					psignal(t, SIGCHLD);
+					wakeup((caddr_t)t);
 					PROC_UNLOCK(t);
 					sx_xunlock(&proctree_lock);
-					wakeup((caddr_t)t);
 					return (0);
 				}
 			}
diff --git a/sys/kern/kern_ktrace.c b/sys/kern/kern_ktrace.c
index 93a2443..393d3da 100644
--- a/sys/kern/kern_ktrace.c
+++ b/sys/kern/kern_ktrace.c
@@ -328,6 +328,7 @@ ktrace(curp, uap)
 			error = ESRCH;
 			goto done;
 		}
+		PROC_UNLOCK(p);
 		if (descend)
 			ret |= ktrsetchildren(curp, p, ops, facs, vp);
 		else
diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c
index 67e78bb..8019a27 100644
--- a/sys/kern/kern_proc.c
+++ b/sys/kern/kern_proc.c
@@ -119,15 +119,16 @@ int
 inferior(p)
 	register struct proc *p;
 {
-	int rval = 1;
+	int rval;
 
-	sx_slock(&proctree_lock);
-	for (; p != curproc; p = p->p_pptr)
-		if (p->p_pid == 0) {
-			rval = 0;
-			break;
-		}
-	sx_sunlock(&proctree_lock);
+	PROC_LOCK_ASSERT(p, MA_OWNED);
+	if (p == curproc)
+		return (1);
+	if (p->p_pid == 0)
+		return (0);
+	PROC_LOCK(p->p_pptr);
+	rval = inferior(p->p_pptr);
+	PROC_UNLOCK(p->p_pptr);
 	return (rval);
 }
 
@@ -142,8 +143,10 @@ pfind(pid)
 
 	sx_slock(&allproc_lock);
 	LIST_FOREACH(p, PIDHASH(pid), p_hash)
-		if (p->p_pid == pid)
+		if (p->p_pid == pid) {
+			PROC_LOCK(p);
 			break;
+		}
 	sx_sunlock(&allproc_lock);
 	return (p);
 }
@@ -188,8 +191,12 @@ enterpgrp(p, pgid, mksess)
 		 */
 		KASSERT(p->p_pid == pgid,
 		    ("enterpgrp: new pgrp and pid != pgid"));
-		if ((np = pfind(savepid)) == NULL || np != p)
+		if ((np = pfind(savepid)) == NULL || np != p) {
+			if (np != NULL)
+				PROC_UNLOCK(np);
 			return (ESRCH);
+		}
+		PROC_UNLOCK(np);
 		MALLOC(pgrp, struct pgrp *, sizeof(struct pgrp), M_PGRP,
 		    M_WAITOK);
 		if (mksess) {
@@ -523,8 +530,10 @@ zpfind(pid_t pid)
 
 	sx_slock(&allproc_lock);
 	LIST_FOREACH(p, &zombproc, p_list)
-		if (p->p_pid == pid)
+		if (p->p_pid == pid) {
+			PROC_LOCK(p);
 			break;
+		}
 	sx_sunlock(&allproc_lock);
 	return (p);
 }
@@ -535,16 +544,27 @@ sysctl_out_proc(struct proc *p, struct sysctl_req *req, int doingzomb)
 {
 	struct kinfo_proc kinfo_proc;
 	int error;
+	struct proc *np;
 	pid_t pid = p->p_pid;
 
 	fill_kinfo_proc(p, &kinfo_proc);
 	error = SYSCTL_OUT(req, (caddr_t)&kinfo_proc, sizeof(kinfo_proc));
 	if (error)
 		return (error);
-	if (!doingzomb && pid && (pfind(pid) != p))
+	if (doingzomb)
+		np = zpfind(pid);
+	else {
+		if (pid == 0)
+			return (0);
+		np = pfind(pid);
+	}
+	if (np == NULL)
 		return EAGAIN;
-	if (doingzomb && zpfind(pid) != p)
+	if (np != p) {
+		PROC_UNLOCK(np);
 		return EAGAIN;
+	}
+	PROC_UNLOCK(np);
 	return (0);
 }
 
@@ -563,8 +583,11 @@ sysctl_kern_proc(SYSCTL_HANDLER_ARGS)
 		p = pfind((pid_t)name[0]);
 		if (!p)
 			return (0);
-		if (p_can(curproc, p, P_CAN_SEE, NULL))
+		if (p_can(curproc, p, P_CAN_SEE, NULL)) {
+			PROC_UNLOCK(p);
 			return (0);
+		}
+		PROC_UNLOCK(p);
 		error = sysctl_out_proc(p, req, 0);
 		return (error);
 	}
@@ -669,8 +692,11 @@ sysctl_kern_proc_args(SYSCTL_HANDLER_ARGS)
 	if (!p)
 		return (0);
 
-	if ((!ps_argsopen) && p_can(curproc, p, P_CAN_SEE, NULL))
+	if ((!ps_argsopen) && p_can(curproc, p, P_CAN_SEE, NULL)) {
+		PROC_UNLOCK(p);
 		return (0);
+	}
+	PROC_UNLOCK(p);
 
 	if (req->newptr && curproc != p)
 		return (EPERM);
@@ -680,9 +706,9 @@ sysctl_kern_proc_args(SYSCTL_HANDLER_ARGS)
 	if (req->newptr == NULL)
 		return (error);
 
+	PROC_LOCK(p);
 	if (p->p_args && --p->p_args->ar_ref == 0) 
 		FREE(p->p_args, M_PARGS);
-	PROC_LOCK(p);
 	p->p_args = NULL;
 	PROC_UNLOCK(p);
 
diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c
index 8aa9018..33095af 100644
--- a/sys/kern/kern_prot.c
+++ b/sys/kern/kern_prot.c
@@ -145,16 +145,18 @@ getpgid(p, uap)
 	struct proc *pt;
 	int error;
 
-	pt = p;
 	if (uap->pid == 0)
-		goto found;
-
-	if ((pt = pfind(uap->pid)) == 0)
-		return ESRCH;
-	if ((error = p_can(p, pt, P_CAN_SEE, NULL)))
-		return (error);
-found:
-	p->p_retval[0] = pt->p_pgrp->pg_id;
+		p->p_retval[0] = p->p_pgrp->pg_id;
+	else {
+		if ((pt = pfind(uap->pid)) == NULL)
+			return ESRCH;
+		if ((error = p_can(p, pt, P_CAN_SEE, NULL))) {
+			PROC_UNLOCK(pt);
+			return (error);
+		}
+		p->p_retval[0] = pt->p_pgrp->pg_id;
+		PROC_UNLOCK(pt);
+	}
 	return 0;
 }
 
@@ -175,16 +177,18 @@ getsid(p, uap)
 	struct proc *pt;
 	int error;
 
-	pt = p;
 	if (uap->pid == 0)
-		goto found;
-
-	if ((pt = pfind(uap->pid)) == 0)
-		return ESRCH;
-	if ((error = p_can(p, pt, P_CAN_SEE, NULL)))
-		return (error);
-found:
-	p->p_retval[0] = pt->p_session->s_sid;
+		p->p_retval[0] = p->p_session->s_sid;
+	else {
+		if ((pt = pfind(uap->pid)) == NULL)
+			return ESRCH;
+		if ((error = p_can(p, pt, P_CAN_SEE, NULL))) {
+			PROC_UNLOCK(pt);
+			return (error);
+		}
+		p->p_retval[0] = pt->p_session->s_sid;
+		PROC_UNLOCK(pt);
+	}
 	return 0;
 }
 
@@ -360,24 +364,42 @@ setpgid(curp, uap)
 	if (uap->pgid < 0)
 		return (EINVAL);
 	if (uap->pid != 0 && uap->pid != curp->p_pid) {
-		if ((targp = pfind(uap->pid)) == 0 || !inferior(targp))
+		if ((targp = pfind(uap->pid)) == NULL || !inferior(targp)) {
+			if (targp)
+				PROC_UNLOCK(targp);
 			return (ESRCH);
-		if ((error = p_can(curproc, targp, P_CAN_SEE, NULL)))
+		}
+		if ((error = p_can(curproc, targp, P_CAN_SEE, NULL))) {
+			PROC_UNLOCK(targp);
 			return (error);
-		if (targp->p_pgrp == NULL ||  targp->p_session != curp->p_session)
+		}
+		if (targp->p_pgrp == NULL ||
+		    targp->p_session != curp->p_session) {
+			PROC_UNLOCK(targp);
 			return (EPERM);
-		if (targp->p_flag & P_EXEC)
+		}
+		if (targp->p_flag & P_EXEC) {
+			PROC_UNLOCK(targp);
 			return (EACCES);
-	} else
+		}
+	} else {
 		targp = curp;
-	if (SESS_LEADER(targp))
+		PROC_LOCK(curp);	/* XXX: not needed */
+	}
+	if (SESS_LEADER(targp)) {
+		PROC_UNLOCK(targp);
 		return (EPERM);
+	}
 	if (uap->pgid == 0)
 		uap->pgid = targp->p_pid;
 	else if (uap->pgid != targp->p_pid)
 		if ((pgrp = pgfind(uap->pgid)) == 0 ||
-	            pgrp->pg_session != curp->p_session)
+	            pgrp->pg_session != curp->p_session) {
+			PROC_UNLOCK(targp);
 			return (EPERM);
+		}
+	/* XXX: We should probably hold the lock across enterpgrp. */
+	PROC_UNLOCK(targp);
 	return (enterpgrp(targp, uap->pgid, 0));
 }
 
diff --git a/sys/kern/kern_resource.c b/sys/kern/kern_resource.c
index 805d6aa..06af873 100644
--- a/sys/kern/kern_resource.c
+++ b/sys/kern/kern_resource.c
@@ -94,14 +94,15 @@ getpriority(curp, uap)
 
 	case PRIO_PROCESS:
 		if (uap->who == 0)
-			p = curp;
-		else
+			low = curp->p_nice;
+		else {
 			p = pfind(uap->who);
-		if (p == 0)
-			break;
-		if (p_can(curp, p, P_CAN_SEE, NULL))
-			break;
-		low = p->p_nice;
+			if (p == NULL)
+				break;
+			if (p_can(curp, p, P_CAN_SEE, NULL) == 0)
+				low = p->p_nice;
+			PROC_UNLOCK(p);
+		}
 		break;
 
 	case PRIO_PGRP: {
@@ -159,14 +160,15 @@ setpriority(curp, uap)
 
 	case PRIO_PROCESS:
 		if (uap->who == 0)
-			p = curp;
-		else
+			error = donice(curp, curp, uap->prio);
+		else {
 			p = pfind(uap->who);
-		if (p == 0)
-			break;
-		if (p_can(curp, p, P_CAN_SEE, NULL))
-			break;
-		error = donice(curp, p, uap->prio);
+			if (p == 0)
+				break;
+			if (p_can(curp, p, P_CAN_SEE, NULL) == 0)
+				error = donice(curp, p, uap->prio);
+			PROC_UNLOCK(p);
+		}
 		found++;
 		break;
 
@@ -254,9 +256,10 @@ rtprio(curp, uap)
 	if (error)
 		return (error);
 
-	if (uap->pid == 0)
+	if (uap->pid == 0) {
 		p = curp;
-	else
+		PROC_LOCK(p);
+	} else
 		p = pfind(uap->pid);
 
 	if (p == 0)
@@ -267,15 +270,18 @@ rtprio(curp, uap)
 		if ((error = p_can(curp, p, P_CAN_SEE, NULL)))
 			return (error);
 		pri_to_rtp(&p->p_pri, &rtp);
+		PROC_UNLOCK(p);
 		return (copyout(&rtp, uap->rtp, sizeof(struct rtprio)));
 	case RTP_SET:
 		if ((error = p_can(curp, p, P_CAN_SCHED, NULL)))
-		        return (error);
+			goto out;
 		/* disallow setting rtprio in most cases if not superuser */
 		if (suser(curp) != 0) {
 			/* can't set someone else's */
-			if (uap->pid)
-				return (EPERM);
+			if (uap->pid) {
+				error = EPERM;
+				goto out;
+			}
 			/* can't set realtime priority */
 /*
  * Realtime priority has to be restricted for reasons which should be
@@ -287,15 +293,21 @@ rtprio(curp, uap)
 #if 0
  			if (RTP_PRIO_IS_REALTIME(rtp.type))
 #endif
-			if (rtp.type != RTP_PRIO_NORMAL)
-				return (EPERM);
+			if (rtp.type != RTP_PRIO_NORMAL) {
+				error = EPERM;
+				goto out;
+			}
 		}
 		if (rtp_to_pri(&rtp, &p->p_pri) == 0)
-			return (0);
-		return (EINVAL);
+			error = 0;
+		else
+			error = EINVAL;
 	default:
-		return (EINVAL);
+		error = EINVAL;
 	}
+out:
+	PROC_UNLOCK(p);
+	return (error);
 }
 
 int
diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index e3f2fdb..b387237 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -898,19 +898,12 @@ killpg1(cp, sig, pgid, all)
 				PROC_UNLOCK(p);
 				continue;
 			}
-			PROC_UNLOCK(p);
-			/*
-			 * XXX: this locking needs work.. specifically the
-			 * session checks..
-			 */
-			if (p_cansignal(cp, p, sig))
-				continue;
-			nfound++;
-			if (sig) {
-				PROC_LOCK(p);
-				psignal(p, sig);
-				PROC_UNLOCK(p);
+			if (p_cansignal(cp, p, sig) == 0) {
+				nfound++;
+				if (sig)
+					psignal(p, sig);
 			}
+			PROC_UNLOCK(p);
 		}
 		sx_sunlock(&allproc_lock);
 	} else {
@@ -930,22 +923,19 @@ killpg1(cp, sig, pgid, all)
 				PROC_UNLOCK(p);
 				continue;
 			}
-			PROC_UNLOCK(p);
 			mtx_lock_spin(&sched_lock);
 			if (p->p_stat == SZOMB) {
 				mtx_unlock_spin(&sched_lock);
+				PROC_UNLOCK(p);
 				continue;
 			}
 			mtx_unlock_spin(&sched_lock);
-			/* XXX: locking b0rked */
-			if (p_cansignal(cp, p, sig))
-				continue;
-			nfound++;
-			if (sig) {
-				PROC_LOCK(p);
-				psignal(p, sig);
-				PROC_UNLOCK(p);
+			if (p_cansignal(cp, p, sig) == 0) {
+				nfound++;
+				if (sig)
+					psignal(p, sig);
 			}
+			PROC_UNLOCK(p);
 		}
 	}
 	return (nfound ? 0 : ESRCH);
@@ -971,14 +961,13 @@ kill(cp, uap)
 		/* kill single process */
 		if ((p = pfind(uap->pid)) == NULL)
 			return (ESRCH);
-		/* XXX: locking b0rked */
-		if (p_cansignal(cp, p, uap->signum))
-			return (EPERM);
-		if (uap->signum) {
-			PROC_LOCK(p);
-			psignal(p, uap->signum);
+		if (p_cansignal(cp, p, uap->signum)) {
 			PROC_UNLOCK(p);
+			return (EPERM);
 		}
+		if (uap->signum)
+			psignal(p, uap->signum);
+		PROC_UNLOCK(p);
 		return (0);
 	}
 	switch (uap->pid) {
diff --git a/sys/kern/p1003_1b.c b/sys/kern/p1003_1b.c
index 860176a..0d1fd0e 100644
--- a/sys/kern/p1003_1b.c
+++ b/sys/kern/p1003_1b.c
@@ -87,9 +87,10 @@ int p31b_proc(struct proc *p, pid_t pid, struct proc **pp)
 	int ret = 0;
 	struct proc *other_proc = 0;
 
-	if (pid == 0)
+	if (pid == 0) {
 		other_proc = p;
-	else
+		PROC_LOCK(p);
+	} else
 		other_proc = pfind(pid);
 
 	if (other_proc)
@@ -100,6 +101,7 @@ int p31b_proc(struct proc *p, pid_t pid, struct proc **pp)
 			*pp = other_proc;
 		else
 			ret = EPERM;
+		PROC_UNLOCK(other_proc);
 	}
 	else
 		ret = ESRCH;
diff --git a/sys/kern/sys_generic.c b/sys/kern/sys_generic.c
index a779711..9414574 100644
--- a/sys/kern/sys_generic.c
+++ b/sys/kern/sys_generic.c
@@ -1034,10 +1034,12 @@ selrecord(selector, sip)
 		mtx_lock_spin(&sched_lock);
 	    	if (p->p_wchan == (caddr_t)&selwait) {
 			mtx_unlock_spin(&sched_lock);
+			PROC_UNLOCK(p);
 			sip->si_flags |= SI_COLL;
 			return;
 		}
 		mtx_unlock_spin(&sched_lock);
+		PROC_UNLOCK(p);
 	}
 	sip->si_pid = mypid;
 }
@@ -1067,12 +1069,9 @@ selwakeup(sip)
 				setrunnable(p);
 			else
 				unsleep(p);
-			mtx_unlock_spin(&sched_lock);
-		} else {
-			mtx_unlock_spin(&sched_lock);
-			PROC_LOCK(p);
+		} else
 			p->p_flag &= ~P_SELECT;
-			PROC_UNLOCK(p);
-		}
+		mtx_unlock_spin(&sched_lock);
+		PROC_UNLOCK(p);
 	}
 }
diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index 4255c14..f6af557 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
@@ -211,14 +211,17 @@ ptrace(curp, uap)
 	int write;
 
 	write = 0;
-	if (uap->req == PT_TRACE_ME)
+	if (uap->req == PT_TRACE_ME) {
 		p = curp;
-	else {
+		PROC_LOCK(p);
+	} else {
 		if ((p = pfind(uap->pid)) == NULL)
 			return ESRCH;
 	}
-	if (p_can(curp, p, P_CAN_SEE, NULL))
+	if (p_can(curp, p, P_CAN_SEE, NULL)) {
+		PROC_UNLOCK(p);
 		return (ESRCH);
+	}
 
 	/*
 	 * Permissions check
@@ -230,19 +233,21 @@ ptrace(curp, uap)
 
 	case PT_ATTACH:
 		/* Self */
-		if (p->p_pid == curp->p_pid)
+		if (p->p_pid == curp->p_pid) {
+			PROC_UNLOCK(p);
 			return EINVAL;
+		}
 
 		/* Already traced */
-		PROC_LOCK(p);
 		if (p->p_flag & P_TRACED) {
 			PROC_UNLOCK(p);
 			return EBUSY;
 		}
-		PROC_UNLOCK(p);
 
-		if ((error = p_can(curp, p, P_CAN_DEBUG, NULL)))
+		if ((error = p_can(curp, p, P_CAN_DEBUG, NULL))) {
+			PROC_UNLOCK(p);
 			return error;
+		}
 
 		/* OK */
 		break;
@@ -276,7 +281,6 @@ ptrace(curp, uap)
 	case PT_SETDBREGS:
 #endif
 		/* not being traced... */
-		PROC_LOCK(p);
 		if ((p->p_flag & P_TRACED) == 0) {
 			PROC_UNLOCK(p);
 			return EPERM;
@@ -296,15 +300,16 @@ ptrace(curp, uap)
 			return EBUSY;
 		}
 		mtx_unlock_spin(&sched_lock);
-		PROC_UNLOCK(p);
 
 		/* OK */
 		break;
 
 	default:
+		PROC_UNLOCK(p);
 		return EINVAL;
 	}
 
+	PROC_UNLOCK(p);
 #ifdef FIX_SSTEP
 	/*
 	 * Single step fixup ala procfs
@@ -374,7 +379,6 @@ ptrace(curp, uap)
 				struct proc *pp;
 
 				pp = pfind(p->p_oppid);
-				PROC_LOCK(p);
 				proc_reparent(p, pp ? pp : initproc);
 			} else
 				PROC_LOCK(p);