Fix filemon locking races.

Convert filemon_lock and struct filemon* lock to sx(9), rather than a
self-rolled reader-writer lock, and hold it for the entire time needed.

At least filemon_lock_write() was not checking for active readers when
it would successfully return with the write lock "held".  This led to
a race with reading entries from filemon_inuse as they were removed.  This
could be seen with QUEUE_MACRO_DEBUG enabled, causing -1 to be read as an
entry rather than a valid struct filemon*.

Fixing filemon_lock_write() to check readers was insufficient to fix the
races.

sx(9) was used as the lock could be held while taking proctree_lock and sleeping
in fo_write.

Sponsored by:	EMC / Isilon Storage Division
MFC after:	2 weeks

2 files changed, 22 insertions, 82 deletions

diff --git a/sys/dev/filemon/filemon.c b/sys/dev/filemon/filemon.c
index c711e3d29204..f8a698f65e3b 100644
--- a/sys/dev/filemon/filemon.c
+++ b/sys/dev/filemon/filemon.c
@@ -1,6 +1,7 @@
 /*-
  * Copyright (c) 2011, David E. O'Brien.
  * Copyright (c) 2009-2011, Juniper Networks, Inc.
+ * Copyright (c) 2015, EMC Corp.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -39,12 +40,14 @@ __FBSDID("$FreeBSD$");
 #include <sys/fcntl.h>
 #include <sys/ioccom.h>
 #include <sys/kernel.h>
+#include <sys/lock.h>
 #include <sys/malloc.h>
 #include <sys/module.h>
 #include <sys/mutex.h>
 #include <sys/poll.h>
 #include <sys/proc.h>
 #include <sys/queue.h>
+#include <sys/sx.h>
 #include <sys/syscall.h>
 #include <sys/sysent.h>
 #include <sys/sysproto.h>
@@ -85,12 +88,8 @@ MALLOC_DEFINE(M_FILEMON, "filemon", "File access monitor");
 
 struct filemon {
 	TAILQ_ENTRY(filemon) link;	/* Link into the in-use list. */
-	struct mtx	mtx;		/* Lock mutex for this filemon. */
-	struct cv	cv;		/* Lock condition variable for this
-					   filemon. */
+	struct sx	lock;		/* Lock mutex for this filemon. */
 	struct file	*fp;		/* Output file pointer. */
-	struct thread	*locker;	/* Ptr to the thread locking this
-					   filemon. */
 	pid_t		pid;		/* The process ID being monitored. */
 	char		fname1[MAXPATHLEN]; /* Temporary filename buffer. */
 	char		fname2[MAXPATHLEN]; /* Temporary filename buffer. */
