Author: bdrewery
Date: Wed Aug 26 03:44:48 2015
New Revision: 287155
URL: https://svnweb.freebsd.org/changeset/base/287155
Log:
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
Modified:
head/sys/dev/filemon/filemon.c
head/sys/dev/filemon/filemon_lock.c
Modified: head/sys/dev/filemon/filemon.c
==============================================================================
--- head/sys/dev/filemon/filemon.c Wed Aug 26 03:37:33 2015
(r287154)
+++ head/sys/dev/filemon/filemon.c Wed Aug 26 03:44:48 2015
(r287155)
@@ -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: head/sys/dev/filemon/filemon_lock.c
==============================================================================
--- head/sys/dev/filemon/filemon_lock.c Wed Aug 26 03:37:33 2015
(r287154)
+++ head/sys/dev/filemon/filemon_lock.c Wed Aug 26 03:44:48 2015
(r287155)
@@ -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);
}
_______________________________________________
[email protected] mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "[email protected]"
