From owner-svn-src-all@freebsd.org Sat Dec 28 22:24:17 2019 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 7E2741CA1FD; Sat, 28 Dec 2019 22:24:17 +0000 (UTC) (envelope-from rmacklem@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 47ldXK2qVpz3K3B; Sat, 28 Dec 2019 22:24:17 +0000 (UTC) (envelope-from rmacklem@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 5C39C7E53; Sat, 28 Dec 2019 22:24:17 +0000 (UTC) (envelope-from rmacklem@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id xBSMOHPp083298; Sat, 28 Dec 2019 22:24:17 GMT (envelope-from rmacklem@FreeBSD.org) Received: (from rmacklem@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id xBSMOGsl083294; Sat, 28 Dec 2019 22:24:16 GMT (envelope-from rmacklem@FreeBSD.org) Message-Id: <201912282224.xBSMOGsl083294@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: rmacklem set sender to rmacklem@FreeBSD.org using -f From: Rick Macklem Date: Sat, 28 Dec 2019 22:24:16 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r356160 - stable/12/sys/fs/nfs X-SVN-Group: stable-12 X-SVN-Commit-Author: rmacklem X-SVN-Commit-Paths: stable/12/sys/fs/nfs X-SVN-Commit-Revision: 356160 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 28 Dec 2019 22:24:17 -0000 Author: rmacklem Date: Sat Dec 28 22:24:16 2019 New Revision: 356160 URL: https://svnweb.freebsd.org/changeset/base/356160 Log: MFC: r355194 Fix two races while handling nfsuserd daemon start/stop. A crash was reported where the nr_client field was NULL during an upcall to the nfsuserd daemon. Since nr_client == NULL only occurs when the nfsuserd daemon is being shut down, it appeared to be caused by a race between doing an upcall and the daemon shutting down. By inspection two races were identified: 1 - The nfsrv_nfsuserd variable is used to indicate whether or not the daemon is running. However it did not handle the intermediate phase where the daemon is starting or stopping. This was fixed by making nfsrv_nfsuserd tri-state and having the functions that are called during start/stop to obey the intermediate state. 2 - nfsrv_nfsuserd was checked to see that the daemon was running at the beginning of an upcall, but nothing prevented the daemon from being shut down while an upcall was still in progress. This race probably caused the crash. The patch fixes this by adding a count of upcalls in progress and having the shut down function delay until this count goes to zero before getting rid of nr_client and related data used by an upcall. Modified: stable/12/sys/fs/nfs/nfs.h stable/12/sys/fs/nfs/nfs_commonport.c stable/12/sys/fs/nfs/nfs_commonsubs.c stable/12/sys/fs/nfs/nfsport.h Directory Properties: stable/12/ (props changed) Modified: stable/12/sys/fs/nfs/nfs.h ============================================================================== --- stable/12/sys/fs/nfs/nfs.h Sat Dec 28 19:04:29 2019 (r356159) +++ stable/12/sys/fs/nfs/nfs.h Sat Dec 28 22:24:16 2019 (r356160) @@ -797,6 +797,9 @@ struct nfsslot { struct mbuf *nfssl_reply; }; +/* Enumerated type for nfsuserd state. */ +typedef enum { NOTRUNNING=0, STARTSTOP=1, RUNNING=2 } nfsuserd_state; + #endif /* _KERNEL */ #endif /* _NFS_NFS_H */ Modified: stable/12/sys/fs/nfs/nfs_commonport.c ============================================================================== --- stable/12/sys/fs/nfs/nfs_commonport.c Sat Dec 28 19:04:29 2019 (r356159) +++ stable/12/sys/fs/nfs/nfs_commonport.c Sat Dec 28 22:24:16 2019 (r356160) @@ -56,7 +56,7 @@ __FBSDID("$FreeBSD$"); #include extern int nfscl_ticks; -extern int nfsrv_nfsuserd; +extern nfsuserd_state nfsrv_nfsuserd; extern struct nfssockreq nfsrv_nfsuserdsock; extern void (*nfsd_call_recall)(struct vnode *, int, struct ucred *, struct thread *); @@ -774,7 +774,7 @@ nfscommon_modevent(module_t mod, int type, void *data) break; case MOD_UNLOAD: - if (newnfs_numnfsd != 0 || nfsrv_nfsuserd != 0 || + if (newnfs_numnfsd != 0 || nfsrv_nfsuserd != NOTRUNNING || nfs_numnfscbd != 0) { error = EBUSY; break; Modified: stable/12/sys/fs/nfs/nfs_commonsubs.c ============================================================================== --- stable/12/sys/fs/nfs/nfs_commonsubs.c Sat Dec 28 19:04:29 2019 (r356159) +++ stable/12/sys/fs/nfs/nfs_commonsubs.c Sat Dec 28 22:24:16 2019 (r356160) @@ -64,7 +64,8 @@ struct timeval nfsboottime; /* Copy boottime once, so int nfscl_ticks; int nfsrv_useacl = 1; struct nfssockreq nfsrv_nfsuserdsock; -int nfsrv_nfsuserd = 0; +nfsuserd_state nfsrv_nfsuserd = NOTRUNNING; +static int nfsrv_userdupcalls = 0; struct nfsreqhead nfsd_reqq; uid_t nfsrv_defaultuid = UID_NOBODY; gid_t nfsrv_defaultgid = GID_NOGROUP; @@ -3524,18 +3525,22 @@ nfsrv_nfsuserdport(struct nfsuserd_args *nargs, NFSPRO int error; NFSLOCKNAMEID(); - if (nfsrv_nfsuserd) { + if (nfsrv_nfsuserd != NOTRUNNING) { NFSUNLOCKNAMEID(); error = EPERM; goto out; } - nfsrv_nfsuserd = 1; - NFSUNLOCKNAMEID(); + nfsrv_nfsuserd = STARTSTOP; /* * Set up the socket record and connect. + * Set nr_client NULL before unlocking, just to ensure that no other + * process/thread/core will use a bogus old value. This could only + * occur if the use of the nameid lock to protect nfsrv_nfsuserd is + * broken. */ rp = &nfsrv_nfsuserdsock; rp->nr_client = NULL; + NFSUNLOCKNAMEID(); rp->nr_sotype = SOCK_DGRAM; rp->nr_soproto = IPPROTO_UDP; rp->nr_lock = (NFSR_RESERVEDPORT | NFSR_LOCALHOST); @@ -3571,9 +3576,15 @@ nfsrv_nfsuserdport(struct nfsuserd_args *nargs, NFSPRO rp->nr_vers = RPCNFSUSERD_VERS; if (error == 0) error = newnfs_connect(NULL, rp, NFSPROCCRED(p), p, 0); - if (error) { + if (error == 0) { + NFSLOCKNAMEID(); + nfsrv_nfsuserd = RUNNING; + NFSUNLOCKNAMEID(); + } else { free(rp->nr_nam, M_SONAME); - nfsrv_nfsuserd = 0; + NFSLOCKNAMEID(); + nfsrv_nfsuserd = NOTRUNNING; + NFSUNLOCKNAMEID(); } out: NFSEXITCODE(error); @@ -3588,14 +3599,21 @@ nfsrv_nfsuserddelport(void) { NFSLOCKNAMEID(); - if (nfsrv_nfsuserd == 0) { + if (nfsrv_nfsuserd != RUNNING) { NFSUNLOCKNAMEID(); return; } - nfsrv_nfsuserd = 0; + nfsrv_nfsuserd = STARTSTOP; + /* Wait for all upcalls to complete. */ + while (nfsrv_userdupcalls > 0) + msleep(&nfsrv_userdupcalls, NFSNAMEIDMUTEXPTR, PVFS, + "nfsupcalls", 0); NFSUNLOCKNAMEID(); newnfs_disconnect(&nfsrv_nfsuserdsock); free(nfsrv_nfsuserdsock.nr_nam, M_SONAME); + NFSLOCKNAMEID(); + nfsrv_nfsuserd = NOTRUNNING; + NFSUNLOCKNAMEID(); } /* @@ -3614,12 +3632,19 @@ nfsrv_getuser(int procnum, uid_t uid, gid_t gid, char int error; NFSLOCKNAMEID(); - if (nfsrv_nfsuserd == 0) { + if (nfsrv_nfsuserd != RUNNING) { NFSUNLOCKNAMEID(); error = EPERM; goto out; } + /* + * Maintain a count of upcalls in progress, so that nfsrv_X() + * can wait until no upcalls are in progress. + */ + nfsrv_userdupcalls++; NFSUNLOCKNAMEID(); + KASSERT(nfsrv_userdupcalls > 0, + ("nfsrv_getuser: non-positive upcalls")); nd = &nfsd; cred = newnfs_getcred(); nd->nd_flag = ND_GSSINITREPLY; @@ -3638,6 +3663,10 @@ nfsrv_getuser(int procnum, uid_t uid, gid_t gid, char } error = newnfs_request(nd, NULL, NULL, &nfsrv_nfsuserdsock, NULL, NULL, cred, RPCPROG_NFSUSERD, RPCNFSUSERD_VERS, NULL, 0, NULL, NULL); + NFSLOCKNAMEID(); + if (--nfsrv_userdupcalls == 0 && nfsrv_nfsuserd == STARTSTOP) + wakeup(&nfsrv_userdupcalls); + NFSUNLOCKNAMEID(); NFSFREECRED(cred); if (!error) { mbuf_freem(nd->nd_mrep); Modified: stable/12/sys/fs/nfs/nfsport.h ============================================================================== --- stable/12/sys/fs/nfs/nfsport.h Sat Dec 28 19:04:29 2019 (r356159) +++ stable/12/sys/fs/nfs/nfsport.h Sat Dec 28 22:24:16 2019 (r356160) @@ -669,6 +669,7 @@ void nfsrvd_rcv(struct socket *, void *, int); #define NFSLOCKSOCK() mtx_lock(&nfs_slock_mutex) #define NFSUNLOCKSOCK() mtx_unlock(&nfs_slock_mutex) #define NFSNAMEIDMUTEX extern struct mtx nfs_nameid_mutex +#define NFSNAMEIDMUTEXPTR (&nfs_nameid_mutex) #define NFSLOCKNAMEID() mtx_lock(&nfs_nameid_mutex) #define NFSUNLOCKNAMEID() mtx_unlock(&nfs_nameid_mutex) #define NFSNAMEIDREQUIRED() mtx_assert(&nfs_nameid_mutex, MA_OWNED)