diff options
author | Bryan Drewery <bdrewery@FreeBSD.org> | 2015-08-26 03:44:48 +0000 |
---|---|---|
committer | Bryan Drewery <bdrewery@FreeBSD.org> | 2015-08-26 03:44:48 +0000 |
commit | 8183f2e31230be0231ab0fed0f73d8bba45854b1 (patch) | |
tree | 60719e4f637d14f338c3fc4065a271b4242477db /sys/dev/filemon | |
parent | 2ae6c7c3ad5c154e045adf13784649bbd029a1db (diff) | |
download | src-8183f2e31230be0231ab0fed0f73d8bba45854b1.tar.gz src-8183f2e31230be0231ab0fed0f73d8bba45854b1.zip |
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
Notes
Notes:
svn path=/head/; revision=287155
Diffstat (limited to 'sys/dev/filemon')
-rw-r--r-- | sys/dev/filemon/filemon.c | 27 | ||||
-rw-r--r-- | sys/dev/filemon/filemon_lock.c | 77 |
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); } |