Date: Wed, 13 Dec 2000 09:21:15 +0000 (GMT) From: Terry Lambert <tlambert@primenet.com> To: jhb@FreeBSD.ORG (John Baldwin) Cc: rwatson@FreeBSD.ORG (Robert Watson), arch@FreeBSD.ORG Subject: Re: Can !curproc touch Message-ID: <200012130921.CAA27456@usr08.primenet.com> In-Reply-To: <XFMail.001212162157.jhb@FreeBSD.org> from "John Baldwin" at Dec 12, 2000 04:21:57 PM
next in thread | previous in thread | raw e-mail | index | archive | help
> 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?
Since p_ucred isn't opaque, you really have no choice but to
protect the structure when dereferencing the pointer.
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.
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.
Thus the code becomes either:
struct ucred *uc;
...
PROC_LOCK(p);
uc = ADDREF(p->p_ucred);
PROC_UNLOCK(p);
error = venus_root(...., uc, ...);
UNREF(uc);
Or:
struct ucred *uc;
...
uc = ADDREF(p->p_ucred); /* dereference&increment is idempotent*/
error = venus_root(...., uc, ...);
UNREF(uc);
The first would be required in the INVARIANTS case, with the
suggested simple reference count API that was recently
discussed. Clearly, avoiding this is a good idea, if possible.
Terry Lambert
terry@lambert.org
---
Any opinions in this posting are my own and not those of my present
or previous employers.
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?200012130921.CAA27456>