hal: don't free a funct entry the running thread may still use (delf/unloadrt crash)

src/hal/hal_lib.c

@@ -162,6 +162,8 @@ static void free_oldname_struct(hal_oldname_t * oldname);
 static void free_funct_struct(hal_funct_t * funct);
 #endif /* RTAPI */
 static void free_funct_entry_struct(hal_funct_entry_t * funct_entry);
+static hal_list_t *funct_entry_unlink(hal_list_t * entry);
+static int thread_wait_quiescent(hal_thread_t * thread);
 #ifdef RTAPI
 static void free_thread_struct(hal_thread_t * thread);
 #endif /* RTAPI */
@@ -2642,7 +2644,18 @@ int hal_del_funct_from_thread(const char *funct_name, const char *thread_name)
 	funct_entry = (hal_funct_entry_t *) list_entry;
 	if (SHMPTR(funct_entry->funct_ptr) == funct) {
 	    /* this funct entry points to our funct, unlink */
-	    list_remove_entry(list_entry);
+	    funct_entry_unlink(list_entry);
+	    /* the realtime thread walks the list without the mutex; it may
+	       be standing on this entry right now. Don't recycle the entry
+	       (or let the caller unload the code) until the thread has
+	       finished the pass that could still see it. */
+	    if (thread_wait_quiescent(thread) < 0) {
+		/* thread is stalled: leak the entry rather than hand the
+		   thread a recycled one */
+		funct->users--;
+		halpr_mutex_release();
+		return -ETIMEDOUT;
+	    }
 	    /* and delete it */
 	    free_funct_entry_struct(funct_entry);
 	    /* done */
@@ -3253,7 +3266,11 @@ static void thread_task(void *arg)
 	    if ( runtime > hal_get_si32(thread->maxtime)) {
 	        hal_set_si32(thread->maxtime, runtime);
 	    }
-            hal_set_sint(thread->threadbeat, ++thread->beatcnt);
+            /* publish the completed pass; release orders every read of
+               funct_list above before the new count is visible (see
+               thread_wait_quiescent()) */
+            __atomic_store_n(&thread->beatcnt, thread->beatcnt + 1, __ATOMIC_RELEASE);
+            hal_set_sint(thread->threadbeat, thread->beatcnt);
 	}
 	/* wait until next period */
 	rtapi_wait();
@@ -3850,7 +3867,13 @@ static void free_funct_struct(hal_funct_t * funct)
 		/* test it */
 		if (SHMPTR(funct_entry->funct_ptr) == funct) {
 		    /* this funct entry points to our funct, unlink */
-		    list_entry = list_remove_entry(list_entry);
+		    list_entry = funct_entry_unlink(list_entry);
+		    /* let the thread leave it before it is recycled and the
+		       code behind it is unloaded */
+		    if (thread_wait_quiescent(thread) < 0) {
+			/* stalled thread: leak the entry */
+			continue;
+		    }
 		    /* and delete it */
 		    free_funct_entry_struct(funct_entry);
 		} else {
@@ -3890,6 +3913,66 @@ static void free_funct_struct(hal_funct_t * funct)
 }
 #endif /* RTAPI */
 
