kernel - Add requires p->p_token locking and holds around fork()'s child proc
authorMatthew Dillon <dillon@apollo.backplane.com>
Thu, 1 Dec 2011 01:24:11 +0000 (17:24 -0800)
committerMatthew Dillon <dillon@apollo.backplane.com>
Thu, 1 Dec 2011 01:24:11 +0000 (17:24 -0800)
* fork() and vfork() allocate a new process, p2, initialize, and add it to
  the allproc list as well as other lists.

* These functions failed to acquire p2's token, p2 becomes visible to the
  rest of the system when it's added to the allproc list.  Even though p2's
  state is set to SIDL, this is insufficient protection.

  Acquire the token prior to adding p2 to allproc and keep holding the token
  until after we have finished initializing p2.

* We must also PHOLD()/PRELE() p2 around the start_forked_proc() call
  to prevent it from getting ripped out from under us (if it exits
  quickly and/or detaches itself from its parent).

* Possibly fixes the random seg-faulting issue we've seen under very heavy
  fork/exec (parallel compile) loads on the 48-core monster.

sys/kern/kern_fork.c

index f5c8f28..cf69eb3 100644 (file)
@@ -120,9 +120,11 @@ sys_fork(struct fork_args *uap)
 
        error = fork1(lp, RFFDG | RFPROC | RFPGLOCK, &p2);
        if (error == 0) {
+               PHOLD(p2);
                start_forked_proc(lp, p2);
                uap->sysmsg_fds[0] = p2->p_pid;
                uap->sysmsg_fds[1] = 0;
+               PRELE(p2);
        }
        return error;
 }
@@ -139,9 +141,11 @@ sys_vfork(struct vfork_args *uap)
 
        error = fork1(lp, RFFDG | RFPROC | RFPPWAIT | RFMEM | RFPGLOCK, &p2);
        if (error == 0) {
+               PHOLD(p2);
                start_forked_proc(lp, p2);
                uap->sysmsg_fds[0] = p2->p_pid;
                uap->sysmsg_fds[1] = 0;
+               PRELE(p2);
        }
        return error;
 }
@@ -171,10 +175,16 @@ sys_rfork(struct rfork_args *uap)
 
        error = fork1(lp, uap->flags | RFPGLOCK, &p2);
        if (error == 0) {
-               if (p2)
+               if (p2) {
+                       PHOLD(p2);
                        start_forked_proc(lp, p2);
-               uap->sysmsg_fds[0] = p2 ? p2->p_pid : 0;
-               uap->sysmsg_fds[1] = 0;
+                       uap->sysmsg_fds[0] = p2->p_pid;
+                       uap->sysmsg_fds[1] = 0;
+                       PRELE(p2);
+               } else {
+                       uap->sysmsg_fds[0] = 0;
+                       uap->sysmsg_fds[1] = 0;
+               }
        }
        return error;
 }
