Index: kern_acct.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/kern_acct.c,v
retrieving revision 1.75
diff -u -r1.75 kern_acct.c
--- kern_acct.c	21 Sep 2005 15:28:07 -0000	1.75
+++ kern_acct.c	21 Sep 2005 18:03:58 -0000
@@ -1,4 +1,5 @@
 /*-
+ * Copyright (c) 2005 Robert N. M. Watson
  * Copyright (c) 1994 Christopher G. Demetriou
  * Copyright (c) 1982, 1986, 1989, 1993
  *	The Regents of the University of California.  All rights reserved.
@@ -56,6 +57,7 @@
 #include <sys/fcntl.h>
 #include <sys/syslog.h>
 #include <sys/kernel.h>
+#include <sys/sx.h>
 #include <sys/sysent.h>
 #include <sys/sysctl.h>
 #include <sys/namei.h>
@@ -90,15 +92,13 @@
 /*
  * Accounting vnode pointer, saved vnode pointer, and flags for each.
  */
-static struct	vnode *acctp;
-static struct	ucred *acctcred;
-static int	acctflags;
-static struct	vnode *savacctp;
-static struct	ucred *savacctcred;
-static int	savacctflags;
+static int		 acct_suspended;
+static struct vnode	*acct_vp;
+static struct ucred	*acct_cred;
+static int		 acct_flags;
+static struct sx	 acct_sx;
 
