kernel - Fix race between procfs / proc sysctls and exec, refactor PHOLD/etc
authorMatthew Dillon <dillon@apollo.backplane.com>
Thu, 1 Dec 2011 04:29:15 +0000 (20:29 -0800)
committerMatthew Dillon <dillon@apollo.backplane.com>
Thu, 1 Dec 2011 04:29:15 +0000 (20:29 -0800)
* During a [v]fork/exec sequence the exec will replace the VM space of the
  target process.  A concurrent 'ps' operation could access the target
  process's vmspace as it was being ripped out, resulting in memory
  corruption.

* The P_INEXEC test in procfs was insufficient, the exec code itself must
  also wait for procfs's PHOLD() on the process to go away before it can
  proceed.  This should properly interlock the entire operation.

* Can occur with procfs or non-procfs ps's (via proc sysctls).

* Possibly related to the seg-fault issue we have where the user stack gets
  corrupted.

* Also revamp PHOLD()/PRELE() and add PSTALL(), changing all manual while()
  loops waiting on p->p_lock to use PSTALL().

  These functions now integrate a wakeup request flag into p->p_lock
  using atomic ops and no longer tsleep() for 1 tick (or hz ticks, or
  whatever).  Wakeups are issued proactively.

sys/kern/kern_exec.c
sys/kern/kern_exit.c
sys/kern/kern_proc.c
sys/sys/proc.h
sys/vfs/procfs/procfs_mem.c

index ed3be80..1505cdc 100644 (file)
@@ -761,6 +761,18 @@ exec_new_vmspace(struct image_params *imgp, struct vmspace *vmcopy)
        p->p_flags |= P_INEXEC;
 
        /*
+        * After setting P_INEXEC wait for any remaining references to
+        * the process (p) to go away.
+        *
+        * In particular, a vfork/exec sequence will replace p->p_vmspace
+        * and we must interlock anyone trying to access the space (aka
+        * procfs or sys_process.c calling procfs_domem()).
+        *
+        * If P_PPWAIT is set the parent vfork()'d and has a PHOLD() on us.
+        */
+       PSTALL(p, "exec1", ((p->p_flags & P_PPWAIT) ? 1 : 0));
+
+       /*
         * Blow away entire process VM, if address space not shared,
         * otherwise, create a new VM space so that other threads are
         * not disrupted.  If we are execing a resident vmspace we
index bcf7f64..df1114c 100644 (file)
@@ -347,11 +347,7 @@ exit1(int rv)
 
        if (p->p_flags & P_PROFIL)
                stopprofclock(p);
-       /*
-        * If parent is waiting for us to exit or exec,
-        * P_PPWAIT is set; we will wakeup the parent below.
-        */
-       p->p_flags &= ~(P_TRACED | P_PPWAIT);
+
        SIGEMPTYSET(p->p_siglist);
        SIGEMPTYSET(lp->lwp_siglist);
        if (timevalisset(&p->p_realtimer.it_value))
@@ -480,6 +476,15 @@ exit1(int rv)
                cache_drop(&p->p_textnch);
 
        /*
+        * We have to handle PPWAIT here or proc_move_allproc_zombie()
+        * will block on the PHOLD() the parent is doing.
+        */
+       if (p->p_flags & P_PPWAIT) {
+               p->p_flags &= ~P_PPWAIT;
+               wakeup(p->p_pptr);
+       }
+
+       /*
         * Move the process to the zombie list.  This will block
         * until the process p_lock count reaches 0.  The process will
         * not be reaped until TDF_EXITING is set by cpu_thread_exit(),
@@ -557,7 +562,10 @@ exit1(int rv)
        } else {
                ksignal(q, SIGCHLD);
        }
+
+       p->p_flags &= ~P_TRACED;
        wakeup(p->p_pptr);
+
        PRELE(q);
        /* lwkt_reltoken(&proc_token); */
        /* NOTE: p->p_pptr can get ripped out */
@@ -903,8 +911,7 @@ loop:
                         * put a hold on the process for short periods of
                         * time.
                         */
-                       while (p->p_lock)
-                               tsleep(p, 0, "reap3", hz);
+                       PSTALL(p, "reap3", 0);
 
                        /* Take care of our return values. */
                        *res = p->p_pid;
@@ -978,11 +985,9 @@ loop:
                         * holders to go away (so the vmspace remains stable),
                         * then scrap it.
                         */
-                       while (p->p_lock)
-                               tsleep(p, 0, "reap4", hz);
+                       PSTALL(p, "reap4", 0);
                        vmspace_exitfree(p);
-                       while (p->p_lock)
-                               tsleep(p, 0, "reap5", hz);
+                       PSTALL(p, "reap5", 0);
 
                        kfree(p, M_PROC);
                        atomic_add_int(&nprocs, -1);
