svn commit: r287598 - stable/10/sys/dev/filemon

Bryan Drewery bdrewery at FreeBSD.org
Wed Sep 9 17:15:14 UTC 2015


Author: bdrewery
Date: Wed Sep  9 17:15:13 2015
New Revision: 287598
URL: https://svnweb.freebsd.org/changeset/base/287598

Log:
  MFC r287151,r287152,r287153,r287155:
  
    r287151:
      Move common locking for filemon_inuse and struct filemon* to
      filemon_pid_check().
    r287152:
      Remove unneeded inuse list locking in filemon_comment().
    r287153:
      Avoid taking proctree_lock and searching parents in wrappers if not needed.
    r287155:
      Fix filemon locking races.
  
   Relnotes:	yes (race fixes)
   Sponsored by:	EMC / Isilon Storage Division

Modified:
  stable/10/sys/dev/filemon/filemon.c
  stable/10/sys/dev/filemon/filemon_lock.c
  stable/10/sys/dev/filemon/filemon_wrapper.c
Directory Properties:
  stable/10/   (props changed)

Modified: stable/10/sys/dev/filemon/filemon.c
==============================================================================
--- stable/10/sys/dev/filemon/filemon.c	Wed Sep  9 13:24:39 2015	(r287597)
+++ stable/10/sys/dev/filemon/filemon.c	Wed Sep  9 17:15:13 2015	(r287598)
@@ -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", "Fil
 
 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 oflag
 
 		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,
 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);

Modified: stable/10/sys/dev/filemon/filemon_lock.c
==============================================================================
--- stable/10/sys/dev/filemon/filemon_lock.c	Wed Sep  9 13:24:39 2015	(r287597)
+++ stable/10/sys/dev/filemon/filemon_lock.c	Wed Sep  9 17:15:13 2015	(r287598)
@@ -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);
 }

Modified: stable/10/sys/dev/filemon/filemon_wrapper.c
==============================================================================
--- stable/10/sys/dev/filemon/filemon_wrapper.c	Wed Sep  9 13:24:39 2015	(r287597)
+++ stable/10/sys/dev/filemon/filemon_wrapper.c	Wed Sep  9 17:15:13 2015	(r287598)
@@ -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
@@ -86,17 +87,25 @@ filemon_pid_check(struct proc *p)
 {
 	struct filemon *filemon;
 
+	filemon_lock_read();
+	if (TAILQ_EMPTY(&filemons_inuse)) {
+		filemon_unlock_read();
+		return (NULL);
+	}
 	sx_slock(&proctree_lock);
 	while (p != initproc) {
 		TAILQ_FOREACH(filemon, &filemons_inuse, link) {
 			if (p->p_pid == filemon->pid) {
 				sx_sunlock(&proctree_lock);
+				filemon_filemon_lock(filemon);
+				filemon_unlock_read();
 				return (filemon);
 			}
 		}
 		p = proc_realparent(p);
 	}
 	sx_sunlock(&proctree_lock);
+	filemon_unlock_read();
 	return (NULL);
 }
 
@@ -109,9 +118,6 @@ filemon_comment(struct filemon *filemon)
 	/* Load timestamp before locking.  Less accurate but less contention. */
 	getmicrotime(&now);
 
-	/* Grab a read lock on the filemon inuse list. */
-	filemon_lock_read();
-
 	/* Lock the found filemon structure. */
 	filemon_filemon_lock(filemon);
 
@@ -124,9 +130,6 @@ filemon_comment(struct filemon *filemon)
 
 	/* Unlock the found filemon structure. */
 	filemon_filemon_unlock(filemon);
-
-	/* Release the read lock. */
-	filemon_unlock_read();
 }
 
 static int
