From owner-freebsd-current Tue Sep 10 23: 9:14 2002 Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.FreeBSD.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 4036A37B400 for ; Tue, 10 Sep 2002 23:09:09 -0700 (PDT) Received: from gw.catspoiler.org (217-ip-163.nccn.net [209.79.217.163]) by mx1.FreeBSD.org (Postfix) with ESMTP id 5E0F843E3B for ; Tue, 10 Sep 2002 23:09:08 -0700 (PDT) (envelope-from dl-freebsd@catspoiler.org) Received: from mousie.catspoiler.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.12.5/8.12.5) with ESMTP id g8B68xwr096967; Tue, 10 Sep 2002 23:09:03 -0700 (PDT) (envelope-from dl-freebsd@catspoiler.org) Message-Id: <200209110609.g8B68xwr096967@gw.catspoiler.org> Date: Tue, 10 Sep 2002 23:08:59 -0700 (PDT) From: Don Lewis Subject: Re: Locking problems in exec To: nate@root.org Cc: current@FreeBSD.ORG, wollman@lcs.mit.edu In-Reply-To: <200209110328.g8B3Slwr096727@gw.catspoiler.org> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On 10 Sep, Don Lewis wrote: > On 10 Sep, Nate Lawson wrote: > >> I'm not sure why fdcheckstd() and setugidsafety() couldn't both happen >> before grabbing the proc lock. Dropping locks in the middle or >> pre-allocating should always be a last resort. > > That is ok as long as there aren't other threads that can mess things up > after fdcheckstd() and setugidsafety() have done their thing. It looks like threads aren't a problem because of the call to thread_single() at the top of execve(). Ptrace() is still a potential problem, so we can't call fdcheckstd() and setugidsafety() until after credential_changing has been calculated, setsugid() has been called and tracing has been disabled, all of which are done after proc lock has been grabbed. It also looks like we should pre-allocate newprocsig instead of temporarily dropping the lock. Index: kern_exec.c =================================================================== RCS file: /home/ncvs/src/sys/kern/kern_exec.c,v retrieving revision 1.189 diff -u -r1.189 kern_exec.c --- kern_exec.c 5 Sep 2002 07:30:14 -0000 1.189 +++ kern_exec.c 11 Sep 2002 05:23:25 -0000 @@ -141,7 +141,7 @@ struct vattr attr; int (*img_first)(struct image_params *); struct pargs *oldargs = NULL, *newargs = NULL; - struct procsig *oldprocsig, *newprocsig; + struct procsig *oldprocsig, *newprocsig = NULL; #ifdef KTRACE struct vnode *tracevp = NULL; #endif @@ -352,6 +352,8 @@ i = imgp->endargs - imgp->stringbase; if (ps_arg_cache_limit >= i + sizeof(struct pargs)) newargs = pargs_alloc(i); + MALLOC(newprocsig, struct procsig *, sizeof(struct procsig), M_SUBPROC, + M_WAITOK); /* close files on exec */ fdcloseexec(td); @@ -369,14 +371,11 @@ mp_fixme("procsig needs a lock"); if (p->p_procsig->ps_refcnt > 1) { oldprocsig = p->p_procsig; - PROC_UNLOCK(p); - MALLOC(newprocsig, struct procsig *, sizeof(struct procsig), - M_SUBPROC, M_WAITOK); bcopy(oldprocsig, newprocsig, sizeof(*newprocsig)); newprocsig->ps_refcnt = 1; oldprocsig->ps_refcnt--; - PROC_LOCK(p); p->p_procsig = newprocsig; + newprocsig = NULL; if (p->p_sigacts == &p->p_uarea->u_sigacts) panic("shared procsig but private sigacts?"); @@ -437,8 +436,15 @@ #endif /* Close any file descriptors 0..2 that reference procfs */ setugidsafety(td); - /* Make sure file descriptors 0..2 are in use. */ + /* + * Make sure file descriptors 0..2 are in use. + * + * fdcheckstd() may call falloc() which may block to + * allocate memory, so temporarily drop the process lock. + */ + PROC_UNLOCK(p); error = fdcheckstd(td); + PROC_LOCK(p); if (error != 0) goto done1; /* @@ -538,6 +544,8 @@ crfree(oldcred); else crfree(newcred); + if (newprocsig != NULL) + free(newprocsig, M_SUBPROC); /* * Handle deferred decrement of ref counts. */ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message