+/* Unlink a funct entry from a list the realtime thread may be walking.
+   Unlike list_remove_entry() the entry keeps its own links, so a thread
+   standing on it still reaches the rest of the list instead of looping on
+   the entry. The entry must not be reused before thread_wait_quiescent().
+   Returns the next entry. */
+static hal_list_t *funct_entry_unlink(hal_list_t * entry)
+{
+    hal_list_t *prev, *next;
+
+    prev = SHMPTR(entry->prev);
+    next = SHMPTR(entry->next);
+    prev->next = entry->next;
+    next->prev = entry->prev;
+    return next;
+}
+
+/* Number of thread periods to wait for a running thread to finish the
+   pass that may still reference an unlinked funct entry. */
+#define HAL_QUIESCE_PERIODS 1000
+
+/* Called with the HAL mutex held, after a funct entry was unlinked from
+   'thread'. The realtime thread walks its funct_list without the mutex,
+   so it may still hold a pointer to the unlinked entry. Wait until the
+   thread completed two more passes (any pass that started before the
+   unlink has ended) before the entry may be freed. Returns 0 when it is
+   safe (or threads are not running), -ETIMEDOUT if the thread stalled. */
+static int thread_wait_quiescent(hal_thread_t * thread)
+{
+    rtapi_sint start, now;
+    long step;
+    long long waited, limit;
+
+    if (hal_data->threads_running == 0) {
+	/* thread is not walking the list */
+	return 0;
+    }
+    /* order the unlink stores before reading the pass counter */
+    __atomic_thread_fence(__ATOMIC_SEQ_CST);
+    start = __atomic_load_n(&thread->beatcnt, __ATOMIC_ACQUIRE);
+    step = thread->period / 4;
+    if (step > rtapi_delay_max()) {
+	step = rtapi_delay_max();
+    }
+    if (step < 1) {
+	step = 1;
+    }
+    limit = (long long)thread->period * HAL_QUIESCE_PERIODS;
+    for (waited = 0; waited < limit; waited += step) {
+	now = __atomic_load_n(&thread->beatcnt, __ATOMIC_ACQUIRE);
+	if (now - start >= 2) {
+	    return 0;
+	}
+	rtapi_delay(step);
+    }
+    rtapi_print_msg(RTAPI_MSG_ERR,
+	"HAL: ERROR: thread '%s' did not complete a pass within %d periods,"
+	" funct entry not freed\n", thread->name, HAL_QUIESCE_PERIODS);
+    return -ETIMEDOUT;
+}
+
 static void free_funct_entry_struct(hal_funct_entry_t * funct_entry)
 {
     hal_funct_t *funct;

tests/hal-delf-live/README

@@ -0,0 +1,14 @@
+delf / unloadrt of a function while its thread is running.
+
+The realtime thread walks its funct_list without the HAL mutex. Without
+quiescence, hal_del_funct_from_thread() unlinks the entry (its links then
+point to itself) and recycles it at once; a thread that is inside the
+entry keeps calling it forever or follows a recycled link, and the
+following unloadrt dlclose()s code the thread may still execute. The
+result is a hung thread or a crashed rtapi_app.
+
+The test loads a component whose function busy-waits a fixed time, so the
+thread spends a large share of each period on the entry being removed, and
+repeats loadrt -> addf -> delf -> unloadrt DELF_ITER times (default 200)
+with threads started. It passes when halrun finishes in time and the
+thread still beats afterwards.

tests/hal-delf-live/beat-check.sh

@@ -0,0 +1,7 @@
+#!/bin/sh
+# exit 0 when thread 't' is still executing passes
+a=$(halcmd getp t.threadbeat)
+sleep 0.1
+b=$(halcmd getp t.threadbeat)
+echo "threadbeat $a -> $b"
+[ "$b" -gt "$a" ]

tests/hal-delf-live/checkresult

@@ -0,0 +1,3 @@
+#!/bin/sh
+# success or failure is determined by the return value of the test.sh script
+exit 0

tests/hal-delf-live/control

@@ -0,0 +1 @@
+Restrictions: sudo

tests/hal-delf-live/delfvictim.comp

@@ -0,0 +1,12 @@
+component delfvictim "Test component for tests/hal-delf-live: burns spin_ns in its function";
+
+pin in bit dummy;
+param rw uint spin_ns = 20000;
+
+function _;
+license "GPL";
+author "SyncTwin";
+;;
+long long t0 = rtapi_get_time();
+(void)period;
+while (rtapi_get_time() - t0 < (long long)spin_ns) { }

tests/hal-delf-live/test.sh

@@ -0,0 +1,36 @@
+#!/bin/bash
+# Repeatedly remove a function from a running thread and unload its
+# component. Without thread quiescence in hal_del_funct_from_thread() this
+# hangs or crashes rtapi_app (probabilistically, usually in the first tens
+# of iterations).
+set -e
+${SUDO} halcompile --install delfvictim.comp
+
+N=${DELF_ITER:-200}
+TMPDIR=$(mktemp -d /tmp/hal-delf-live.XXXXXX)
+trap 'rm -rf "$TMPDIR"' 0 1 2 3 15
+HAL="$TMPDIR/loop.hal"
+
+{
+    echo "loadrt threads name1=t period1=100000"
+    echo "start"
+    for _ in $(seq "$N"); do
+        echo "loadrt delfvictim names=v"
+        echo "addf v t"
+        echo "loadusr -w sleep 0.002"
+        echo "delf v t"
+        echo "unloadrt delfvictim"
+    done
+    # the thread must still be beating after the loop
+    echo "loadusr -w $(pwd)/beat-check.sh"
+} > "$HAL"
+
+set +e
+timeout -k 5 $((N / 2 + 60)) halrun -f "$HAL"
+RES=$?
+set -e
+if [ $RES -ne 0 ]; then
+    echo "FAIL: halrun exited with $RES after delf/unloadrt in a running thread"
+    exit 1
+fi
+echo "PASS: $N x loadrt/addf/delf/unloadrt with running thread"