Date: Mon, 26 Mar 2007 23:57:59 -0400 From: Kris Kennaway <kris@obsecurity.org> To: Divacky Roman <xdivac02@stud.fit.vutbr.cz> Cc: hackers@freebsd.org Subject: Re: investigation of Giant usage in kernel Message-ID: <20070327035759.GA47764@xor.obsecurity.org> In-Reply-To: <20070304141136.GA4935@stud.fit.vutbr.cz> References: <20070304141136.GA4935@stud.fit.vutbr.cz>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Mar 04, 2007 at 03:11:36PM +0100, Divacky Roman wrote: > hi > > I looked at where Giant is held in the kernel and I found these interesting things: > > 1) in fs/fifofs/fifo_vnops.c we lock Giant when calling sorecieve()/sosend() > this is a bandaid for fixing a race that doesnt have to exist anymore. ie. > it needs some testing and can be remvoed I have removed this on my test machines and will see if the problem recurs. It is probably not necessary. > 2) in geom/geom_dev.c we lock Giant for: > > 2a) calling make_dev() which seems unecessary because make_dev protects > itself with devmtx > 2b) setting a flag in cdev which I think is unecessary as well I removed this in CVS. > 3) in kern/kern_descrip.c we lock Giant for the whole life of flock() I dont see > a need for this protection becuase > > 3a) access to fp is protected with FILE_LOCK()ing > 3b) otherwise it operates on local variables > 3c) it also calls VOP_* functions but those dont require Giant, right? I am not sure about this one. It might be tied in to 4). > 4) in kern/kern_lockf.c we lock Giant for the whole life of lf_advlock() I dont > think this is necessary because > > 4a) it operates on local variables > 4b) it calls various lf_*lock() functions which seems to either protect > themselves or not needed protection at all There is no synchronization between e.g. the tailq manipulation from different threads, so locking is needed. I have replaced this with a global lockf_mtx in my p4 branch but a finer grained solution is clearly desirable. This will probably involve rewriting or restructuring the code though. > 5) in kern/kern_time.c we lock Giant to protect > > 5a) tc_setclock() call - I dont know if this is necessary or not > 5b) lease_updatetime call which is a function pointer that seems to be > only assigned at one place (line 119 in kern_time.c) to a NOP function. > can this be removed? I guess you are protecting against two threads setting the clock at once. This does not seem to be a big deal, I don't imagine this will ever be in the critical path of anything. > 6) in vm/device_pager.c we lock Giant to (also) protect cdevsw->d_mmap but the device mmap > is either MPSAFE or assigned to giant_mmap (a wrapper that locks GIant and calls the original > d_mmap) so this seems unecessary. I'm not sure about this one either. Kris
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070327035759.GA47764>