Date: Wed, 24 Jan 2001 09:13:36 -0800 From: Alfred Perlstein <bright@wintelcom.net> To: John Baldwin <jhb@FreeBSD.org> Cc: Bruce Evans <bde@zeta.org.au>, cvs-all@FreeBSD.org, Garrett Wollman <wollman@khavrinen.lcs.mit.edu>, cvs-committers@FreeBSD.org, Matt Dillon <dillon@FreeBSD.org>, Chuck Paterson <cp@FreeBSD.org> Subject: Re: cvs commit: src/sys/isofs/cd9660 cd9660_vfsops.c Message-ID: <20010124091336.U26076@fw.wintelcom.net> In-Reply-To: <XFMail.010124043438.jhb@FreeBSD.org>; from jhb@FreeBSD.org on Wed, Jan 24, 2001 at 04:34:38AM -0800 References: <Pine.BSF.4.21.0101242229170.44683-100000@besplex.bde.org> <XFMail.010124043438.jhb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
I'm cc'ing those who took interest in this issue as well as a couple of people who I hope will catch any mistakes in my reasoning. Don't forget that I'm capable of providing a pretty convincing arguement and still be wrong, so keep on your toes. :) See below... * John Baldwin <jhb@FreeBSD.org> [010124 04:35] wrote: > > On 24-Jan-01 Bruce Evans wrote: > > On Tue, 23 Jan 2001, Alfred Perlstein wrote: > > > >> * John Baldwin <jhb@FreeBSD.org> [010123 16:26] wrote: > >> > jhb 2001/01/23 16:26:19 PST > >> > > >> > Modified files: > >> > sys/isofs/cd9660 cd9660_vfsops.c > >> > Log: > >> > Proc locking to protect p_ucred while we obtain additional > >> > references. > >> > >> I really don't think you need the PROC_LOCK for these. > >> > >> You only need the 'uc' variable, and even then it's only to protect > >> against rfork threads playing with setuid which is an inhernent > >> race condition as p_ucred shouldn't be NULL. (afaik). > > > > I don't think you need any locking or crhold()ing for these. I think > > xxx_mount() is only called with p == curproc, so p_ucred can't > > change. > > Unfortunately, most vfs and vnop interfaces including VFS_MOUNT() > > make > > it unclear that p == curproc by pretending to support arbitrary p's. > > Yes, Alfred and I talked this out over the phone today. I will be > backing out these ucred changes and not committing any more of them. > > I'm also going to be revisiting calcru() very soon as well. > > >> Just give it some more thought, because I'm not sure I'm right > >> about this. > > > > Me too. Several issues with struct ucred came up: 1) setgroups modifies p->pc_ucred in place The issue with setgroups is that _if_ rfork threads all share the p->pc_ucred field of struct proc, (which I believe they will need to shortly if they don't already) this can not be allowed to happen. One must make a temporary pointer assignment of the copied cred and only assign it to p->pc_ucred when the modification is complete: p->pc_ucred = crcopy(p->pc_ucred); p->pc_ucred.cr_uid = 55; p->pc_ucred.cr_gid = 100; must become: struct ucred *uc, *uc2; PROC_LOCK(p); /* prevent against concurrant _updates_ */ /* * don't use crcopy(), it could return the same ucred, * we _need_ a seperate copy */ uc = crdup(p->pc_ucred); uc->cr_uid = 55; uc->cr_gid = 100; uc2 = p->pc_ucred; /* we need old reference for crfree() */ /* * write barrier for readers to sync structure updates before * pointer asignment */ wb(); p->pc_ucred = uc; /* ok, assignments done above 'uc' is stable */ crfree(uc2); /* release now that we don't reference it anymore */ PROC_UNLOCK(p); This should ensure that anyone reading p->pc_ucred will get a stable pointer that isn't in the midst of an update nor about to be destroyed. 2) async operations We were also concerned about the VOP's using the ucreds internally (perhaps sticking them on some queue of holding them for NFS). However it doesn't make sense to syncronously crhold/crfree as that is the responsibility of the VOP (or any funcntion) to do if it plans on using the ucred after it returns. Since we're the caller, we already have _our_ reference, it is up to the function to obtain another reference by using our context (which already has a reference). Of course it's now the async subsystems's responsiblity to actually crfree(). Anything that didn't do this (calling crhold() on a passed ucred that it wishes to 'hold' onto) was broken even before SMPng. The only exception is that it may be how it was intended to work by the caller doing a crhold, but no crfree after return. (bumping the count beforehand). -- -Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org] "I have the heart of a child; I keep it in a jar on my desk." To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010124091336.U26076>