Merge pull request #48 from CHERIoT-Platform/hlefeuvre/stack-overflow-resilient-error-handler

Move to a stack-overflow-resilient error handler

Author: hlef

lib/tcpip/FreeRTOS_IP_wrapper.c

@@ -17,6 +17,7 @@ void ip_thread_start(void);
 #include <futex.h>
 #include <locks.h>
 #include <stdatomic.h>
+#include <unwind.h>
 
 /**
  * Flag used to synchronize the network stack thread and user threads at
@@ -37,11 +38,16 @@ static uint32_t threadEntryGuard;
 uint16_t networkThreadID;
 
 /**
- * Global lock acquired by the IP thread at startup time. This lock is never
- * released and acquiring it after startup will always fail. This lock can be
- * used to force the IP thread to run for a short amount of time, e.g., when
- * the internal FreeRTOS message queue is full and we want to let the IP thread
- * empty it to close a socket.
+ * Global lock acquired by the IP thread at startup time. In normal running
+ * conditions, this lock is never released and acquiring it after startup will
+ * always fail. This lock can be used to force the IP thread to run for a short
+ * amount of time, e.g., when the internal FreeRTOS message queue is full and
+ * we want to let the IP thread empty it to close a socket. See such an example
+ * in `close_socket_retry`.
+ *
+ * Corner case: during a network stack reset, the lock is shortly released to
+ * notify other threads that the IP thread is restarting. See comment in
+ * `ip_thread_entry` for more details.
  */
 struct FlagLockState ipThreadLockState;
 
@@ -106,6 +112,8 @@ void ip_cleanup(void)
 	// modifiable by a potentially compromised compartment.
 }
 
+extern void reset_network_stack_state(bool isIpThread);
+
 void __cheri_compartment("TCPIP") ip_thread_entry(void)
 {
 	FreeRTOS_printf(("ip_thread_entry\n"));
@@ -114,36 +122,53 @@ void __cheri_compartment("TCPIP") ip_thread_entry(void)
 
 	while (1)
 	{
-		while (threadEntryGuard == 0)
+		CHERIOT_DURING
 		{
+			while (threadEntryGuard == 0)
+			{
+				FreeRTOS_printf(
+				  ("Sleeping until the IP task is supposed to start\n"));
+				futex_wait(&threadEntryGuard, 0);
+			}
+
+			// Reset the guard now: we will only ever re-iterate
+			// the loop in the case of a network stack reset, in
+			// which case we want to wait again for a call to
+			// `ip_thread_start`.
+			//
+			// Note that we cannot reset this in `ip_cleanup`,
+			// because that would overwrite the 1 written by
+			// `ip_thread_start` if the latter was called before we
+			// call `ip_cleanup`. This will happen if the IP thread
+			// crashes, in which case we would first call
+			// `ip_thread_start` in the error handler, and then
+			// reset the context to `ip_thread_entry` (itself then
+			// calling `ip_cleanup`).
+			threadEntryGuard = 0;
+
+			xIPTaskHandle = networkThreadID;
 			FreeRTOS_printf(
-			  ("Sleeping until the IP task is supposed to start\n"));
-			futex_wait(&threadEntryGuard, 0);
-		}
+			  ("ip_thread_entry starting, thread ID is %p\n", xIPTaskHandle));
 
-		// Reset the guard now: we will only ever re-enter this
-		// function in the case of a network stack reset, in which case
-		// we want to wait again for a call to `ip_thread_start`.
-		//
-		// Note that we cannot reset this in `ip_cleanup`, because that
-		// would overwrite the 1 written by `ip_thread_start` if the
-		// latter was called before we call `ip_cleanup`. This will
-		// happen if the IP thread crashes, in which case we would
-		// first call `ip_thread_start` in the error handler, and then
-		// reset the context to `ip_thread_entry` (itself then calling
-		// `ip_cleanup`).
-		threadEntryGuard = 0;
-
-		xIPTaskHandle = networkThreadID;
-		FreeRTOS_printf(
-		  ("ip_thread_entry starting, thread ID is %p\n", xIPTaskHandle));
-
-		flaglock_priority_inheriting_lock(&ipThreadLockState);
-
-		// FreeRTOS event loop. This will only return if a user thread
-		// crashed.
-		prvIPTask(NULL);
+			flaglock_priority_inheriting_lock(&ipThreadLockState);
 
+			// FreeRTOS event loop. This will only return if a user
+			// thread crashed.
+			prvIPTask(NULL);
+		}
+		CHERIOT_HANDLER
+		{
+			// Call the network stack error handler for the network thread.
+			reset_network_stack_state(true /* this is the IP thread */);
+		}
+		CHERIOT_END_HANDLER
+
+		// Release the IP thread lock. We will re-acquire it in the
+		// next loop iteration. This is necessary if a user thread
+		// triggered a reset. In such a case, the user thread error
+		// handler will wait for the IP thread to restart by acquiring
+		// the `ipThreadLockState` with an infinite timeout (and
+		// immediately releasing it thereafter).
 		flaglock_unlock(&ipThreadLockState);
 	}
 }

