hw: fix race in signal dispatching

There was a race when the component entrypoint wanted to do
'wait_and_dispatch_one_signal'. In this function it raises a flag for
the signal proxy thread to notice that the entrypoint also wants to
block for signals. When the flag is set and the signal proxy wakes up
with a new signal, it tried to cancel the blocking of the entrypoint.
However, if the entrypoint had not reached the signal blocking at this
point, the cancel blocking failed without a solution. Now, the new
Kernel::cancel_next_signal_blocking call solves the problem by storing a
request to cancel the next signal blocking of a thread immediately
without blocking itself.

Ref #2284

repos/base-hw/include/kernel/interface.h

@@ -32,15 +32,16 @@ namespace Kernel
 	constexpr Call_arg call_id_kill_signal_context()      { return  6; }
 	constexpr Call_arg call_id_submit_signal()            { return  7; }
 	constexpr Call_arg call_id_await_signal()             { return  8; }
-	constexpr Call_arg call_id_ack_signal()           { return  9; }
+	constexpr Call_arg call_id_cancel_next_await_signal() { return  9; }
-	constexpr Call_arg call_id_print_char()           { return 10; }
+	constexpr Call_arg call_id_ack_signal()               { return 10; }
-	constexpr Call_arg call_id_update_data_region()   { return 11; }
+	constexpr Call_arg call_id_print_char()               { return 11; }
-	constexpr Call_arg call_id_update_instr_region()  { return 12; }
+	constexpr Call_arg call_id_update_data_region()       { return 12; }
-	constexpr Call_arg call_id_ack_cap()              { return 13; }
+	constexpr Call_arg call_id_update_instr_region()      { return 13; }
-	constexpr Call_arg call_id_delete_cap()           { return 14; }
+	constexpr Call_arg call_id_ack_cap()                  { return 14; }
-	constexpr Call_arg call_id_timeout()              { return 15; }
+	constexpr Call_arg call_id_delete_cap()               { return 15; }
-	constexpr Call_arg call_id_timeout_age_us()       { return 16; }
+	constexpr Call_arg call_id_timeout()                  { return 16; }
-	constexpr Call_arg call_id_timeout_max_us()       { return 17; }
+	constexpr Call_arg call_id_timeout_age_us()           { return 17; }
+	constexpr Call_arg call_id_timeout_max_us()           { return 18; }
 
 
 	/*****************************************************************
@@ -281,6 +282,22 @@ namespace Kernel
 		return call(call_id_await_signal(), receiver_id);
 	}
 
+	/**
+	 * Request to cancel the next signal blocking of a local thread
+	 *
+	 * \param thread_id  capability id of the targeted thread
+	 *
+	 * Does not block. Targeted thread must be in the same PD as the caller.
+	 * If the targeted thread is in a signal blocking, cancels the blocking
+	 * directly. Otherwise, stores the request and avoids the next signal
+	 * blocking of the targeted thread as if it was immediately cancelled.
+	 * If the target thread already holds a request, further ones get ignored.
+	 */
+	inline void cancel_next_await_signal(capid_t const thread_id)
+	{
+		call(call_id_cancel_next_await_signal(), thread_id);
+	}
+
 
 	/**
 	 * Trigger a specific signal context

repos/base-hw/src/core/include/kernel/thread.h

@@ -109,6 +109,7 @@ class Kernel::Thread
 		char const * const _label;
 		capid_t            _timeout_sigid = 0;
 		bool               _paused = false;
+		bool               _cancel_next_await_signal = false;
 
 		void _init();
 
@@ -210,6 +211,7 @@ class Kernel::Thread
 		void _call_update_instr_region();
 		void _call_print_char();
 		void _call_await_signal();
+		void _call_cancel_next_await_signal();
 		void _call_submit_signal();
 		void _call_ack_signal();
 		void _call_kill_signal_context();

repos/base-hw/src/core/kernel/thread.cc

@@ -404,6 +404,12 @@ void Thread::_call_print_char() { Kernel::log((char)user_arg_1()); }
 
 void Thread::_call_await_signal()
 {
+	/* cancel if another thread set our "cancel next await signal" bit */
+	if (_cancel_next_await_signal) {
+		user_arg_0(-1);
+		_cancel_next_await_signal = false;
+		return;
+	}
 	/* lookup receiver */
 	Signal_receiver * const r = pd()->cap_tree().find<Signal_receiver>(user_arg_1());
 	if (!r) {
@@ -422,6 +428,31 @@ void Thread::_call_await_signal()
 }
 
 
