kernel: Properly separate and handle THREAD_BLOCK_TYPE_USER.

Consider this scenario:
 * A userland thread puts its ID into some structure so that it
   can be woken up later, sets its wait_status to initiate the
   begin of the wait, and then calls _user_block_thread.
 * A second thread finishes whatever task the first thread
   intended to wait for, reads the ID almost immediately
   after it was written, and calls _user_unblock_thread.
 * _user_unblock_thread was called so soon that the first
   thread is not yet blocked on the _user_block_thread block,
   but is instead blocked on e.g. the thread's main mutex.
 * The first thread's thread_block() call returns B_OK.
   As in this example it was inside mutex_lock, it thinks
   that it now owns the mutex.
 * But it doesn't own the mutex, and so (until yesterday)
   all sorts of mayhem and then a random crash occurs, or
   (after yesterday) an assert-failure is tripped that
   the thread does not own the mutex it expected to.

The above scenario is not a hypothetical, but is in fact the
exact scenario behind the strange panics in #15211.

The solution is to only have _user_unblock_thread actually
unblock threads that were blocked by _user_block_thread,
so I've introduced a new BLOCK_TYPE to differentiate these.
While I'm at it, remove the BLOCK_TYPE_USER_BASE, which was
never used (and now never will be.) If we want to differentiate
different consumers of _user_block_thread for debugging
purposes, we should use the currently-unused "object"
argument to thread_block, instead of cluttering the
relatively-clean block type debugging code with special
types.

One final note: The race condition which was the case of
this bug does not, in fact, imply a deadlock on the part
of the rw_lock here. The wait_status is protected by the
thread's mutex, which is acquired by both _user_block_thread
and _user_unblock_thread, and so if _user_unblock_thread
succeeds faster than _user_block_thread can initiate
the block, it will just see that wait_status is already
<= 0 and return immediately.

Fixes #15211.
This commit is contained in:
Augustin Cavalier
2019-08-05 22:31:02 -04:00
parent 64a4b621b8
commit 2c588b031f
5 changed files with 25 additions and 3 deletions
+1 -1
View File
@@ -28,9 +28,9 @@ enum {
THREAD_BLOCK_TYPE_SIGNAL = 3,
THREAD_BLOCK_TYPE_MUTEX = 4,
THREAD_BLOCK_TYPE_RW_LOCK = 5,
THREAD_BLOCK_TYPE_USER = 6,
THREAD_BLOCK_TYPE_OTHER = 9999,
THREAD_BLOCK_TYPE_USER_BASE = 10000
};
+2
View File
@@ -46,6 +46,8 @@ wait_object_type_name(uint32 type)
return "mutex";
case THREAD_BLOCK_TYPE_RW_LOCK:
return "rw lock";
case THREAD_BLOCK_TYPE_USER:
return "user";
case THREAD_BLOCK_TYPE_OTHER:
return "other";
case THREAD_BLOCK_TYPE_SNOOZE:
@@ -109,6 +109,9 @@ wait_object_to_string(scheduling_analysis_wait_object* waitObject, char* buffer,
else
sprintf(buffer, "rwlock %p (%s)", object, waitObject->name);
break;
case THREAD_BLOCK_TYPE_USER:
strcpy(buffer, "user");
break;
case THREAD_BLOCK_TYPE_OTHER:
sprintf(buffer, "other %p (%s)", object, waitObject->name);
break;
@@ -79,6 +79,9 @@ ScheduleThread::AddDump(TraceOutput& out)
case THREAD_BLOCK_TYPE_RW_LOCK:
out.Print("rwlock %p", fPreviousWaitObject);
break;
case THREAD_BLOCK_TYPE_USER:
out.Print("_user_block_thread()");
break;
case THREAD_BLOCK_TYPE_OTHER:
out.Print("other (%p)", fPreviousWaitObject);
// We could print the string, but it might come from a
+16 -2
View File
@@ -1699,6 +1699,10 @@ _dump_thread_info(Thread *thread, bool shortInfo)
kprintf("rwlock %p ", thread->wait.object);
break;
case THREAD_BLOCK_TYPE_USER:
kprintf("user%*s", B_PRINTF_POINTER_WIDTH + 11, "");
break;
case THREAD_BLOCK_TYPE_OTHER:
kprintf("other%*s", B_PRINTF_POINTER_WIDTH + 10, "");
break;
@@ -1782,6 +1786,10 @@ _dump_thread_info(Thread *thread, bool shortInfo)
kprintf("rwlock %p\n", thread->wait.object);
break;
case THREAD_BLOCK_TYPE_USER:
kprintf("user\n");
break;
case THREAD_BLOCK_TYPE_OTHER:
kprintf("other (%s)\n", (char*)thread->wait.object);
break;
@@ -2976,7 +2984,13 @@ user_unblock_thread(thread_id threadID, status_t status)
if (thread->user_thread->wait_status > 0) {
thread->user_thread->wait_status = status;
clear_ac();
thread_unblock_locked(thread, status);
// Even if the user_thread->wait_status was > 0, it may be the
// case that this thread is actually blocked on something else.
if (thread->state == B_THREAD_WAITING
&& thread->wait.type == THREAD_BLOCK_TYPE_USER) {
thread_unblock_locked(thread, status);
}
} else
clear_ac();
@@ -3715,7 +3729,7 @@ _user_block_thread(uint32 flags, bigtime_t timeout)
clear_ac();
// nope, so wait
thread_prepare_to_block(thread, flags, THREAD_BLOCK_TYPE_OTHER, "user");
thread_prepare_to_block(thread, flags, THREAD_BLOCK_TYPE_USER, NULL);
threadLocker.Unlock();