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

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

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.

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

> --
> John Baldwin
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to
> "freebsd-current-unsubscribe_at_freebsd.org"