Date: Tue, 10 Sep 2002 23:08:59 -0700 (PDT) From: Don Lewis <dl-freebsd@catspoiler.org> To: nate@root.org Cc: current@FreeBSD.ORG, wollman@lcs.mit.edu Subject: Re: Locking problems in exec Message-ID: <200209110609.g8B68xwr096967@gw.catspoiler.org> In-Reply-To: <200209110328.g8B3Slwr096727@gw.catspoiler.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200209110609.g8B68xwr096967>