@@ -241,7 +251,8 @@ int
 fork1(struct lwp *lp1, int flags, struct proc **procp)
 {
        struct proc *p1 = lp1->lwp_proc;
-       struct proc *p2, *pptr;
+       struct proc *p2;
+       struct proc *pptr;
        struct pgrp *p1grp;
        struct pgrp *plkgrp;
        uid_t uid;
@@ -256,6 +267,7 @@ fork1(struct lwp *lp1, int flags, struct proc **procp)
 
        lwkt_gettoken(&p1->p_token);
        plkgrp = NULL;
+       p2 = NULL;
 
        /*
         * Here we don't create a new process, but we divorce
@@ -363,39 +375,48 @@ fork1(struct lwp *lp1, int flags, struct proc **procp)
                goto done;
        }
 
-       /* Allocate new proc. */
+       /*
+        * Allocate a new process, don't get fancy: zero the structure.
+        */
        p2 = kmalloc(sizeof(struct proc), M_PROC, M_WAITOK|M_ZERO);
 
        /*
-        * Setup linkage for kernel based threading XXX lwp
+        * Core initialization.  SIDL is a safety state that protects the
+        * partially initialized process once it starts getting hooked
+        * into system structures and becomes addressable.
+        *
+        * We must be sure to acquire p2->p_token as well, we must hold it
+        * once the process is on the allproc list to avoid things such
+        * as competing modifications to p_flags.
         */
-       if (flags & RFTHREAD) {
-               p2->p_peers = p1->p_peers;
-               p1->p_peers = p2;
-               p2->p_leader = p1->p_leader;
-       } else {
-               p2->p_leader = p2;
-       }
+       p2->p_lasttid = -1;     /* first tid will be 0 */
+       p2->p_stat = SIDL;
 
        RB_INIT(&p2->p_lwp_tree);
        spin_init(&p2->p_spin);
        lwkt_token_init(&p2->p_token, "proc");
-       p2->p_lasttid = -1;     /* first tid will be 0 */
+       lwkt_gettoken(&p2->p_token);
 
        /*
-        * Setting the state to SIDL protects the partially initialized
-        * process once it starts getting hooked into the rest of the system.
+        * Setup linkage for kernel based threading XXX lwp.  Also add the
+        * process to the allproclist.
+        *
+        * The process structure is addressable after this point.
         */
-       p2->p_stat = SIDL;
+       if (flags & RFTHREAD) {
+               p2->p_peers = p1->p_peers;
+               p1->p_peers = p2;
+               p2->p_leader = p1->p_leader;
+       } else {
+               p2->p_leader = p2;
+       }
        proc_add_allproc(p2);
 
        /*
-        * Make a proc table entry for the new process.
-        * The whole structure was zeroed above, so copy the section that is
-        * copied directly from the parent.
+        * Initialize the section which is copied verbatim from the parent.
         */
        bcopy(&p1->p_startcopy, &p2->p_startcopy,
-           (unsigned) ((caddr_t)&p2->p_endcopy - (caddr_t)&p2->p_startcopy));
+             ((caddr_t)&p2->p_endcopy - (caddr_t)&p2->p_startcopy));
 
        /*
         * Duplicate sub-structures as needed.  Increase reference counts
@@ -458,11 +479,8 @@ fork1(struct lwp *lp1, int flags, struct proc **procp)
        } else {
                p2->p_fd = fdshare(p1);
                if (p1->p_fdtol == NULL) {
-                       lwkt_gettoken(&p1->p_token);
-                       p1->p_fdtol =
-                               filedesc_to_leader_alloc(NULL,
-                                                        p1->p_leader);
-                       lwkt_reltoken(&p1->p_token);
+                       p1->p_fdtol = filedesc_to_leader_alloc(NULL,
+                                                              p1->p_leader);
                }
                if ((flags & RFTHREAD) != 0) {
                        /*
@@ -487,7 +505,7 @@ fork1(struct lwp *lp1, int flags, struct proc **procp)
         * been preserved.
         */
        p2->p_flags |= p1->p_flags & P_SUGID;
-       if (p1->p_session->s_ttyvp != NULL && p1->p_flags & P_CONTROLT)
+       if (p1->p_session->s_ttyvp != NULL && (p1->p_flags & P_CONTROLT))
                p2->p_flags |= P_CONTROLT;
        if (flags & RFPPWAIT)
                p2->p_flags |= P_PPWAIT;
@@ -562,16 +580,20 @@ fork1(struct lwp *lp1, int flags, struct proc **procp)
 
        if (flags == (RFFDG | RFPROC | RFPGLOCK)) {
                mycpu->gd_cnt.v_forks++;
-               mycpu->gd_cnt.v_forkpages += p2->p_vmspace->vm_dsize + p2->p_vmspace->vm_ssize;
+               mycpu->gd_cnt.v_forkpages += p2->p_vmspace->vm_dsize +
+                                            p2->p_vmspace->vm_ssize;
        } else if (flags == (RFFDG | RFPROC | RFPPWAIT | RFMEM | RFPGLOCK)) {
                mycpu->gd_cnt.v_vforks++;
-               mycpu->gd_cnt.v_vforkpages += p2->p_vmspace->vm_dsize + p2->p_vmspace->vm_ssize;
+               mycpu->gd_cnt.v_vforkpages += p2->p_vmspace->vm_dsize +
+                                             p2->p_vmspace->vm_ssize;
        } else if (p1 == &proc0) {
                mycpu->gd_cnt.v_kthreads++;
-               mycpu->gd_cnt.v_kthreadpages += p2->p_vmspace->vm_dsize + p2->p_vmspace->vm_ssize;
+               mycpu->gd_cnt.v_kthreadpages += p2->p_vmspace->vm_dsize +
+                                               p2->p_vmspace->vm_ssize;
        } else {
                mycpu->gd_cnt.v_rforks++;
-               mycpu->gd_cnt.v_rforkpages += p2->p_vmspace->vm_dsize + p2->p_vmspace->vm_ssize;
+               mycpu->gd_cnt.v_rforkpages += p2->p_vmspace->vm_dsize +
+                                             p2->p_vmspace->vm_ssize;
        }
 
        /*
@@ -601,6 +623,8 @@ fork1(struct lwp *lp1, int flags, struct proc **procp)
        *procp = p2;
        error = 0;
 done:
+       if (p2)
+               lwkt_reltoken(&p2->p_token);
        lwkt_reltoken(&p1->p_token);
        if (plkgrp) {
                lockmgr(&plkgrp->pg_lock, LK_RELEASE);
@@ -724,6 +748,8 @@ rm_at_fork(forklist_fn function)
 /*
  * Add a forked process to the run queue after any remaining setup, such
  * as setting the fork handler, has been completed.
+ *
+ * p2 is held by the caller.
  */
 void
 start_forked_proc(struct lwp *lp1, struct proc *p2)