From 237d2bff3adbdae0c46c750697bd825b8df8eafb Mon Sep 17 00:00:00 2001
From: Norman Feske <norman.feske@genode-labs.com>
Date: Tue, 29 Jan 2019 15:26:17 +0100
Subject: [PATCH] base: fix deadlock during signal-context dissolve

This patch moves the removal of the signal context from the
'_platform_finish_dissolve' to the '_platform_begin_dissolve'
method. This is needed because the removal involves taking
the signal-registry lock. The latter must adhere the same
locking order as the code path used for signal delivery.

Fixes #3109
---
 repos/base-hw/src/lib/base/signal_receiver.cc |  7 ++++--
 repos/base/src/lib/base/signal.cc             | 13 +++++++---
 repos/base/src/lib/base/signal_common.cc      | 24 ++++++++++++++-----
 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/repos/base-hw/src/lib/base/signal_receiver.cc b/repos/base-hw/src/lib/base/signal_receiver.cc
index f63a33b8e..179318231 100644
--- a/repos/base-hw/src/lib/base/signal_receiver.cc
+++ b/repos/base-hw/src/lib/base/signal_receiver.cc
@@ -82,8 +82,11 @@ void Signal_receiver::_platform_begin_dissolve(Signal_context * const c)
 	 * from taking the lock, and set an invalid context to prevent further
 	 * processing
 	 */
-	c->_pending     = true;
-	c->_curr_signal = Signal::Data(nullptr, 0);
+	{
+		Lock::Guard context_guard(c->_lock);
+		c->_pending     = true;
+		c->_curr_signal = Signal::Data(nullptr, 0);
+	}
 	Kernel::kill_signal_context(Capability_space::capid(c->_cap));
 }
 
diff --git a/repos/base/src/lib/base/signal.cc b/repos/base/src/lib/base/signal.cc
index e0aa979b7..242753e1e 100644
--- a/repos/base/src/lib/base/signal.cc
+++ b/repos/base/src/lib/base/signal.cc
@@ -318,11 +318,18 @@ void Signal_receiver::dispatch_signals(Signal_source *signal_source)
 }
 
 
-void Signal_receiver::_platform_begin_dissolve(Signal_context *) { }
+void Signal_receiver::_platform_begin_dissolve(Signal_context *context)
+{
+	/*
+	 * Because the 'remove' operation takes the registry lock, the context
+	 * must not be locked when calling this method. See the comment in
+	 * 'Signal_receiver::dissolve'.
+	 */
+	signal_context_registry()->remove(&context->_registry_le);
+}
 
 
-void Signal_receiver::_platform_finish_dissolve(Signal_context * const c) {
-	signal_context_registry()->remove(&c->_registry_le); }
+void Signal_receiver::_platform_finish_dissolve(Signal_context *) { }
 
 
 void Signal_receiver::_platform_destructor() { }
diff --git a/repos/base/src/lib/base/signal_common.cc b/repos/base/src/lib/base/signal_common.cc
index aa3a25b23..1e4d61ab4 100644
--- a/repos/base/src/lib/base/signal_common.cc
+++ b/repos/base/src/lib/base/signal_common.cc
@@ -200,8 +200,11 @@ Signal_receiver::~Signal_receiver()
 	Lock::Guard contexts_lock_guard(_contexts_lock);
 
 	/* disassociate contexts from the receiver */
-	while (_contexts.head())
-		_unsynchronized_dissolve(_contexts.head());
+	while (Signal_context *context = _contexts.head()) {
+		_platform_begin_dissolve(context);
+		_unsynchronized_dissolve(context);
+		_platform_finish_dissolve(context);
+	}
 
 	_platform_destructor();
 }
@@ -209,8 +212,6 @@ Signal_receiver::~Signal_receiver()
 
 void Signal_receiver::_unsynchronized_dissolve(Signal_context * const context)
 {
-	_platform_begin_dissolve(context);
-
 	/* tell core to stop sending signals referring to the context */
 	env_deprecated()->pd_session()->free_context(context->_cap);
 
@@ -220,8 +221,6 @@ void Signal_receiver::_unsynchronized_dissolve(Signal_context * const context)
 
 	/* remove context from context list */
 	_contexts.remove(context);
-
-	_platform_finish_dissolve(context);
 }
 
 
@@ -231,13 +230,26 @@ void Signal_receiver::dissolve(Signal_context *context)
 		throw Context_not_associated();
 
 	{
+		/*
+		 * We must adhere to the following lock-taking order:
+		 *
+		 * 1. Taking the lock for the list of contexts ('_contexts_lock')
+		 * 2. Taking the context-registry lock (this happens inside
+		 *    '_platform_begin_dissolve' on platforms that use such a
+		 *    registry)
+		 * 3. Taking the lock for an individual signal context
+		 */
 		Lock::Guard contexts_lock_guard(_contexts_lock);
 
+		_platform_begin_dissolve(context);
+
 		Lock::Guard context_lock_guard(context->_lock);
 
 		_unsynchronized_dissolve(context);
 	}
 
+	_platform_finish_dissolve(context);
+
 	Lock::Guard context_destroy_lock_guard(context->_destroy_lock);
 }