From owner-freebsd-current Mon Feb 11 19:49:35 2002 Delivered-To: freebsd-current@freebsd.org Received: from newman2.bestweb.net (newman2.bestweb.net [209.94.102.67]) by hub.freebsd.org (Postfix) with ESMTP id 20A3B37B67F; Mon, 11 Feb 2002 18:19:27 -0800 (PST) Received: from okeeffe.bestweb.net (okeefe.bestweb.net [209.94.100.110]) by newman2.bestweb.net (Postfix) with ESMTP id 158B323215; Mon, 11 Feb 2002 21:18:20 -0500 (EST) Received: by okeeffe.bestweb.net (Postfix, from userid 0) id 085A59F416; Mon, 11 Feb 2002 21:12:53 -0500 (EST) Date: Mon, 11 Feb 2002 20:44:40 -0500 (EST) From: John Baldwin To: Terry Lambert Subject: Re: ucred holding patch, BDE version Cc: current@freebsd.org, bde@freebsd.org, Message-Id: <20020212021253.085A59F416@okeeffe.bestweb.net> Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On 12-Feb-02 Terry Lambert wrote: > Julian Elischer wrote: >> > This makes little sense to me. >> > >> > Maybe I'm missing something, but by virtue of ownership we don't >> > have to worry about the ucred's refcount on entry into the kernel >> > because it is the owner and no one else is allowed to change our >> > privledges besideds ourselves via set[ug]id(). >> >> multiple threads can do it.. >> >> The proclock is needed to get the reference, >> guarding against other threads, and giant is needed fo rnot to free it >> because if it reaches a refcount of 0 it needs to call free(). (which john >> assures me needs Giant at this time). >> We could avoid the proclock with judicious use of an atomic refcount >> incrementing method. >> >> When Giant goes away it won't be so bad but it will STILL be quicker to >> not drop it across userland. > > 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. 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. > 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. > -- Terry -- John Baldwin <>< 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 To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message