Date: Thu, 12 Sep 2002 11:17:27 -0400 (EDT) From: John Baldwin <jhb@FreeBSD.org> To: Don Lewis <dl-freebsd@catspoiler.org> Cc: nate@root.org, current@FreeBSD.org, wollman@lcs.mit.edu Subject: Re: Locking problems in exec Message-ID: <XFMail.20020912111727.jhb@FreeBSD.org> In-Reply-To: <200209112208.g8BM8Kwr099002@gw.catspoiler.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11-Sep-2002 Don Lewis wrote:
> On 11 Sep, John Baldwin wrote:
>>
>> On 11-Sep-2002 Don Lewis wrote:
>>> 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.
>>
>> We used to do that but backed it out because it wasn't deemed to be
>> that necessary. If you have a demonstrable problematic race condition
>> that this fixes then it might be a good idea. However, allocating
>> stuff we don't need isn't but so great either.
>
> I haven't observed any problems with the procsig stuff, but it sure hit
> me in the eye when I was scanning the code to see if the fdcheckfd()
> could be done before grabbing the proc lock. The mp_fixme() suggests to
> me that the entire if block is going to be protected by its own lock
> sometime in the future:
>
> PROC_LOCK(p);
> mp_fixme("procsig needs a lock");
This is about adding a lock to the procsig structure itself. The proc
lock does not protect the procsig reference count, etc. The proc lock
just protects the actual p_procsig member of struct proc.
> 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;
> if (p->p_sigacts == &p->p_uarea->u_sigacts)
> panic("shared procsig but private sigacts?");
>
> p->p_uarea->u_sigacts = *p->p_sigacts;
> p->p_sigacts = &p->p_uarea->u_sigacts;
> }
>
>
> We probably won't want to hold the lock across the call to MALLOC(), and
> dropping the lock and possibly blocking, giving ps_refcnt the
> opportunity to change behind our back, doesn't seem wise. Copying a
> shared object and manipulating its reference count while it is unlocked
> doesn't sound wise either. We may be protected in other ways at the
> moment, but this code suggests to me that this won't always be the case.
procsig doesn't have any locks on it yet. At some point it will, but for
now Giant is what handles that case.
>> I think ptrace(2)
>> is not an issue because ptrace(2) won't attach to a P_INEXEC process
>> IIRC.
>
> I think you're correct, but we still can't call fdcheckstd() before we
> know that we are doing the set-id case, and that decision is made after
> we've grabbed the proc lock. Do you think it is reasonable to
> temporarily drop the proc lock for the fdcheckstd() call?
Yes. Between single-threading the process and P_INEXEC most of the
proc-related races in exec() are handled.
--
John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve!" - http://www.FreeBSD.org/
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?XFMail.20020912111727.jhb>