[src/trunk]: src/sys posix_spawn:

[rmind <rmind%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/3ac633ac8008
branches:  trunk
changeset: 779084:3ac633ac8008
user:      rmind <rmind%NetBSD.org@localhost>
date:      Mon Apr 30 21:19:58 2012 +0000

description:
posix_spawn:
- Remove copy-pasting in error paths, use execve_free_{vmspace,data}().
- Move some code (both in the init and exit paths) out of the locks.
- Slightly simplify do_posix_spawn() callers.
- Add few asserts and comments.

diffstat:

 sys/compat/netbsd32/netbsd32_execve.c |   30 +-
 sys/kern/kern_exec.c                  |  314 +++++++++++++++------------------
 sys/sys/exec.h                        |    4 +-
 sys/sys/spawn.h                       |   10 +-
 4 files changed, 160 insertions(+), 198 deletions(-)

diffs (truncated from 773 to 300 lines):

diff -r 658909ecb634 -r 3ac633ac8008 sys/compat/netbsd32/netbsd32_execve.c
--- a/sys/compat/netbsd32/netbsd32_execve.c     Mon Apr 30 20:41:33 2012 +0000
+++ b/sys/compat/netbsd32/netbsd32_execve.c     Mon Apr 30 21:19:58 2012 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: netbsd32_execve.c,v 1.34 2012/04/08 11:27:44 martin Exp $      */
+/*     $NetBSD: netbsd32_execve.c,v 1.35 2012/04/30 21:19:58 rmind Exp $       */
 
 /*
  * Copyright (c) 1998, 2001 Matthew R. Green
@@ -28,7 +28,7 @@
 
 #include <sys/cdefs.h>
 
-__KERNEL_RCSID(0, "$NetBSD: netbsd32_execve.c,v 1.34 2012/04/08 11:27:44 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: netbsd32_execve.c,v 1.35 2012/04/30 21:19:58 rmind Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -178,7 +178,6 @@
        struct posix_spawn_file_actions *fa = NULL;
        struct posix_spawnattr *sa = NULL;
        pid_t pid;
-       bool child_ok = false;
 
        error = check_posix_spawn(l);
        if (error) {
@@ -191,7 +190,7 @@
                error = netbsd32_posix_spawn_fa_alloc(&fa,
                    SCARG_P32(uap, file_actions));
                if (error)
-                       goto error_exit;
+                       goto fail;
        }
 
        /* copyin posix_spawnattr struct */
@@ -199,17 +198,17 @@
                sa = kmem_alloc(sizeof(*sa), KM_SLEEP);
                error = copyin(SCARG_P32(uap, attrp), sa, sizeof(*sa));
                if (error)
-                       goto error_exit;
+                       goto fail;
        }
 
        /*
         * Do the spawn
         */
-       error = do_posix_spawn(l, &pid, &child_ok, SCARG_P32(uap, path), fa,
+       error = do_posix_spawn(l, &pid, SCARG_P32(uap, path), fa,
            sa, SCARG_P32(uap, argv), SCARG_P32(uap, envp),
            netbsd32_execve_fetch_element);
        if (error)
-               goto error_exit;
+               goto fail;
 
        if (error == 0 && SCARG_P32(uap, pid) != NULL)
                error = copyout(&pid, SCARG_P32(uap, pid), sizeof(pid));
@@ -217,17 +216,14 @@
        *retval = error;
        return 0;
 
- error_exit:
-       if (!child_ok) {
-               (void)chgproccnt(kauth_cred_getuid(l->l_cred), -1);
-               atomic_dec_uint(&nprocs);
+fail:
+       (void)chgproccnt(kauth_cred_getuid(l->l_cred), -1);
+       atomic_dec_uint(&nprocs);
 
-               if (sa)
-                       kmem_free(sa, sizeof(*sa));
-               if (fa)
-                       posix_spawn_fa_free(fa, fa->len);
-       }
-
+       if (sa)
+               kmem_free(sa, sizeof(*sa));
+       if (fa)
+               posix_spawn_fa_free(fa, fa->len);
        *retval = error;
        return 0;
 }
