Date: Wed, 13 Dec 2000 10:11:36 -0800 (PST) From: John Baldwin <jhb@FreeBSD.ORG> To: Terry Lambert <tlambert@primenet.com> Cc: arch@FreeBSD.ORG, (Robert Watson) <rwatson@FreeBSD.ORG> Subject: Re: Can !curproc touch Message-ID: <XFMail.001213101136.jhb@FreeBSD.org> In-Reply-To: <200012130921.CAA27456@usr08.primenet.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 13-Dec-00 Terry Lambert wrote: >> Well, currently nothing uses the struct proc mutex when touching >> p_ucred. This is some code from coda: > > [ ... unprotected code ... ] > >> To fix this I could do this: >> >> struct ucred *uc; >> ... >> PROC_LOCK(p); >> uc = p->p_ucred; >> crhold(uc); >> PROC_UNLOCK(p); >> error = venus_root(...., uc, ...); >> crfree(uc); >> >> In place of the last line. However, note that p is always curproc. >> If all the other places that accees p->p_ucred only do so if p == >> curproc, then I don't need to use an explicit lock to protect >> p_ucred, as it will be implicitly locked. Thus, my question is is >> there a place where a process A can read or write the p_ucred of >> process B. If there is, then I need to use the proc mutex to >> protect p_ucred. If not, I can leave p_ucred alone. > > NB: I think you meant to call crcopy() and not crhold(), right? Hmm, actually, yes, I can probably do a uc = crcopy(p_ucred) in place of the assignment and crhold(). However, that operation isn't atomic, so theoretically p_ucred might change out from under me, which would be bad. > Since p_ucred isn't opaque, you really have no choice but to > protect the structure when dereferencing the pointer. Yes. :( > The real problem here is that you haven't fixed the code, though. > > Consider the case of a multithreaded program, where one thread > sleeps in "venus_root", _before_ dereferencing the "uc" you > pass in. Meanwhile, a second thread does a setuid() call, which > destroys the memory pointed to by "p->p_ucred" ... and therefore > the memory pointed to by "uc". A subseqnet dereference of "uc" > in "venus_root" will fail. Er, no. When the second thread does a setuid(), setuid() won't destory the ucred because it will have a refcount > 1, instead it will call crcopy(), which will make a duplicate ucred that will become p_ucred, and decrement the refcount on the original ucred. When venus_root() returns and calls crfree(), then the refcount on the original ucred will drop to 0, and _then_ it will be free'd. > If a dereference and adding a reference are each atomic, AND the > kernel is non-preemptive, you don't need to protect the dereference, > since the aggregate operation will end up being idempotent. If > not, then you need to protect the pointer during the dereference > of the pointer and the adding of the reference to the credential. Well, with SMPng, the kernel is somewhat pre-emptive (for light weight interrupt threads at least), so I think I'll be stuck using the proc lock. -- John Baldwin <jhb@FreeBSD.org> -- http://www.FreeBSD.org/~jhb/ PGP Key: http://www.Baldwin.cx/~john/pgpkey.asc "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?XFMail.001213101136.jhb>