Re: making crdup()/crcopy() safe??
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. -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
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@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
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
Re: making crdup()/crcopy() safe??
John,
On Tue, Dec 20, 2011 at 09:00:39AM -0500, John Baldwin wrote:
J In general the caller of crdup is expected to hold a reference on cred or
some
J other lock to ensure that cred remains valid and cannot be free'd while it
is
J being duplicated. There is no global lock that crdup can hold for that,
J instead the caller is required to guarantee that.
I already noticed to Rick in a private email, that there is suspisious
place in new NFS, where newly allocated (via crdup) cred is temporarily
placed on td_ucred, and then removed at the end of function. The function
may sleep in malloc() and also block on mutexes.
I suspect, that it may happen, that some other kernel facility performs
crfree(td-td_ucred), and later on we are using already freed cred.
However, I can't imagine which facility may do this. What if process gets
SIGKILL while its thread is blocked on mutex or sleeping? Would it
be reaped after it yields or before?
Attached patch demonstrates what I am trying to address, but I'm not sure
that this temporary placing on td_ucred is really unsafe. Can you please
look at this?
--
Totus tuus, Glebius.
Index: nfs_commonkrpc.c
===
--- nfs_commonkrpc.c (revision 228700)
+++ nfs_commonkrpc.c (working copy)
@@ -186,9 +186,9 @@
* Use the credential in nr_cred, if not NULL.
*/
if (nrp-nr_cred != NULL)
- td-td_ucred = nrp-nr_cred;
+ td-td_ucred = NFSHOLDCRED(nrp-nr_cred);
else
- td-td_ucred = cred;
+ td-td_ucred = NFSHOLDCRED(cred);
saddr = nrp-nr_nam;
if (saddr-sa_family == AF_INET)
@@ -220,10 +220,8 @@
saddr = NFSSOCKADDR(nrp-nr_nam, struct sockaddr *);
error = socreate(saddr-sa_family, so, nrp-nr_sotype,
nrp-nr_soproto, td-td_ucred, td);
- if (error) {
- td-td_ucred = origcred;
+ if (error)
goto out;
- }
do {
if (error != 0 pktscale 2)
pktscale--;
@@ -251,10 +249,8 @@
error = soreserve(so, sndreserve, rcvreserve);
} while (error != 0 pktscale 2);
soclose(so);
- if (error) {
- td-td_ucred = origcred;
+ if (error)
goto out;
- }
client = clnt_reconnect_create(nconf, saddr, nrp-nr_prog,
nrp-nr_vers, sndreserve, rcvreserve);
@@ -305,10 +301,11 @@
mtx_unlock(nrp-nr_mtx);
}
+out:
/* Restore current thread's credentials. */
+ NFSFREECRED(td-td_ucred);
td-td_ucred = origcred;
-out:
NFSEXITCODE(error);
return (error);
}
Index: nfsport.h
===
--- nfsport.h (revision 228700)
+++ nfsport.h (working copy)
@@ -515,6 +515,7 @@
#define NFSNEWCRED(c) (crdup(c))
#define NFSPROCCRED(p) ((p)-td_ucred)
#define NFSFREECRED(c) (crfree(c))
+#define NFSHOLDCRED(c) (crhold(c))
#define NFSUIOPROC(u, p) ((u)-uio_td = NULL)
#define NFSPROCP(p) ((p)-td_proc)
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: making crdup()/crcopy() safe??
On Tuesday, December 20, 2011 2:31:40 pm Gleb Smirnoff wrote: John, On Tue, Dec 20, 2011 at 09:00:39AM -0500, John Baldwin wrote: J In general the caller of crdup is expected to hold a reference on cred or some J other lock to ensure that cred remains valid and cannot be free'd while it is J being duplicated. There is no global lock that crdup can hold for that, J instead the caller is required to guarantee that. I already noticed to Rick in a private email, that there is suspisious place in new NFS, where newly allocated (via crdup) cred is temporarily placed on td_ucred, and then removed at the end of function. The function may sleep in malloc() and also block on mutexes. None of that matters. Only curthread touches td_ucred. It isn't going to free its own credential while it is asleep. :) I suspect, that it may happen, that some other kernel facility performs crfree(td-td_ucred), and later on we are using already freed cred. However, I can't imagine which facility may do this. What if process gets SIGKILL while its thread is blocked on mutex or sleeping? Would it be reaped after it yields or before? No, a signal is merely marked as pending. It isn't delivered to a thread in the kernel until it exits back out of the kernel and prepares to return to userland (e.g. in userret()). Only at that time will the thread actually be killed. -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
making crdup()/crcopy() safe??
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? Thanks in advance for any info, rick ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