diff -r 658909ecb634 -r 3ac633ac8008 sys/kern/kern_exec.c
--- a/sys/kern/kern_exec.c      Mon Apr 30 20:41:33 2012 +0000
+++ b/sys/kern/kern_exec.c      Mon Apr 30 21:19:58 2012 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: kern_exec.c,v 1.350 2012/04/15 15:35:00 martin Exp $   */
+/*     $NetBSD: kern_exec.c,v 1.351 2012/04/30 21:19:58 rmind Exp $    */
 
 /*-
  * Copyright (c) 2008 The NetBSD Foundation, Inc.
@@ -59,7 +59,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_exec.c,v 1.350 2012/04/15 15:35:00 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_exec.c,v 1.351 2012/04/30 21:19:58 rmind Exp $");
 
 #include "opt_exec.h"
 #include "opt_ktrace.h"
@@ -211,9 +211,8 @@
  * Exec lock. Used to control access to execsw[] structures.
  * This must not be static so that netbsd32 can access it, too.
  */
-krwlock_t exec_lock;
-
-static kmutex_t sigobject_lock;
+krwlock_t              exec_lock       __cacheline_aligned;
+static kmutex_t                sigobject_lock  __cacheline_aligned;
 
 /*
  * Data used between a loadvm and execve part of an "exec" operation
@@ -296,7 +295,6 @@
  *                     exec header unmodified.
  */
 int
-/*ARGSUSED*/
 check_exec(struct lwp *l, struct exec_package *epp, struct pathbuf *pb)
 {
        int             error, i;
@@ -502,7 +500,7 @@
            SCARG(uap, envp), execve_fetch_element);
 }
 
-int   
+int
 sys_fexecve(struct lwp *l, const struct sys_fexecve_args *uap,
     register_t *retval)
 {
@@ -564,6 +562,43 @@
 #endif
 }
 
