Re: making crdup()/crcopy() safe??

On Tuesday, December 20, 2011 10:38:34 am Rick Macklem wrote:
> John Baldwin wrote:
> > On Monday, December 19, 2011 8:21:45 pm Rick Macklem wrote:
> > > Hi,
> > >
> > > A recent NFS client crash:
> > >   http://glebius.int.ru/tmp/nfs_panic.jpg
> > > appears to have happened because some field is
> > > bogus when crfree() is called. I've asked Gleb
> > > to disassemble crfree() for me, so I can try and
> > > see exactly which field causes the crash, however...
> > >
> > > Basically, the code:
> > >    newcred = crdup(cred);
> > >    - does read with newcred
> > >    crfree(newcred); <-- which crashes at 0x65 into
> > >                          crfree()
> > >
> > > Looking at crdup(), it calls crcopy(), which copies
> > > 4 pointers and then ref. counts them:
> > >   cr_uidinfo, cr_ruidinfo, cr_prison and cr_loginclass
> > >
> > > It seems some lock should be held while crcopy() does this,
> > > so that the pointers don't get deref'd during the copy/ref. count?
> > > (Or is there some rule that guarantees these won't change. ie. No
> > >  no calls to change_euid() or similar.)
> > >
> > > Is there such a lock and should crdup() use it?
> > 
> > In general the caller of crdup is expected to hold a reference on cred
> > or some
> > other lock to ensure that cred remains valid and cannot be free'd
> > while it is
> > being duplicated. There is no global lock that crdup can hold for
> > that,
> > instead the caller is required to guarantee that.
> > 
> Well, I think it does hold a reference on cred. I think the problem is
> that this doesn't stop another process from changing the pointer fields:
>  cr_uidinfo, cr_ruidinfo, cr_loginclass and cr_prison

No, a credential with more than one reference is immutable and can not be 
changed.

> For example, change_euid() replaces cr_uidinfo. There is something called
> cr_copysafe(), which assumes PROC_LOCK(p) is held. However, for the case
> that crashed, it is an iod read-ahead thread, so I don't think I know
> what the correct "p" argument is? It also appears that PROC_LOCK(p) is
> used to lock change_euid(), when it replaces cr_uidinfo with a different
> pointer. (Basically, it appears that the cr_uidinfo, cr_ruidinfo,
> cr_loginclass and cr_prison are protected by PROC_LOCK(p), but it isn't
> obvious to me that the NFS iod thread can know what the correct "p" is.
> In fact, that process may have already exited, since the "cred" is 
refenenced
> by b_rcred for the buffer in the buffer cache that is being read-ahead.

No, change_euid() only operates on a private credential where the caller knows 
that it holds the only reference to the credential.  The various system calls 
like setuid(), etc. all allocate a new credential, grab the PROC_LOCK to 
protect what p_ucred refers to (and to serialize updates to p_ucred for a 
given process), copy the existing credential from p_ucred into the new 
credential, update the new credential as appropriate, then change p_ucred to 
point to the new credential before dropping the PROC_LOCK.  The old credential 
then has its reference count dropped (since p_ucred no longer references it) 
via crfree().  However, that old credential is not changed and will remain 
immutable until the last reference is dropped and it is freed.

> For my NFS client case, I need a "new" cred, but it has to have cr_uidinfo
> etc filled in, since the kernel rpc does a crdup() and the cr_uidinfo
> field is used in socket calls further down. Basically, the NFS client
> fills in uid, gid-list for the "new" cred and doesn't care about other
> fields, except whatever the kernel rpc and socket ops care about.
> 
> Would it be ok if, instead of using crdup(), I create the "new" cred via:
>   cr = crget();
>   - do the same as crcopy(), except for the pointer fields, which would be
>     set as follows
>   cr->cr_uidinfo = uifind(0);  /* This means that chgsbsize() will record
>                                 * the change for uid 0, but since this is
>                                 * an iod thread for the NFS client, that
>                                 * seems ok?
>                                 */
>   cr->cr_ruidinfo = uifind(0);
>   cr->cr_loginclass = loginclass_find("default");
>   /* For cr_prison, I think what crcopy() does is safe, since cr_prison
>    * doesn't normally get changed after a process does I/O, I think?
>    * Alternately, it could be set to &prison0. Does setting it to &prison0
>    * break anything?
>    */
>   prison_hold(cr->cr_prison);
> 
> crfree() does check for these fields being non-NULL before freeing them,
> but crdup() does not check for the NULL case before incrementing ref cnts
> on them. If crdup() were changed to check for non-NULL, then I think the
> only one of the above that would need to be set is cr_uidinfo, since that
> appears to be the only one used by socket ops.
> 
> Comments? Am I missing something? Thanks, rick

Using crdup() should be fine.  Your old credential should not be changed out 
from under you since you hold a reference to it.  I think there is some other
bug that trashed your temporary credential before it was free'd.

-- 
John Baldwin