From owner-freebsd-current@FreeBSD.ORG Tue Dec 20 19:31:41 2011 Return-Path: Delivered-To: freebsd-current@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C8AAD106564A; Tue, 20 Dec 2011 19:31:41 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebius.int.ru (glebius.int.ru [81.19.64.117]) by mx1.freebsd.org (Postfix) with ESMTP id 537DD8FC13; Tue, 20 Dec 2011 19:31:41 +0000 (UTC) Received: from cell.glebius.int.ru (localhost [127.0.0.1]) by cell.glebius.int.ru (8.14.5/8.14.5) with ESMTP id pBKJVe5Z071432; Tue, 20 Dec 2011 23:31:40 +0400 (MSK) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebius.int.ru (8.14.5/8.14.5/Submit) id pBKJVeQC071431; Tue, 20 Dec 2011 23:31:40 +0400 (MSK) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebius.int.ru: glebius set sender to glebius@FreeBSD.org using -f Date: Tue, 20 Dec 2011 23:31:40 +0400 From: Gleb Smirnoff To: John Baldwin Message-ID: <20111220193139.GD70684@FreeBSD.org> References: <261812530.427572.1324344105125.JavaMail.root@erie.cs.uoguelph.ca> <201112200900.39087.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="FCuugMFkClbJLl1L" Content-Disposition: inline In-Reply-To: <201112200900.39087.jhb@freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-current@FreeBSD.org, Rick Macklem Subject: Re: making crdup()/crcopy() safe?? X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Dec 2011 19:31:42 -0000 --FCuugMFkClbJLl1L Content-Type: text/plain; charset=koi8-r Content-Disposition: inline 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. --FCuugMFkClbJLl1L Content-Type: text/x-diff; charset=koi8-r Content-Disposition: attachment; filename="nfs_cred.diff" 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) --FCuugMFkClbJLl1L--