Date: Mon, 11 Feb 2002 19:03:32 -0800 From: Terry Lambert <tlambert2@mindspring.com> To: John Baldwin <jhb@FreeBSD.org> Cc: current@freebsd.org, bde@freebsd.org, Alfred Perlstein <bright@mu.org>, Julian Elischer <julian@elischer.org> Subject: Re: ucred holding patch, BDE version Message-ID: <3C688604.75C0E136@mindspring.com> References: <XFMail.020211204440.jhb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
John Baldwin wrote:
> > The "multiple threads" argument is bogus, since the calls
> > to [gs]et[ug]id() are on a per process, not a per thread
> > basis.
>
> Thread 1 makes a syscall that blocks. Say it blocks in one of 3 VOP's all of
> which need a credential. Thread 2 changes the credentials of the process.
> Thread 1 now resumes assuming that say a VADMIN or VACCESS suceeded with the
> old cred that may not be valid any longer and performs VOP's with the now newer
> credential (which it may even read a stale value of w/o a lock thus using some
> random memory for the creds) to do its other VOP's which may now fail weirdly.
Then the combination of calls fails. There is no case where
it will succeed when previously it would not. I would argue
that the failure is a lost race condition that exists in the
code anyway, and that this approach merely makes the race
failure more reliably reproducible. 8-).
I think you could "fix" this case, in any event, by saving a
reference to the credential at the start (or, better, as a
result of the sleep, when you know it's going to happen),
any time you are going to split a system call across multiple
potential blocking operations. That certainly is *NOT* as
common to all system calls as this discussion implies.
> The idea of per-thread ucred references is so that a thread will have the same
> credentials for the lifetime of a syscall so that the entire syscall is self
> consistent. It also means that except for when we update the ucred reference,
> we don't need locks to access thread credentials since the thread references
> are to read-only credentials. We discussed this on -arch _months_ ago and
> everyone agreed with it then.
As long as the pointer updates are atomic, you don't need to
hold a lock to reference it anyway. You don't care over the
update, since it's a drop of priviledge.
> > Perhaps this dicsussion is enough impetus to justify
> > revisiting the atomic_t type definitions, which would
> > be useful as reference counted hold/release mechanisms
> > that would obviate the need for locks here? This would
> > at least let you defer getting rid of the per thread
> > cred instances until later.
>
> All having the refcount_t and other refcount_* functions would do is let us get
> rid of the per-ucred mutex (actually a pool mutex, so the overhead per ucred is
> just a pointer right now). It wouln't change the fact that we need a lock to
> make sure p_ucred is up to date before we read a value we need to depend on or
> actually use.
Not true. You can take a reference to it, and the reference
is guaranteed to not change out from under you, so long as
it is counted. Use of a stack variable to store the value
over the count increment allows a compare after the increment,
after which a retry is possible (if there was a race during
the reference taking). Thus there is no need for a lock, e.g.:
cred *
addreftocred(cred **cpp)
{
cred *rcp; /* stack variable */
again:
rcp = *cp;
atomic_plusplus( rcp->count); /* magic atomic crap */
if( rcp != *cp) {
atomic_mimusminus( rcp->count);
goto again;
}
return( rcp);
}
This depends on the rarity of credential changes; in the
degenerate case, where there is a credential change between
each normal system call that is not a credential change, the
overhead is immense. But people who write code like that
can be safely punished without fear. 8-).
-- Terry
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?3C688604.75C0E136>