index 871a9b9..4ab6a16 100644 (file)
@@ -141,6 +141,91 @@ procinit(void)
 }
 
 /*
+ * Process hold/release support functions.  These functions must be MPSAFE.
+ * Called via the PHOLD(), PRELE(), and PSTALL() macros.
+ *
+ * p->p_lock is a simple hold count with a waiting interlock.  No wakeup()
+ * is issued unless someone is actually waiting for the process.
+ *
+ * Most holds are short-term, allowing a process scan or other similar
+ * operation to access a proc structure without it getting ripped out from
+ * under us.  procfs and process-list sysctl ops also use the hold function
+ * interlocked with various p_flags to keep the vmspace intact when reading
+ * or writing a user process's address space.
+ *
+ * There are two situations where a hold count can be longer.  Exiting lwps
+ * hold the process until the lwp is reaped, and the parent will hold the
+ * child during vfork()/exec() sequences while the child is marked P_PPWAIT.
+ *
+ * The kernel waits for the hold count to drop to 0 (or 1 in some cases) at
+ * various critical points in the fork/exec and exit paths before proceeding.
+ */
+#define PLOCK_WAITING  0x40000000
+#define PLOCK_MASK     0x3FFFFFFF
+
+void
+pstall(struct proc *p, const char *wmesg, int count)
+{
+       int o;
+       int n;
+
+       for (;;) {
+               o = p->p_lock;
+               cpu_ccfence();
+               if ((o & PLOCK_MASK) <= count)
+                       break;
+               n = o | PLOCK_WAITING;
+               tsleep_interlock(&p->p_lock, 0);
+               if (atomic_cmpset_int(&p->p_lock, o, n)) {
+                       tsleep(&p->p_lock, PINTERLOCKED, wmesg, 0);
+               }
+       }
+}
+
+void
+phold(struct proc *p)
+{
+       int o;
+       int n;
+
+       for (;;) {
+               o = p->p_lock;
+               cpu_ccfence();
+               n = o + 1;
+               if (atomic_cmpset_int(&p->p_lock, o, n))
+                       break;
+       }
+}
+
+void
+prele(struct proc *p)
+{
+       int o;
+       int n;
+
+       /*
+        * Fast path
+        */
+       if (atomic_cmpset_int(&p->p_lock, 1, 0))
+               return;
+
+       /*
+        * Slow path
+        */
+       for (;;) {
+               o = p->p_lock;
+               KKASSERT((o & PLOCK_MASK) > 0);
+               cpu_ccfence();
+               n = (o - 1) & ~PLOCK_WAITING;
+               if (atomic_cmpset_int(&p->p_lock, o, n)) {
+                       if (o & PLOCK_WAITING)
+                               wakeup(&p->p_lock);
+                       break;
+               }
+       }
+}
+
+/*
  * Is p an inferior of the current process?
  *
  * No requirements.
@@ -642,9 +727,7 @@ void
 proc_move_allproc_zombie(struct proc *p)
 {
        lwkt_gettoken(&proc_token);
-       while (p->p_lock) {
-               tsleep(p, 0, "reap1", hz / 10);
-       }
+       PSTALL(p, "reap1", 0);
        LIST_REMOVE(p, p_list);
        LIST_INSERT_HEAD(&zombproc, p, p_list);
        LIST_REMOVE(p, p_hash);
@@ -666,9 +749,7 @@ void
 proc_remove_zombie(struct proc *p)
 {
        lwkt_gettoken(&proc_token);
-       while (p->p_lock) {
-               tsleep(p, 0, "reap2", hz / 10);
-       }
+       PSTALL(p, "reap2", 0);
        LIST_REMOVE(p, p_list); /* off zombproc */
        LIST_REMOVE(p, p_sibling);
        lwkt_reltoken(&proc_token);
index 6f4fddf..9ae50ce 100644 (file)
@@ -448,8 +448,10 @@ extern void stopevent(struct proc*, unsigned int, unsigned int);
  *
  * MPSAFE
  */
-#define PHOLD(p)       atomic_add_int(&(p)->p_lock, 1)
-#define PRELE(p)       atomic_add_int(&(p)->p_lock, -1)
+#define PHOLD(p)       phold((p))
+#define PRELE(p)       prele((p))
+#define PSTALL(p, msg, n) \
+       do { if ((p)->p_lock > (n)) pstall((p), (msg), (n)); } while (0)
 
 /*
  * Hold lwp in memory, don't destruct, normally for ptrace/procfs work
@@ -549,6 +551,9 @@ void        cpu_thread_wait (struct thread *);
 void   setsugid (void);
 void   faultin (struct proc *p);
 void   swapin_request (void);
+void   phold (struct proc *);
+void   prele (struct proc *);
+void   pstall (struct proc *, const char *, int);
 
 u_int32_t      procrunnable (void);
 
index 1379be1..2232c20 100644 (file)
@@ -92,7 +92,8 @@ procfs_rwmem(struct proc *curp, struct proc *p, struct uio *uio)
        vm = p->p_vmspace;
        if (p->p_stat == SIDL || p->p_stat == SZOMB)
                return EFAULT;
-       if ((p->p_flags & P_WEXIT) || sysref_isinactive(&vm->vm_sysref))
+       if ((p->p_flags & (P_WEXIT | P_INEXEC)) ||
+           sysref_isinactive(&vm->vm_sysref))
                return EFAULT;
 
        /*