From 11f9ca696ee12e1732dd2f15dd8660e0689194b6 Mon Sep 17 00:00:00 2001 From: Mark Johnston Date: Sat, 9 Jan 2016 01:56:46 +0000 Subject: [PATCH] Prevent cv_waiters wraparound. r282971 attempted to fix this problem by decrementing cv_waiters after waking up from sleeping on a condition variable, but this can result in a use-after-free if the CV is freed before all woken threads have had a chance to run. Instead, avoid incrementing cv_waiters past INT_MAX, and have cv_signal() explicitly check for sleeping threads once cv_waiters has reached this bound. Reviewed by: jhb MFC after: 2 weeks Sponsored by: EMC / Isilon Storage Division Differential Revision: https://reviews.freebsd.org/D4822 --- sys/kern/kern_condvar.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/sys/kern/kern_condvar.c b/sys/kern/kern_condvar.c index 2700a25d477c..95a6d09e1775 100644 --- a/sys/kern/kern_condvar.c +++ b/sys/kern/kern_condvar.c @@ -31,6 +31,7 @@ __FBSDID("$FreeBSD$"); #include #include +#include #include #include #include @@ -46,6 +47,17 @@ __FBSDID("$FreeBSD$"); #include #endif +/* + * A bound below which cv_waiters is valid. Once cv_waiters reaches this bound, + * cv_signal must manually check the wait queue for threads. + */ +#define CV_WAITERS_BOUND INT_MAX + +#define CV_WAITERS_INC(cvp) do { \ + if ((cvp)->cv_waiters < CV_WAITERS_BOUND) \ + (cvp)->cv_waiters++; \ +} while (0) + /* * Common sanity checks for cv_wait* functions. */ @@ -122,7 +134,7 @@ _cv_wait(struct cv *cvp, struct lock_object *lock) sleepq_lock(cvp); - cvp->cv_waiters++; + CV_WAITERS_INC(cvp); if (lock == &Giant.lock_object) mtx_assert(&Giant, MA_OWNED); DROP_GIANT(); @@ -184,7 +196,7 @@ _cv_wait_unlock(struct cv *cvp, struct lock_object *lock) sleepq_lock(cvp); - cvp->cv_waiters++; + CV_WAITERS_INC(cvp); DROP_GIANT(); sleepq_add(cvp, lock, cvp->cv_description, SLEEPQ_CONDVAR, 0); @@ -240,7 +252,7 @@ _cv_wait_sig(struct cv *cvp, struct lock_object *lock) sleepq_lock(cvp); - cvp->cv_waiters++; + CV_WAITERS_INC(cvp); if (lock == &Giant.lock_object) mtx_assert(&Giant, MA_OWNED); DROP_GIANT(); @@ -307,7 +319,7 @@ _cv_timedwait_sbt(struct cv *cvp, struct lock_object *lock, sbintime_t sbt, sleepq_lock(cvp); - cvp->cv_waiters++; + CV_WAITERS_INC(cvp); if (lock == &Giant.lock_object) mtx_assert(&Giant, MA_OWNED); DROP_GIANT(); @@ -376,7 +388,7 @@ _cv_timedwait_sig_sbt(struct cv *cvp, struct lock_object *lock, sleepq_lock(cvp); - cvp->cv_waiters++; + CV_WAITERS_INC(cvp); if (lock == &Giant.lock_object) mtx_assert(&Giant, MA_OWNED); DROP_GIANT(); @@ -422,8 +434,15 @@ cv_signal(struct cv *cvp) wakeup_swapper = 0; sleepq_lock(cvp); if (cvp->cv_waiters > 0) { - cvp->cv_waiters--; - wakeup_swapper = sleepq_signal(cvp, SLEEPQ_CONDVAR, 0, 0); + if (cvp->cv_waiters == CV_WAITERS_BOUND && + sleepq_lookup(cvp) == NULL) { + cvp->cv_waiters = 0; + } else { + if (cvp->cv_waiters < CV_WAITERS_BOUND) + cvp->cv_waiters--; + wakeup_swapper = sleepq_signal(cvp, SLEEPQ_CONDVAR, 0, + 0); + } } sleepq_release(cvp); if (wakeup_swapper)