@@ -99,11 +98,7 @@ struct filemon {
 
 static TAILQ_HEAD(, filemon) filemons_inuse = TAILQ_HEAD_INITIALIZER(filemons_inuse);
 static TAILQ_HEAD(, filemon) filemons_free = TAILQ_HEAD_INITIALIZER(filemons_free);
-static int n_readers = 0;
-static struct mtx access_mtx;
-static struct cv access_cv;
-static struct thread *access_owner = NULL;
-static struct thread *access_requester = NULL;
+static struct sx access_lock;
 
 static struct cdev *filemon_dev;
 
@@ -203,8 +198,7 @@ filemon_open(struct cdev *dev, int oflags __unused, int devtype __unused,
 
 		filemon->fp = NULL;
 
-		mtx_init(&filemon->mtx, "filemon", "filemon", MTX_DEF);
-		cv_init(&filemon->cv, "filemon");
+		sx_init(&filemon->lock, "filemon");
 	}
 
 	filemon->pid = curproc->p_pid;
@@ -234,8 +228,7 @@ filemon_close(struct cdev *dev __unused, int flag __unused, int fmt __unused,
 static void
 filemon_load(void *dummy __unused)
 {
-	mtx_init(&access_mtx, "filemon", "filemon", MTX_DEF);
-	cv_init(&access_cv, "filemon");
+	sx_init(&access_lock, "filemons_inuse");
 
 	/* Install the syscall wrappers. */
 	filemon_wrapper_install();
@@ -270,14 +263,12 @@ filemon_unload(void)
 		filemon_lock_write();
 		while ((filemon = TAILQ_FIRST(&filemons_free)) != NULL) {
 			TAILQ_REMOVE(&filemons_free, filemon, link);
-			mtx_destroy(&filemon->mtx);
-			cv_destroy(&filemon->cv);
+			sx_destroy(&filemon->lock);
 			free(filemon, M_FILEMON);
 		}
 		filemon_unlock_write();
 
-		mtx_destroy(&access_mtx);
-		cv_destroy(&access_cv);
+		sx_destroy(&access_lock);
 	}
 
 	return (error);
diff --git a/sys/dev/filemon/filemon_lock.c b/sys/dev/filemon/filemon_lock.c
index 6e836d1b26b8..a0347000fc23 100644
--- a/sys/dev/filemon/filemon_lock.c
+++ b/sys/dev/filemon/filemon_lock.c
@@ -1,5 +1,6 @@
 /*-
  * Copyright (c) 2009-2011, Juniper Networks, Inc.
+ * Copyright (c) 2015, EMC Corp.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -27,96 +28,44 @@
 #include <sys/cdefs.h>
 __FBSDID("$FreeBSD$");
 
-static void
+static __inline void
 filemon_filemon_lock(struct filemon *filemon)
 {
-	mtx_lock(&filemon->mtx);
 
-	while (filemon->locker != NULL && filemon->locker != curthread)
-		cv_wait(&filemon->cv, &filemon->mtx);
-
-	filemon->locker = curthread;
-
-	mtx_unlock(&filemon->mtx);
+	sx_xlock(&filemon->lock);
 }
 
-static void
+static __inline void
 filemon_filemon_unlock(struct filemon *filemon)
 {
-	mtx_lock(&filemon->mtx);
-
-	if (filemon->locker == curthread)
-		filemon->locker = NULL;
-
-	/* Wake up threads waiting. */
-	cv_broadcast(&filemon->cv);
 
-	mtx_unlock(&filemon->mtx);
+	sx_xunlock(&filemon->lock);
 }
 
-static void
+static __inline void
 filemon_lock_read(void)
 {
-	mtx_lock(&access_mtx);
-
-	while (access_owner != NULL || access_requester != NULL)
-		cv_wait(&access_cv, &access_mtx);
-
-	n_readers++;
-
-	/* Wake up threads waiting. */
-	cv_broadcast(&access_cv);
 
-	mtx_unlock(&access_mtx);
+	sx_slock(&access_lock);
 }
 
-static void
+static __inline void
 filemon_unlock_read(void)
 {
-	mtx_lock(&access_mtx);
 
-	if (n_readers > 0)
-		n_readers--;
-
-	/* Wake up a thread waiting. */
-	cv_broadcast(&access_cv);
-
-	mtx_unlock(&access_mtx);
+	sx_sunlock(&access_lock);
 }
 
-static void
+static __inline void
 filemon_lock_write(void)
 {
-	mtx_lock(&access_mtx);
-
-	while (access_owner != curthread) {
-		if (access_owner == NULL &&
-		    (access_requester == NULL ||
-		    access_requester == curthread)) {
-			access_owner = curthread;
-			access_requester = NULL;
-		} else {
-			if (access_requester == NULL)
-				access_requester = curthread;
 
-			cv_wait(&access_cv, &access_mtx);
-		}
-	}
-
-	mtx_unlock(&access_mtx);
+	sx_xlock(&access_lock);
 }
 
-static void
+static __inline void
 filemon_unlock_write(void)
 {
-	mtx_lock(&access_mtx);
-
-	/* Sanity check that the current thread actually has the write lock. */
-	if (access_owner == curthread)
-		access_owner = NULL;
-
-	/* Wake up a thread waiting. */
-	cv_broadcast(&access_cv);
 
-	mtx_unlock(&access_mtx);
+	sx_xunlock(&access_lock);
 }