Date: Sat, 10 Jun 2000 14:39:58 -0400 (EDT) From: Brian Fundakowski Feldman <green@FreeBSD.org> To: Alfred Perlstein <bright@wintelcom.net> Cc: hackers@FreeBSD.ORG Subject: Re: uidinfo has many race conditions. Message-ID: <Pine.BSF.4.21.0006101431240.68227-100000@green.dyndns.org> In-Reply-To: <20000610110326.R18462@fw.wintelcom.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 10 Jun 2000, Alfred Perlstein wrote: > * Brian Fundakowski Feldman <green@FreeBSD.ORG> [000610 09:13] wrote: > > On Fri, 9 Jun 2000, Alfred Perlstein wrote: > > > > > * Alfred Perlstein <bright@wintelcom.net> [000609 16:45] wrote: > > > > hi, > > > > > > > > Is it just me or does the fact that uidinfo structures (see > > > > kern/kern_proc.c) are allocated with M_WAITOK after finding them > > > > fails and then inserted into the uidhash structure a race condition? > > > > > > > > Index: kern_proc.c > > > > =================================================================== > > > > > > Yes, I know i forgot to put the created ones back into the list, I was > > > just a bit flusteres after reading over the code. I'll have some more > > > later. > > > > With regard to sbsize itself, the test-and-branch conditions do not have > > to be atomic. It really isn't that important. The incrementing does, > > though, and to fix that a very lightweight mutex should be used. How > > about a simplelock? That should work perfectly. > > Well if we get an atomic_t it could be done that way, even if we > fail the race for updating and go beyond our rlimit slightly it's > much cheaper than a spinlock from my PoV. The only problem is > that afaik on some archs atomic_t can be quite small, we'd have > to watch for overflow, perhaps a spinlock is a better idea however > only if the next thing I mention here is realized: An atomic_t can only be as large as the largest generally usable integer register, unless I'm missing something important. That means that an i386 would have a bigger rlim_t than atomic_t. > I'm somewhat upset now that I understand sbsize; every operation > on a socket involves a lookup of the uidinfo and manipulating it. > You could optimize it by having a flag in the socket struct that > says "created with a non-infinity RLIMIT", which means that chgsbsize > needs to be called when the size changes, but for the common case > it's not done. Well, first of all, it's not every operation on a socket. It's specific size changes, creation, and deletion. On ATM there's also a resize on connect(), and PF_LOCAL has pretty stupid send/receive routines that need it; however, performance is really not affected at all even there. > Do you think you can do that? I suppose so. I don't think that would change the behavior (exposed to the users) at all; it's not solving the problem you brought up. > > As for uidinfo itself, I feel it should be done with a mutex over > > uihashtbl. It should be grabbed if the hash for the uid is not > > found, tested again (to see if we lost the race), and allocated. > > I can't see a way to poke holes in that, and it would be quite > > efficient. > > A mutex on each of the hash entrie heads would work, I really don't > like the manual inlining of the lookup and insert functions, > seperating them would be a good idea, it really reduces the complexity > of the code. Yes, they should. The code should have the proper mutexes and be inlineable. Might as well do this now; simplelocks are cheap. > -- > -Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org] > "I have the heart of a child; I keep it in a jar on my desk." -- Brian Fundakowski Feldman \ FreeBSD: The Power to Serve! / green@FreeBSD.org `------------------------------' To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-hackers" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0006101431240.68227-100000>