lib/tcpip/tcpip-internal.h

@@ -5,13 +5,16 @@
 #include <FreeRTOS_IP.h>
 #include <ds/linked_list.h>
 #include <locks.hh>
+#include <unwind.h>
 
 /**
  * Internal helpers and data structures for use inside of the TCP/IP
  * compartment. These should be called or used only from/in the TCP/IP
  * compartment.
  */
 
+extern "C" void reset_network_stack_state(bool isIpThread);
+
 /**
  * Flags for the state of restart.
  */
@@ -97,30 +100,38 @@ extern std::atomic<uint8_t> userThreadCount;
 
 /**
  * Helper to run a function ensuring that the thread counters are updated
- * appropriately. Every entry point of the TCP/IP stack API (with the exception
- * of the driver thread, see below) should go through this unless it
- * manipulates `userThreadCount` manually.
+ * appropriately and that the thread is running under the error handler.  Every
+ * entry point of the TCP/IP stack API (with the exception of the driver
+ * thread, see below) should go through this unless it manipulates
+ * `userThreadCount` and sets up the error handler manually.
  */
 auto with_restarting_checks(auto operation, auto errorValue)
 {
-	if (restartState.load() != 0)
-	{
-		// yield to give a chance to the restart code to make some
-		// progress, in case applications are aggressively trying to
-		// re-open the socket.
-		yield();
-		return errorValue;
-	}
-	userThreadCount++;
-	auto ret = operation();
-	// The decrement will happen in the error handler if the thread
-	// crashes.
-	//
-	// FIXME The decrement will *not* happen when a thread runs out of call
-	// stack in the TCP/IP compartment (because the error handler does not
-	// execute in that case). This will be fixed when we enable the error
-	// handler to run on stack overflow.
-	userThreadCount--;
+	auto ret = errorValue;
+
+	on_error(
+	  [&]() {
+		  // Is a reset ongoing?
+		  if (restartState.load() == 0)
+		  {
+			  // We are not resetting.
+			  userThreadCount++;
+			  ret = operation();
+			  // The decrement will happen in the error handler if
+			  // the thread crashes.
+			  userThreadCount--;
+			  return;
+		  }
+		  // A reset is ongoing. yield to give a chance to the restart
+		  // code to make some progress, in case applications are
+		  // aggressively trying to re-open the socket.
+		  yield();
+	  },
+	  [&]() {
+		  // Call the network stack error handler.
+		  reset_network_stack_state(false /* this is not the IP thread */);
+	  });
+
 	return ret;
 }
 
@@ -135,8 +146,20 @@ auto with_restarting_checks(auto operation, auto errorValue)
  */
 auto with_restarting_checks_driver(auto operation, auto errorValue)
 {
-	userThreadCount++;
-	auto ret = operation();
-	userThreadCount--;
+	auto ret = errorValue;
+
+	on_error(
+	  [&]() {
+		  userThreadCount++;
+		  ret = operation();
+		  // The decrement will happen in the error handler if the thread
+		  // crashes.
+		  userThreadCount--;
+	  },
+	  [&]() {
+		  // Call the network stack error handler.
+		  reset_network_stack_state(false /* this is not the IP thread */);
+	  });
+
 	return ret;
 }