+void Thread::_call_cancel_next_await_signal()
+{
+	/* kill the caller if he has no protection domain */
+	if (!pd()) {
+		error(*this, ": PD not set");
+		_die();
+		return;
+	}
+	/* kill the caller if the capability of the target thread is invalid */
+	Thread * const thread = pd()->cap_tree().find<Thread>(user_arg_1());
+	if (!thread || pd() != thread->pd()) {
+		error(*this, ": failed to lookup thread ", (unsigned)user_arg_1());
+		_die();
+		return;
+	}
+	/* resume the target thread directly if it blocks for signals */
+	if (thread->_state == AWAITS_SIGNAL) {
+		thread->_cancel_blocking();
+		return;
+	}
+	/* if not, keep in mind to cancel its next signal blocking */
+	thread->_cancel_next_await_signal = true;
+}
+
+
 void Thread::_call_submit_signal()
 {
 	/* lookup signal context */
@@ -556,6 +587,7 @@ void Thread::_call()
 	case call_id_kill_signal_context():      _call_kill_signal_context(); return;
 	case call_id_submit_signal():            _call_submit_signal(); return;
 	case call_id_await_signal():             _call_await_signal(); return;
+	case call_id_cancel_next_await_signal(): _call_cancel_next_await_signal(); return;
 	case call_id_ack_signal():               _call_ack_signal(); return;
 	case call_id_print_char():               _call_print_char(); return;
 	case call_id_ack_cap():                  _call_ack_cap(); return;

repos/base-hw/src/lib/base/signal.cc

@@ -19,6 +19,8 @@
 #include <base/trace/events.h>
 
 /* base-internal includes */
+#include <base/internal/native_thread.h>
+#include <base/internal/lock_helper.h>
 #include <base/internal/native_utcb.h>
 #include <base/internal/native_env.h>
 #include <base/internal/capability_space.h>
@@ -135,8 +137,7 @@ void Signal_receiver::block_for_signal()
 
 void Signal_receiver::unblock_signal_waiter(Rpc_entrypoint &rpc_ep)
 {
-	/* force ep out of 'await_signal' in 'block_for_signal' */
+	Kernel::cancel_next_await_signal(native_thread_id(&rpc_ep));
-	rpc_ep.cancel_blocking();
 }
 
 

repos/base/include/base/entrypoint.h

@@ -94,6 +94,8 @@ class Genode::Entrypoint : Genode::Noncopyable
 		};
 
 		int               _signal_recipient { NONE };
+		Genode::Lock      _signal_pending_lock;
+		Genode::Lock      _signal_pending_ack_lock;
 		Post_signal_hook *_post_signal_hook = nullptr;
 
 		void _execute_post_signal_hook()

repos/base/include/base/rpc_server.h

@@ -31,6 +31,8 @@ namespace Genode {
 	class Rpc_object_base;
 	template <typename, typename> struct Rpc_object;
 	class Rpc_entrypoint;
+
+	class Signal_receiver;
 }
 
 
@@ -293,6 +295,14 @@ struct Genode::Rpc_object : Rpc_object_base, Rpc_dispatcher<RPC_INTERFACE, SERVE
  */
 class Genode::Rpc_entrypoint : Thread, public Object_pool<Rpc_object_base>
 {
+	/**
+	 * This is only needed because in 'base-hw' we need the Thread
+	 * pointer of the entrypoint to cancel its next signal blocking.
+	 * Remove it as soon as signal dispatching in 'base-hw' doesn't need
+	 * multiple threads anymore.
+	 */
+	friend class Signal_receiver;
+
 	private:
 
 		/**

repos/base/src/lib/base/entrypoint.cc

@@ -65,7 +65,11 @@ void Entrypoint::_process_incoming_signals()
 		do {
 			_sig_rec->block_for_signal();
 
-			int success = cmpxchg(&_signal_recipient, NONE, SIGNAL_PROXY);
+			int success;
+			{
+				Lock::Guard guard(_signal_pending_lock);
+				success = cmpxchg(&_signal_recipient, NONE, SIGNAL_PROXY);
+			}
 
 			/* common case, entrypoint is not in 'wait_and_dispatch_one_signal' */
 			if (success) {
@@ -86,6 +90,11 @@ void Entrypoint::_process_incoming_signals()
 				 * block for next signal
 				 */
 				_sig_rec->unblock_signal_waiter(*_rpc_ep);
+
+				/*
+				 * wait for the acknowledgment by the entrypoint
+				 */
+				_signal_pending_ack_lock.lock();
 			}
 		} while (!_suspended);
 
@@ -125,25 +134,26 @@ void Entrypoint::wait_and_dispatch_one_signal()
 	for (;;) {
 
 		try {
+			_signal_pending_lock.lock();
 
-			{
 			cmpxchg(&_signal_recipient, NONE, ENTRYPOINT);
-
 			Signal sig =_sig_rec->pending_signal();
-
 			cmpxchg(&_signal_recipient, ENTRYPOINT, NONE);
 
+			_signal_pending_lock.unlock();
+
+			_signal_pending_ack_lock.unlock();
+
 			_dispatch_signal(sig);
-			}
+			break;
-
-			_execute_post_signal_hook();
-
-			return;
 
 		} catch (Signal_receiver::Signal_not_pending) {
+			_signal_pending_lock.unlock();
 			_sig_rec->block_for_signal();
 		}
 	}
+
+	_execute_post_signal_hook();
 }