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 08e9a10..e976f2f 100644 (file)
--- 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 dfff659..f258926 100644 (file)
--- 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 71b2d0b..29c3b01 100644 (file)
--- 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);
 }