@@ -138,13 +141,7 @@ filemon_wrapper_chdir(struct thread *td,
 	struct filemon *filemon;
 
 	if ((ret = sys_chdir(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			copyinstr(uap->path, filemon->fname1,
 			    sizeof(filemon->fname1), &done);
 
@@ -157,9 +154,6 @@ filemon_wrapper_chdir(struct thread *td,
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -177,13 +171,7 @@ filemon_wrapper_execve(struct thread *td
 	copyinstr(uap->fname, fname, sizeof(fname), &done);
 
 	if ((ret = sys_execve(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			len = snprintf(filemon->msgbufr,
 			    sizeof(filemon->msgbufr), "E %d %s\n",
 			    curproc->p_pid, fname);
@@ -193,9 +181,6 @@ filemon_wrapper_execve(struct thread *td
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -215,13 +200,7 @@ filemon_wrapper_freebsd32_execve(struct 
 	copyinstr(uap->fname, fname, sizeof(fname), &done);
 
 	if ((ret = freebsd32_execve(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			len = snprintf(filemon->msgbufr,
 			    sizeof(filemon->msgbufr), "E %d %s\n",
 			    curproc->p_pid, fname);
@@ -231,9 +210,6 @@ filemon_wrapper_freebsd32_execve(struct 
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -248,13 +224,7 @@ filemon_wrapper_fork(struct thread *td, 
 	struct filemon *filemon;
 
 	if ((ret = sys_fork(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			len = snprintf(filemon->msgbufr,
 			    sizeof(filemon->msgbufr), "F %d %ld\n",
 			    curproc->p_pid, (long)curthread->td_retval[0]);
@@ -264,9 +234,6 @@ filemon_wrapper_fork(struct thread *td, 
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -281,13 +248,7 @@ filemon_wrapper_open(struct thread *td, 
 	struct filemon *filemon;
 
 	if ((ret = sys_open(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			copyinstr(uap->path, filemon->fname1,
 			    sizeof(filemon->fname1), &done);
 
@@ -313,9 +274,6 @@ filemon_wrapper_open(struct thread *td, 
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -330,13 +288,7 @@ filemon_wrapper_openat(struct thread *td
 	struct filemon *filemon;
 
 	if ((ret = sys_openat(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			copyinstr(uap->path, filemon->fname1,
 			    sizeof(filemon->fname1), &done);
 
@@ -375,9 +327,6 @@ filemon_wrapper_openat(struct thread *td
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -392,13 +341,7 @@ filemon_wrapper_rename(struct thread *td
 	struct filemon *filemon;
 
 	if ((ret = sys_rename(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			copyinstr(uap->from, filemon->fname1,
 			    sizeof(filemon->fname1), &done);
 			copyinstr(uap->to, filemon->fname2,
@@ -413,9 +356,6 @@ filemon_wrapper_rename(struct thread *td
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -430,13 +370,7 @@ filemon_wrapper_link(struct thread *td, 
 	struct filemon *filemon;
 
 	if ((ret = sys_link(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			copyinstr(uap->path, filemon->fname1,
 			    sizeof(filemon->fname1), &done);
 			copyinstr(uap->link, filemon->fname2,
@@ -451,9 +385,6 @@ filemon_wrapper_link(struct thread *td, 
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -468,13 +399,7 @@ filemon_wrapper_symlink(struct thread *t
 	struct filemon *filemon;
 
 	if ((ret = sys_symlink(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			copyinstr(uap->path, filemon->fname1,
 			    sizeof(filemon->fname1), &done);
 			copyinstr(uap->link, filemon->fname2,
@@ -489,9 +414,6 @@ filemon_wrapper_symlink(struct thread *t
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -507,13 +429,7 @@ filemon_wrapper_linkat(struct thread *td
 	struct filemon *filemon;
 
 	if ((ret = sys_linkat(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			copyinstr(uap->path1, filemon->fname1,
 			    sizeof(filemon->fname1), &done);
 			copyinstr(uap->path2, filemon->fname2,
@@ -528,9 +444,6 @@ filemon_wrapper_linkat(struct thread *td
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -546,13 +459,7 @@ filemon_wrapper_stat(struct thread *td, 
 	struct filemon *filemon;
 
 	if ((ret = sys_stat(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			copyinstr(uap->path, filemon->fname1,
 			    sizeof(filemon->fname1), &done);
 
@@ -565,9 +472,6 @@ filemon_wrapper_stat(struct thread *td, 
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -584,13 +488,7 @@ filemon_wrapper_freebsd32_stat(struct th
 	struct filemon *filemon;
 
 	if ((ret = freebsd32_stat(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			copyinstr(uap->path, filemon->fname1,
 			    sizeof(filemon->fname1), &done);
 
@@ -603,9 +501,6 @@ filemon_wrapper_freebsd32_stat(struct th
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -622,13 +517,7 @@ filemon_wrapper_sys_exit(struct thread *
 	/* Get timestamp before locking. */
 	getmicrotime(&now);
 
-	/* Grab a read lock on the filemon inuse list. */
-	filemon_lock_read();
-
 	if ((filemon = filemon_pid_check(curproc)) != NULL) {
-		/* Lock the found filemon structure. */
-		filemon_filemon_lock(filemon);
-
 		len = snprintf(filemon->msgbufr, sizeof(filemon->msgbufr),
 		    "X %d %d\n", curproc->p_pid, uap->rval);
 
@@ -649,9 +538,6 @@ filemon_wrapper_sys_exit(struct thread *
 		filemon_filemon_unlock(filemon);
 	}
 
-	/* Release the read lock. */
-	filemon_unlock_read();
-
 	sys_sys_exit(td, uap);
 }
 
@@ -664,13 +550,7 @@ filemon_wrapper_unlink(struct thread *td
 	struct filemon *filemon;
 
 	if ((ret = sys_unlink(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			copyinstr(uap->path, filemon->fname1,
 			    sizeof(filemon->fname1), &done);
 
@@ -683,9 +563,6 @@ filemon_wrapper_unlink(struct thread *td
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);
@@ -699,13 +576,7 @@ filemon_wrapper_vfork(struct thread *td,
 	struct filemon *filemon;
 
 	if ((ret = sys_vfork(td, uap)) == 0) {
-		/* Grab a read lock on the filemon inuse list. */
-		filemon_lock_read();
-
 		if ((filemon = filemon_pid_check(curproc)) != NULL) {
-			/* Lock the found filemon structure. */
-			filemon_filemon_lock(filemon);
-
 			len = snprintf(filemon->msgbufr,
 			    sizeof(filemon->msgbufr), "F %d %ld\n",
 			    curproc->p_pid, (long)curthread->td_retval[0]);
@@ -715,9 +586,6 @@ filemon_wrapper_vfork(struct thread *td,
 			/* Unlock the found filemon structure. */
 			filemon_filemon_unlock(filemon);
 		}
-
-		/* Release the read lock. */
-		filemon_unlock_read();
 	}
 
 	return (ret);


More information about the svn-src-all mailing list