+static void
+execve_free_vmspace(struct execve_data *ed)
+{
+
+       /*
+        * Free the vmspace-creation commands and release their references.
+        */
+       kill_vmcmds(&ed->ed_pack.ep_vmcmds);
+
+       /* Kill any opened file descriptor, if necessary. */
+       if (ed->ed_pack.ep_flags & EXEC_HASFD) {
+               ed->ed_pack.ep_flags &= ~EXEC_HASFD;
+               fd_close(ed->ed_pack.ep_fd);
+       }
+
+       /* Close and put the executed file. */
+       vn_lock(ed->ed_pack.ep_vp, LK_EXCLUSIVE | LK_RETRY);
+       VOP_CLOSE(ed->ed_pack.ep_vp, FREAD, curlwp->l_cred);
+       vput(ed->ed_pack.ep_vp);
+       pool_put(&exec_pool, ed->ed_argp);
+}
+
+static void
+execve_free_data(struct execve_data *ed)
+{
+
+       kmem_free(ed->ed_pack.ep_hdr, ed->ed_pack.ep_hdrlen);
+       if (ed->ed_pack.ep_emul_root != NULL)
+               vrele(ed->ed_pack.ep_emul_root);
+       if (ed->ed_pack.ep_interp != NULL)
+               vrele(ed->ed_pack.ep_interp);
+
+       pathbuf_stringcopy_put(ed->ed_pathbuf, ed->ed_pathstring);
+       pathbuf_destroy(ed->ed_pathbuf);
+       PNBUF_PUT(ed->ed_resolvedpathbuf);
+}
+
 static int
 execve_loadvm(struct lwp *l, const char *path, char * const *args,
        char * const *envs, execve_fetch_element_t fetch_element,
@@ -575,11 +610,12 @@
        size_t                  i, len;
        struct exec_fakearg     *tmpfap;
        u_int                   modgen;
+       bool                    freevm;
 
        KASSERT(data != NULL);
 
        p = l->l_proc;
-       modgen = 0;
+       modgen = 0;
 
        SDT_PROBE(proc,,,exec, path, 0, 0, 0, 0);
 
@@ -599,6 +635,7 @@
         * to call exec in order to do something useful.
         */
  retry:
+       freevm = false;
        if (p->p_flag & PK_SUGID) {
                if (kauth_authorize_process(l->l_cred, KAUTH_PROCESS_RLIMIT,
                     p, KAUTH_ARG(KAUTH_REQ_PROCESS_RLIMIT_BYPASS),
@@ -610,14 +647,6 @@
        }
 
        /*
-        * Drain existing references and forbid new ones.  The process
-        * should be left alone until we're done here.  This is necessary
-        * to avoid race conditions - e.g. in ptrace() - that might allow
-        * a local user to illicitly obtain elevated privileges.
-        */
-       rw_enter(&p->p_reflock, RW_WRITER);
-
-       /*
         * Init the namei data to point the file user's program name.
         * This is done here rather than in check_exec(), so that it's
         * possible to override this settings if any of makecmd/probe
@@ -626,9 +655,7 @@
         */
        error = pathbuf_copyin(path, &data->ed_pathbuf);
        if (error) {
-               DPRINTF(("%s: pathbuf_copyin path @%p %d\n", __func__,
-                   path, error));
-               goto clrflg;
+               return error;
        }
        data->ed_pathstring = pathbuf_stringcopy_get(data->ed_pathbuf);
 
@@ -657,6 +684,13 @@
        data->ed_pack.ep_esch = NULL;
        data->ed_pack.ep_pax_flags = 0;
 
+       /*
+        * Drain existing references and forbid new ones.  The process
+        * should be left alone until we're done here.  This is necessary
+        * to avoid race conditions - e.g. in ptrace() - that might allow
+        * a local user to illicitly obtain elevated privileges.
+        */
+       rw_enter(&p->p_reflock, RW_WRITER);
        rw_enter(&exec_lock, RW_READER);
 
        /* see if we can run it. */
@@ -665,8 +699,9 @@
                        DPRINTF(("%s: check exec failed %d\n",
                            __func__, error));
                }
-               goto freehdr;
+               goto bad;
        }
+       freevm = true;
 
        /* XXX -- THE FOLLOWING SECTION NEEDS MAJOR CLEANUP */
 
@@ -802,35 +837,14 @@
 
        return 0;
 
- bad:
-       /* free the vmspace-creation commands, and release their references */
-       kill_vmcmds(&data->ed_pack.ep_vmcmds);
-       /* kill any opened file descriptor, if necessary */
-       if (data->ed_pack.ep_flags & EXEC_HASFD) {
-               data->ed_pack.ep_flags &= ~EXEC_HASFD;
-               fd_close(data->ed_pack.ep_fd);
-       }
-       /* close and put the exec'd file */
-       vn_lock(data->ed_pack.ep_vp, LK_EXCLUSIVE | LK_RETRY);
-       VOP_CLOSE(data->ed_pack.ep_vp, FREAD, l->l_cred);
-       vput(data->ed_pack.ep_vp);
-       pool_put(&exec_pool, data->ed_argp);
+bad:
+       rw_exit(&exec_lock);
+       rw_exit(&p->p_reflock);
 
- freehdr:
-       kmem_free(data->ed_pack.ep_hdr, data->ed_pack.ep_hdrlen);
-       if (data->ed_pack.ep_emul_root != NULL)
-               vrele(data->ed_pack.ep_emul_root);
-       if (data->ed_pack.ep_interp != NULL)
-               vrele(data->ed_pack.ep_interp);
-
-       rw_exit(&exec_lock);
-
-       pathbuf_stringcopy_put(data->ed_pathbuf, data->ed_pathstring);
-       pathbuf_destroy(data->ed_pathbuf);
-       PNBUF_PUT(data->ed_resolvedpathbuf);
-
- clrflg:
-       rw_exit(&p->p_reflock);
+       if (freevm) {
+               execve_free_vmspace(data);
+       }
+       execve_free_data(data);
 
        if (modgen != module_gen && error == ENOEXEC) {
                modgen = module_gen;
@@ -842,41 +856,11 @@
        return error;
 }
 
-static void
-execve_free_data(struct execve_data *data)
-{
-
-       /* free the vmspace-creation commands, and release their references */
-       kill_vmcmds(&data->ed_pack.ep_vmcmds);
-       /* kill any opened file descriptor, if necessary */
-       if (data->ed_pack.ep_flags & EXEC_HASFD) {