hrev52454 adds 3 changesets to branch 'master'
old head: d787141342549b3d75777e62131f506b9b775840
new head: 498bd544a4b4bbedeb178dd0200821d95be52592
overview:
https://git.haiku-os.org/haiku/log/?qt=range&q=498bd544a4b4+%5Ed78714134254
----------------------------------------------------------------------------
adb4e6e8c5f5: kernel: Reset lock holder and count on mutex_destroy().
Previously, there were a number of circumstances where these were
not getting reset properly, leading to some destroyed mutexes having
holders of the last thread which locked them, and some with "-1",
which meant that the next call to "mutex_lock" just behaved as if
the lock was still valid (!), and so the unlucky caller would deadlock
forever.
Now we properly reset these fields, which means from now on attempts to
lock or unlock destroyed mutexes will lead to "PANIC: uninitialized mutex"
on KDEBUG kernels, and (as before) an infinite deadlock on non-KDEBUG
kernels (perhaps we should store the thread_id of the locker on non-KDEBUG
kernels also?).
As the next commits will show, this already uncovered a number of bugs,
and there are of course potentially more strange deadlocks caused by this.
f1f04fa6d4da: network/stack: Correct deinitialization order.
DeleteChains() needs the chain locks and domains, so those need to
be uninitialized after them. This now matches the constructor's
deinitialization order.
Fixes a panic exposed by the previous commit.
498bd544a4b4: freebsd11_network: Fix race condition leading to lock of deleted
mutex.
* Initialize "status" to B_NO_INIT, which will skip the main 'if'
the first go-around and go straight to the acquire_sem_etc(),
as we will have be invoked from the callout initializer, and so
there will of course be no callouts.
* Actually check the return code of mutex_lock, and do another loop
iteration (which skips this main 'if' as status will not be one
of those things.)
* Correct failure deinitialization order in init_callout().
* Destroy the mutex after the worker thread exits (this is the real fix.)
Fixes #14660, and other "hang on cursor" / "hang on black screen" /
or possibly even a "hang on rocket" introduced in yesterday's builds.
[ Augustin Cavalier <waddlesplash@xxxxxxxxx> ]
----------------------------------------------------------------------------
3 files changed, 27 insertions(+), 31 deletions(-)
src/add-ons/kernel/network/stack/stack.cpp | 15 +++++++-------
src/libs/compat/freebsd11_network/callout.cpp | 19 +++++++++---------
src/system/kernel/locks/lock.cpp | 24 +++++++++--------------
############################################################################
Commit: adb4e6e8c5f5fbf0638ade3ef1b7baef7c8e4cef
URL: https://git.haiku-os.org/haiku/commit/?id=adb4e6e8c5f5
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Mon Oct 29 04:38:30 2018 UTC
kernel: Reset lock holder and count on mutex_destroy().
Previously, there were a number of circumstances where these were
not getting reset properly, leading to some destroyed mutexes having
holders of the last thread which locked them, and some with "-1",
which meant that the next call to "mutex_lock" just behaved as if
the lock was still valid (!), and so the unlucky caller would deadlock
forever.
Now we properly reset these fields, which means from now on attempts to
lock or unlock destroyed mutexes will lead to "PANIC: uninitialized mutex"
on KDEBUG kernels, and (as before) an infinite deadlock on non-KDEBUG
kernels (perhaps we should store the thread_id of the locker on non-KDEBUG
kernels also?).
As the next commits will show, this already uncovered a number of bugs,
and there are of course potentially more strange deadlocks caused by this.
----------------------------------------------------------------------------
diff --git a/src/system/kernel/locks/lock.cpp b/src/system/kernel/locks/lock.cpp
index 2108874f53..f8381a9aa7 100644
--- a/src/system/kernel/locks/lock.cpp
+++ b/src/system/kernel/locks/lock.cpp
@@ -572,19 +572,7 @@ dump_rw_lock_info(int argc, char** argv)
void
mutex_init(mutex* lock, const char *name)
{
- lock->name = name;
- lock->waiters = NULL;
- B_INITIALIZE_SPINLOCK(&lock->lock);
-#if KDEBUG
- lock->holder = -1;
-#else
- lock->count = 0;
- lock->ignore_unlock_count = 0;
-#endif
- lock->flags = 0;
-
- T_SCHEDULING_ANALYSIS(InitMutex(lock, name));
- NotifyWaitObjectListeners(&WaitObjectListener::MutexInitialized, lock);
+ mutex_init_etc(lock, name, 0);
}
@@ -618,7 +606,7 @@ mutex_destroy(mutex* lock)
#if KDEBUG
if (lock->waiters != NULL && thread_get_current_thread_id()
- != lock->holder) {
+ != lock->holder) {
panic("mutex_destroy(): there are blocking threads, but caller
doesn't "
"hold the lock (%p)", lock);
if (_mutex_lock(lock, &locker) != B_OK)
@@ -636,6 +624,12 @@ mutex_destroy(mutex* lock)
}
lock->name = NULL;
+ lock->flags = 0;
+#if KDEBUG
+ lock->holder = 0;
+#else
+ lock->count = INT16_MIN;
+#endif
locker.Unlock();
@@ -717,7 +711,7 @@ _mutex_lock(mutex* lock, void* _locker)
panic("_mutex_lock(): double lock of %p by thread %" B_PRId32,
lock,
lock->holder);
} else if (lock->holder == 0)
- panic("_mutex_lock(): using unitialized lock %p", lock);
+ panic("_mutex_lock(): using uninitialized lock %p", lock);
#else
if ((lock->flags & MUTEX_FLAG_RELEASED) != 0) {
lock->flags &= ~MUTEX_FLAG_RELEASED;
############################################################################
Commit: f1f04fa6d4da0cf5598028743fb844442bb7e7d6
URL: https://git.haiku-os.org/haiku/commit/?id=f1f04fa6d4da
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Mon Oct 29 04:45:10 2018 UTC
network/stack: Correct deinitialization order.
DeleteChains() needs the chain locks and domains, so those need to
be uninitialized after them. This now matches the constructor's
deinitialization order.
Fixes a panic exposed by the previous commit.
----------------------------------------------------------------------------
diff --git a/src/add-ons/kernel/network/stack/stack.cpp
b/src/add-ons/kernel/network/stack/stack.cpp
index 0e7a22c28d..83a8e66d49 100644
--- a/src/add-ons/kernel/network/stack/stack.cpp
+++ b/src/add-ons/kernel/network/stack/stack.cpp
@@ -877,21 +877,22 @@ uninit_stack()
TRACE(("Unloading network stack\n"));
put_module(NET_SOCKET_MODULE_NAME);
- uninit_timers();
- uninit_device_interfaces();
- uninit_interfaces();
- uninit_domains();
uninit_notifications();
- mutex_destroy(&sChainLock);
- mutex_destroy(&sInitializeChainLock);
-
// remove chains and families
chain::DeleteChains(sProtocolChains);
chain::DeleteChains(sDatalinkProtocolChains);
chain::DeleteChains(sReceivingProtocolChains);
+ mutex_destroy(&sChainLock);
+ mutex_destroy(&sInitializeChainLock);
+
+ uninit_timers();
+ uninit_device_interfaces();
+ uninit_interfaces();
+ uninit_domains();
+
struct family* current;
current = sFamilies->Clear(true);
while (current) {
############################################################################
Revision: hrev52454
Commit: 498bd544a4b4bbedeb178dd0200821d95be52592
URL: https://git.haiku-os.org/haiku/commit/?id=498bd544a4b4
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Mon Oct 29 04:47:29 2018 UTC
Ticket: https://dev.haiku-os.org/ticket/14660
freebsd11_network: Fix race condition leading to lock of deleted mutex.
* Initialize "status" to B_NO_INIT, which will skip the main 'if'
the first go-around and go straight to the acquire_sem_etc(),
as we will have be invoked from the callout initializer, and so
there will of course be no callouts.
* Actually check the return code of mutex_lock, and do another loop
iteration (which skips this main 'if' as status will not be one
of those things.)
* Correct failure deinitialization order in init_callout().
* Destroy the mutex after the worker thread exits (this is the real fix.)
Fixes #14660, and other "hang on cursor" / "hang on black screen" /
or possibly even a "hang on rocket" introduced in yesterday's builds.
----------------------------------------------------------------------------
diff --git a/src/libs/compat/freebsd11_network/callout.cpp
b/src/libs/compat/freebsd11_network/callout.cpp
index 7f2315ca3e..681f50fcfb 100644
--- a/src/libs/compat/freebsd11_network/callout.cpp
+++ b/src/libs/compat/freebsd11_network/callout.cpp
@@ -37,14 +37,15 @@ static bigtime_t sTimeout;
static status_t
callout_thread(void* /*data*/)
{
- status_t status = B_OK;
+ status_t status = B_NO_INIT;
do {
bigtime_t timeout = B_INFINITE_TIMEOUT;
if (status == B_TIMED_OUT || status == B_OK) {
// scan timers for new timeout and/or execute a timer
- mutex_lock(&sLock);
+ if ((status = mutex_lock(&sLock)) != B_OK)
+ continue;
struct callout* c = NULL;
while (true) {
@@ -71,7 +72,8 @@ callout_thread(void* /*data*/)
&& (c->c_flags &
CALLOUT_RETURNUNLOCKED) == 0)
mtx_unlock(mutex);
- mutex_lock(&sLock);
+ if ((status = mutex_lock(&sLock)) !=
B_OK)
+ continue;
sCurrentCallout = NULL;
c = NULL;
@@ -127,10 +129,10 @@ init_callout(void)
return resume_thread(sThread);
-err1:
- mutex_destroy(&sLock);
err2:
delete_sem(sWaitSem);
+err1:
+ mutex_destroy(&sLock);
return status;
}
@@ -139,12 +141,11 @@ void
uninit_callout(void)
{
delete_sem(sWaitSem);
- mutex_lock(&sLock);
- mutex_destroy(&sLock);
+ wait_for_thread(sThread, NULL);
- status_t status;
- wait_for_thread(sThread, &status);
+ mutex_lock(&sLock);
+ mutex_destroy(&sLock);
}
