Date: Mon, 11 Feb 2002 18:51:06 -0800 From: Terry Lambert <tlambert2@mindspring.com> To: Julian Elischer <julian@elischer.org> Cc: Alfred Perlstein <bright@mu.org>, jhb@freebsd.org, bde@freebsd.org, current@freebsd.org Subject: Re: ucred holding patch, BDE version Message-ID: <3C68831A.12A67EAB@mindspring.com> References: <Pine.BSF.4.21.0202111744150.18316-100000@InterJet.elischer.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Julian Elischer 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. > > there is no such thing as a per process syscall. > Two threads can always do the same syscall at the same time. > there needs to be a proc-lock to stop it from becoming > chaotic in there. In actual fact, since you cannot alter a cred > but only replace that which the process points to it's not > quite that bad, but you need to either lock it or have atomic > reference-counting that can handle the possibility that > the cred could have bee decremented to 0 by another thread just before > you checked it. I would argue that: 1) There are a small number of system calls where this is true. 2) It's worth doing the synchornization in-kernel for the process alone, where this is the case. The problem I have with the crd locking is that each process does not do a gratuitous clone of the active cred before doing its own thing (if you will remember, I suggested this in the context of the cred reference count overflow bug, back when I found the problem last year). The upshot of this is that the lock will stall anyone using that cred. This argues that creds should be, minimally, per process, if not per CPU, instead of shared references smeared all over the place, and locked on each reference, even though it's not possible for a cred to be changed at all out from under you -- only replaced wholesale, since once instanced, a cred may not be changed. Ask John and Robert about the permissability of changing cred contents, and the NFS changes that resulted (fairly) recently. Personally, I think that cred changes are rare enough, even in the degenerate case, that it's worth taking the synchronization event as an intraprocess global IPI, rather than using a lock. > > Personally, I still do not understand the need to have a > > cred reference per thread, the only thing that makes any > > sense about that is to optimize the degenerate case of a > > daemon that makes calls as another ID, on behalf of a lot > > of users (or, sequentially, at least, different users). > > One example of such a program would be SAMBA (but *not* > > NFS, due to "access" semantics on objects based on path > > component access exclusion by credential not being an > > effective mechanism for NFS file handles). > > the cred that is in force at the time that the syscall STARTS > is used for the full syscall otherwise you could have > one cred used for the first part of a syscall and a completely > differnet one used for the secnd part of a syscall. Again: the model permits dropping, but not gaining, of priviledges. THe worst case failure, then, is: 1) A call starts on a thread without proper program level stall synchornization between threads. 2) A subsequent thread drops priviledges, and this drop is unprotected by the stall. 3) The call completes, but fails during completion for lack of privledges present when the call was started, because the drop was not stalled. Frankly, I don't see this being a problem: badly written code with a race condition will occasionally lose the race, rather than being implicitly protected (an assumption that is not portable, if it is depended upon), and the code will end up being fixed by its author. > > I think that you would need to have [gs]et[ug]id() be on a > > per thread basis for this to be an efficiency, and I think > > trying to do this pessimizes everything else. > > > > My gut tells me that creds should be per process, and > > that the references to them should be taken sparingly, > > and then only if a need can be justified, rather than > > "just in case some day". > > creads can only be changed per process but the threads only pick > up the change on next syscall startup. I think this is the fundamental misunderstanding: creds never change. The can only be replaced, and then only with a cred of equal or lesser priviledge. > > Kirk at one time called vnodes "the structure that ate the > > kernel"; he was wrong: it was creds. > > I believe it was Mike Karels. Whatever. It's "creds" now. > > 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. > > I've made that point before and I believe that jhb has said he would like > such primatives. I'm still not convinced that it's necessary to lock what you are trying to lock, but I certainly agree on atomic_t. -- 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?3C68831A.12A67EAB>