From: Matthew Dillon <dillon@apollo.backplane.com>
Date: Wed, 16 Feb 2011 05:53:05 +0000 (-0800)
Subject: kernel - Fix MP refcount race in struct pargs
X-Git-Tag: v2.11.0~267^2~65
X-Git-Url: https://gitweb.dragonflybsd.org/dragonfly.git/commitdiff_plain/19bfc8aba97a2cdc7b0d822266cdb5ae5d2e9a80

kernel - Fix MP refcount race in struct pargs

* Protect p->p_args with p->p_token.

* Protect pargs refcounts with sys/refcount.h.

* This fixes at least one memory corruption bug during heavy fork/exec/exit
  testing with fastbulk.

Testing-by: dillon, tuxillo
---

diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c
index 08e9a10e5f..e976f2f2a3 100644
--- a/sys/kern/kern_exec.c
+++ b/sys/kern/kern_exec.c
@@ -72,6 +72,7 @@
 #include <sys/user.h>
 #include <sys/reg.h>
 
+#include <sys/refcount.h>
 #include <sys/thread2.h>
 #include <sys/mplock2.h>
 
@@ -183,6 +184,7 @@ kern_execve(struct nlookupdata *nd, struct image_args *args)
 	struct lwp *lp = td->td_lwp;
 	struct proc *p = td->td_proc;
 	register_t *stack_base;
+	struct pargs *pa;
 	int error, len, i;
 	struct image_params image_params, *imgp;
 	struct vattr attr;
@@ -194,6 +196,7 @@ kern_execve(struct nlookupdata *nd, struct image_args *args)
 	}
 
 	KKASSERT(p);
+	lwkt_gettoken(&p->p_token);
 	imgp = &image_params;
 
 	/*
@@ -485,19 +488,27 @@ interpret:
 	/* Set the access time on the vnode */
 	vn_mark_atime(imgp->vp, td);
 
-	/* Free any previous argument cache */
-	if (p->p_args && --p->p_args->ar_ref == 0)
-		FREE(p->p_args, M_PARGS);
+	/*
+	 * Free any previous argument cache
+	 */
+	pa = p->p_args;
 	p->p_args = NULL;
+	if (pa && refcount_release(&pa->ar_ref)) {
+		kfree(pa, M_PARGS);
+		pa = NULL;
+	}
 
-	/* Cache arguments if they fit inside our allowance */
+	/*
+	 * Cache arguments if they fit inside our allowance
+	 */
 	i = imgp->args->begin_envv - imgp->args->begin_argv;
-	if (ps_arg_cache_limit >= i + sizeof(struct pargs)) {
-		MALLOC(p->p_args, struct pargs *, sizeof(struct pargs) + i, 
-		    M_PARGS, M_WAITOK);
-		p->p_args->ar_ref = 1;
-		p->p_args->ar_length = i;
-		bcopy(imgp->args->begin_argv, p->p_args->ar_args, i);
+	if (sizeof(struct pargs) + i <= ps_arg_cache_limit) {
+		pa = kmalloc(sizeof(struct pargs) + i, M_PARGS, M_WAITOK);
+		refcount_init(&pa->ar_ref, 1);
+		pa->ar_length = i;
+		bcopy(imgp->args->begin_argv, pa->ar_args, i);
+		KKASSERT(p->p_args == NULL);
+		p->p_args = pa;
 	}
 
 exec_fail_dealloc:
@@ -515,6 +526,7 @@ exec_fail_dealloc:
 
 	if (error == 0) {
 		++mycpu->gd_cnt.v_exec;
+		lwkt_reltoken(&p->p_token);
 		return (0);
 	}
 
@@ -527,6 +539,7 @@ exec_fail:
 	 */
 	if (imgp->vmspace_destroyed & 2)
 		p->p_flag &= ~P_INEXEC;
+	lwkt_reltoken(&p->p_token);
 	if (imgp->vmspace_destroyed) {
 		/*
 		 * Sorry, no more process anymore. exit gracefully.
diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index dfff659aaf..f258926840 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -65,6 +65,7 @@
 #include <vm/vm_extern.h>
 
 #include <sys/vmmeter.h>
+#include <sys/refcount.h>
 #include <sys/thread2.h>
 #include <sys/signal2.h>
 #include <sys/spinlock2.h>
@@ -402,7 +403,7 @@ fork1(struct lwp *lp1, int flags, struct proc **procp)
 		p2->p_flag |= P_JAILED;
 
 	if (p2->p_args)
-		p2->p_args->ar_ref++;
+		refcount_acquire(&p2->p_args->ar_ref);
 
 	p2->p_usched = p1->p_usched;
 	/* XXX: verify copy of the secondary iosched stuff */
diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c
index 71b2d0bf34..29c3b015f6 100644
--- a/sys/kern/kern_proc.c
+++ b/sys/kern/kern_proc.c
@@ -1002,6 +1002,7 @@ sysctl_kern_proc_args(SYSCTL_HANDLER_ARGS)
 	int *name = (int*) arg1;
 	u_int namelen = arg2;
 	struct proc *p;
+	struct pargs *opa;
 	struct pargs *pa;
 	int error = 0;
 	struct ucred *cr1 = curproc->p_ucred;
@@ -1009,10 +1010,11 @@ sysctl_kern_proc_args(SYSCTL_HANDLER_ARGS)
 	if (namelen != 1) 
 		return (EINVAL);
 
-	lwkt_gettoken(&proc_token);
 	p = pfindn((pid_t)name[0]);
 	if (p == NULL)
-		goto done;
+		goto done2;
+	lwkt_gettoken(&p->p_token);
+	PHOLD(p);
 
 	if ((!ps_argsopen) && p_trespass(cr1, p->p_ucred))
 		goto done;
@@ -1021,38 +1023,37 @@ sysctl_kern_proc_args(SYSCTL_HANDLER_ARGS)
 		error = EPERM;
 		goto done;
 	}
-
-	PHOLD(p);
 	if (req->oldptr && p->p_args != NULL) {
 		error = SYSCTL_OUT(req, p->p_args->ar_args,
 				   p->p_args->ar_length);
 	}
-	if (req->newptr == NULL) {
-		PRELE(p);
+	if (req->newptr == NULL)
 		goto done;
-	}
-
-	if (p->p_args && --p->p_args->ar_ref == 0) 
-		FREE(p->p_args, M_PARGS);
-	p->p_args = NULL;
 
 	if (req->newlen + sizeof(struct pargs) > ps_arg_cache_limit) {
-		PRELE(p);
 		goto done;
 	}
 
-	MALLOC(pa, struct pargs *, sizeof(struct pargs) + req->newlen, 
-	       M_PARGS, M_WAITOK);
-	pa->ar_ref = 1;
+	pa = kmalloc(sizeof(struct pargs) + req->newlen, M_PARGS, M_WAITOK);
+	refcount_init(&pa->ar_ref, 1);
 	pa->ar_length = req->newlen;
 	error = SYSCTL_IN(req, pa->ar_args, req->newlen);
-	if (!error)
-		p->p_args = pa;
-	else
-		FREE(pa, M_PARGS);
-	PRELE(p);
+	if (error) {
+		kfree(pa, M_PARGS);
+		goto done;
+	}
+
+	opa = p->p_args;
+	p->p_args = pa;
+
+	KKASSERT(opa->ar_ref > 0);
+	if (refcount_release(&opa->ar_ref)) {
+		kfree(opa, M_PARGS);
+	}
 done:
-	lwkt_reltoken(&proc_token);
+	PRELE(p);
+	lwkt_reltoken(&p->p_token);
+done2:
 	return (error);
 }