-static struct mtx	acct_mtx;
-MTX_SYSINIT(acct, &acct_mtx, "accounting", MTX_DEF);
+SX_SYSINIT(acct, &acct_sx, "acct_sx");
 
 /*
  * Values associated with enabling and disabling accounting
@@ -129,85 +129,86 @@
 	} */ *uap;
 {
 	struct nameidata nd;
-	int error, flags;
+	int error, flags, vfslocked = 0;
 
 	/* Make sure that the caller is root. */
 	error = suser(td);
 	if (error)
 		return (error);
 
-	mtx_lock(&Giant);
-
 	/*
 	 * If accounting is to be started to a file, open that file for
 	 * appending and make sure it's a 'normal'.
 	 */
 	if (uap->path != NULL) {
-		NDINIT(&nd, LOOKUP, NOFOLLOW, UIO_USERSPACE, uap->path, td);
+		NDINIT(&nd, LOOKUP, NOFOLLOW | MPSAFE, UIO_USERSPACE,
+		    uap->path, td);
 		flags = FWRITE | O_APPEND;
 		error = vn_open(&nd, &flags, 0, -1);
+		vfslocked = NDHASGIANT(&nd);
 		if (error)
-			goto done2;
+			goto done_vfs;
 		NDFREE(&nd, NDF_ONLY_PNBUF);
 #ifdef MAC
 		error = mac_check_system_acct(td->td_ucred, nd.ni_vp);
 		if (error) {
 			VOP_UNLOCK(nd.ni_vp, 0, td);
 			vn_close(nd.ni_vp, flags, td->td_ucred, td);
-			goto done2;
+			goto done_vfs;
 		}
 #endif
 		VOP_UNLOCK(nd.ni_vp, 0, td);
 		if (nd.ni_vp->v_type != VREG) {
 			vn_close(nd.ni_vp, flags, td->td_ucred, td);
 			error = EACCES;
-			goto done2;
+			goto done_vfs;
 		}
 #ifdef MAC
 	} else {
 		error = mac_check_system_acct(td->td_ucred, NULL);
 		if (error)
-			goto done2;
+			goto done_vfs;
 #endif
 	}
 
-	mtx_lock(&acct_mtx);
+	/*
+	 * Disallow concurrent access to the accounting vnode while we swap
+	 * it out, in order to prevent access after close.
+	 */
+	sx_xlock(&acct_sx);
 
 	/*
 	 * If accounting was previously enabled, kill the old space-watcher,
 	 * close the file, and (if no new file was specified, leave).
-	 *
-	 * XXX arr: should not hold lock over vnode operation.
 	 */
-	if (acctp != NULLVP || savacctp != NULLVP) {
+	if (acct_vp != NULL) {
 		callout_stop(&acctwatch_callout);
-		error = vn_close((acctp != NULLVP ? acctp : savacctp),
-		    (acctp != NULLVP ? acctflags : savacctflags),
-		    (acctcred != NOCRED ? acctcred : savacctcred), td);
-		acctp = savacctp = NULLVP;
-		crfree(acctcred != NOCRED ? acctcred : savacctcred);
-		acctcred = savacctcred = NOCRED;
+		error = vn_close(acct_vp, acct_flags, acct_cred, td);
+		crfree(acct_cred);
+		acct_vp = NULL;
+		acct_cred = NULL;
+		acct_flags = 0;
 		log(LOG_NOTICE, "Accounting disabled\n");
 	}
-	if (uap->path == NULL) {
-		mtx_unlock(&acct_mtx);
-		goto done2;
-	}
+	if (uap->path == NULL)
+		goto done_sx;
 
 	/*
 	 * Save the new accounting file vnode, and schedule the new
 	 * free space watcher.
 	 */
-	acctp = nd.ni_vp;
-	acctcred = crhold(td->td_ucred);
-	acctflags = flags;
+	acct_suspended = 0;
+	acct_vp = nd.ni_vp;
+	acct_cred = crhold(td->td_ucred);
+	acct_flags = flags;
 	callout_init(&acctwatch_callout, 0);
-	mtx_unlock(&acct_mtx);
 	log(LOG_NOTICE, "Accounting enabled\n");
 	acctwatch(NULL);
 
-done2:
-	mtx_unlock(&Giant);
+done_sx:
+	sx_xunlock(&acct_sx);
+done_vfs:
+	VFS_UNLOCK_GIANT(vfslocked);
 	return (error);
 }
 
@@ -226,27 +227,24 @@
 	struct plimit *newlim, *oldlim;
 	struct proc *p;
 	struct rusage *r;
-	struct ucred *uc;
-	struct vnode *vp;
 	int t, ret;
 
 	/*
 	 * Lockless check of accounting condition before doing the hard
 	 * work.
 	 */
-	if (acctp == NULLVP)
+	if (acct_vp == NULL || acct_suspended)
 		return (0);
 
-	mtx_lock(&acct_mtx);
+	sx_slock(&acct_sx);
 
 	/*
 	 * If accounting isn't enabled, don't bother.  Have to check again
 	 * once we own the lock in case we raced with disabling of accounting
 	 * by another thread.
 	 */
-	vp = acctp;
-	if (vp == NULLVP) {
-		mtx_unlock(&acct_mtx);
+	if (acct_vp == NULL || acct_suspended) {
+		sx_sunlock(&acct_sx);
 		return (0);
 	}
 
@@ -303,13 +301,6 @@
 	PROC_UNLOCK(p);
 
 	/*
-	 * Finish doing things that require acct_mtx, and release acct_mtx.
-	 */
-	uc = crhold(acctcred);
-	vref(vp);
-	mtx_unlock(&acct_mtx);
-
-	/*
 	 * Eliminate any file size rlimit.
 	 */
 	newlim = lim_alloc();
@@ -324,12 +315,12 @@
 	/*
 	 * Write the accounting information to the file.
 	 */
-	VOP_LEASE(vp, td, uc, LEASE_WRITE);
-	ret = vn_rdwr(UIO_WRITE, vp, (caddr_t)&acct, sizeof (acct),
-	    (off_t)0, UIO_SYSSPACE, IO_APPEND|IO_UNIT, uc, NOCRED,
+	VOP_LEASE(acct_vp, td, acct_cred, LEASE_WRITE);
+	ret = vn_rdwr(UIO_WRITE, acct_vp, (caddr_t)&acct, sizeof (acct),
+	    (off_t)0, UIO_SYSSPACE, IO_APPEND|IO_UNIT, acct_cred, NOCRED,
 	    (int *)0, td);
-	vrele(vp);
-	crfree(uc);
+
+	sx_sunlock(&acct_sx);
 	return (ret);
 }
 
@@ -385,53 +376,39 @@
 {
 	struct statfs sb;
 
-	mtx_lock(&acct_mtx);
+	sx_xlock(&acct_sx);	/* May change accounting configuration. */
+
+	if (acct_vp->v_type == VBAD) {
+		(void) vn_close(acct_vp, acct_flags, acct_cred, NULL);
+		crfree(acct_cred);
+		acct_vp = NULL;
+		acct_cred = NULL;
+		acct_flags = 0;
+		sx_xunlock(&acct_sx);
+		return;
+	}
 
 	/*
-	 * XXX arr: need to fix the issue of holding acct_mtx over
-	 * the below vnode operations.
+	 * Stopping here is better than continuing, maybe it will be VBAD
+	 * next time around.
 	 */
-	if (savacctp != NULLVP) {
-		if (savacctp->v_type == VBAD) {
-			(void) vn_close(savacctp, savacctflags, savacctcred,
-			    NULL);
-			savacctp = NULLVP;
-			savacctcred = NOCRED;
-			mtx_unlock(&acct_mtx);
-			return;
-		}
-		(void)VFS_STATFS(savacctp->v_mount, &sb, curthread);
+	if (VFS_STATFS(acct_vp->v_mount, &sb, curthread) < 0) {
+		sx_xunlock(&acct_sx);
+		return;
+	}
+
+	if (acct_suspended) {
 		if (sb.f_bavail > acctresume * sb.f_blocks / 100) {
-			acctp = savacctp;
-			acctcred = savacctcred;
-			acctflags = savacctflags;
-			savacctp = NULLVP;
-			savacctcred = NOCRED;
+			acct_suspended = 0;
 			log(LOG_NOTICE, "Accounting resumed\n");
 		}
 	} else {
-		if (acctp == NULLVP) {
-			mtx_unlock(&acct_mtx);
-			return;
-		}
-		if (acctp->v_type == VBAD) {
-			(void) vn_close(acctp, acctflags, acctcred, NULL);
-			acctp = NULLVP;
-			crfree(acctcred);
-			acctcred = NOCRED;
-			mtx_unlock(&acct_mtx);
-			return;
-		}
-		(void)VFS_STATFS(acctp->v_mount, &sb, curthread);
 		if (sb.f_bavail <= acctsuspend * sb.f_blocks / 100) {
-			savacctp = acctp;
-			savacctflags = acctflags;
-			savacctcred = acctcred;
-			acctp = NULLVP;
-			acctcred = NOCRED;
+			acct_suspended = 1;
 			log(LOG_NOTICE, "Accounting suspended\n");
 		}
 	}
+
 	callout_reset(&acctwatch_callout, acctchkfreq * hz, acctwatch, NULL);
-	mtx_unlock(&acct_mtx);
+	sx_xunlock(&acct_sx);
 }
