diff --git a/Makefile b/Makefile index 18937cf..e07fef7 100644 --- a/Makefile +++ b/Makefile @@ -218,6 +218,12 @@ $(BUILD_DIR)/test-fault-signal-mt: tests/test-fault-signal-mt.c | $(BUILD_DIR) @echo " CROSS $< (with -lpthread)" $(Q)$(CROSS_COMPILE)gcc -D_GNU_SOURCE -static -O2 -o $@ $< -lpthread +# test-exit-group-worker has a non-main pthread issue exit_group while +# spinner threads hammer memory, guarding the join-before-teardown order. +$(BUILD_DIR)/test-exit-group-worker: tests/test-exit-group-worker.c | $(BUILD_DIR) + @echo " CROSS $< (with -lpthread)" + $(Q)$(CROSS_COMPILE)gcc -D_GNU_SOURCE -static -O2 -o $@ $< -lpthread + # test-shim-cred-race spawns a pthread reader while the main thread # toggles setresuid; the reader spins on the identity fast path. $(BUILD_DIR)/test-shim-cred-race: tests/test-shim-cred-race.c | $(BUILD_DIR) diff --git a/src/core/guest.c b/src/core/guest.c index a0ebde7..cf3b4fa 100644 --- a/src/core/guest.c +++ b/src/core/guest.c @@ -602,12 +602,14 @@ void guest_destroy(guest_t *g) * leave hv_vcpu_run. A worker still inside the guest at unmap time takes a * stage-2 translation fault on its next instruction fetch and surfaces as * "unexpected exception EC=0x20" in the crash report. The foot terminal - * reproduction tripped exactly that race. The exit_group syscall handler - * already runs request, interrupt, and join before its own teardown; the - * destroy path needs the same prefix because forkipc.c:vcpu_run_loop + * reproduction tripped exactly that race. On the exit_group path the + * syscall handler runs request and interrupt, and main() joins the workers + * after its run loop returns; the destroy path needs the full + * request-interrupt-join sequence here because forkipc.c:vcpu_run_loop * returns straight into guest_destroy without going through the guest - * exit_group handler. The request is guarded on the prior state so a - * process that already chose its exit code keeps it intact. + * exit_group handler or main()'s teardown. The request is guarded on the + * prior state so a process that already chose its exit code keeps it + * intact. * * The wake signals cover workers blocked outside hv_vcpu_run: futex waiters * poll futex_interrupt_requested, and any thread parked in epoll or poll diff --git a/src/main.c b/src/main.c index 5629043..b64fade 100644 --- a/src/main.c +++ b/src/main.c @@ -36,6 +36,7 @@ #include "runtime/forkipc.h" #include "runtime/proctitle.h" +#include "runtime/thread.h" #include "syscall/fuse.h" #include "syscall/path.h" @@ -667,9 +668,22 @@ int main(int argc, char **argv) */ int exit_code = vcpu_run_loop(vcpu, vexit, &g, verbose, timeout_sec); - /* Tear down debugger state before freeing guest/vCPU resources. */ + /* Tear down debugger state before joining workers: a worker parked in + * gdb_stub_handle_stop() stays active (not deactivated) until this + * broadcasts resume_cond, so joining first would just time out and + * detach it while it is still paused. */ gdb_stub_shutdown(); + /* Wait for worker vCPU threads to stop before tearing down guest memory. + * The main thread leaves the run loop as soon as it observes the + * exit_group flag, but sibling vCPU threads may still be mid-iteration in + * their own run loops (e.g. touching shim_globals). cleanup_main_resources + * unmaps the guest slab via guest_destroy, so a still-running worker would + * fault on freed guest memory and crash the host with SIGSEGV, masking the + * real exit code. thread_join_workers() is a no-op once the workers have + * already wound down (the common single-threaded case). */ + thread_join_workers(); + /* Diagnostic counter dump runs before guest_destroy so the shim_data * mapping is still valid. ELFUSE_SHIM_STATS is the gate; an unset variable * produces no output. diff --git a/src/runtime/forkipc.c b/src/runtime/forkipc.c index 686b652..387dda2 100644 --- a/src/runtime/forkipc.c +++ b/src/runtime/forkipc.c @@ -687,9 +687,9 @@ static int64_t sys_clone_thread(hv_vcpu_t parent_vcpu, return -LINUX_EFAULT; } - /* Create the host pthread (joinable; exit_group joins all workers via - * thread_join_workers_cb before process exit). Threads clean up their TID - * address via CLONE_CHILD_CLEARTID + futex wake. + /* Create the host pthread (joinable; the main thread joins all workers + * via thread_join_workers before guest teardown). Threads clean up their + * TID address via CLONE_CHILD_CLEARTID + futex wake. */ pthread_t host_thread; pthread_attr_t attr; diff --git a/src/runtime/futex.c b/src/runtime/futex.c index ded2099..a3e1662 100644 --- a/src/runtime/futex.c +++ b/src/runtime/futex.c @@ -370,6 +370,27 @@ static uint64_t futex_remaining_ns(const struct timespec *deadline, return rem < cap_ns ? rem : cap_ns; } +/* Absolute CLOCK_REALTIME wait target one bounded quantum from now: the guest + * deadline capped at FUTEX_OS_SYNC_POLL_CAP_NS. Every condvar sleep in the + * timed-wait paths goes through this so a waiter parked on a distant guest + * deadline still re-checks the process-wide teardown flags + * (proc_exit_group_requested / futex_interrupt) each quantum; an uncapped + * sleep would outlive thread_join_workers' poll cap and race + * guest_destroy's unmap of the memory the waiter touches on wake. + * Returns false when the guest deadline has already passed. + */ +static bool futex_quantum_deadline(const struct timespec *deadline, + struct timespec *out) +{ + uint64_t rem_ns = futex_remaining_ns(deadline, FUTEX_OS_SYNC_POLL_CAP_NS); + if (rem_ns == 0) + return false; + clock_gettime(CLOCK_REALTIME, out); + out->tv_nsec += (long) rem_ns; + timespec_normalize(out); + return true; +} + #if ELFUSE_HAVE_OS_SYNC_WAIT_ON_ADDRESS /* Wake up to budget kernel-side waiters at uaddr via @@ -629,12 +650,23 @@ static int64_t futex_wait(guest_t *g, while (!__atomic_load_n(&waiter.woken, __ATOMIC_ACQUIRE)) { if (has_timeout) { - int rc = pthread_cond_timedwait(&waiter.cond, &b->lock, &deadline); - if (rc != 0) { - /* Timeout (ETIMEDOUT) or error; stop waiting */ + /* Sleep in bounded quanta rather than to the guest deadline: a + * worker parked here for a long guest timeout (JVM parkNanos, + * sem_timedwait) would otherwise be unreachable by exit_group / + * futex_interrupt, outlive thread_join_workers' poll cap, and + * race guest_destroy's unmap. The interrupt checks mirror the + * no-timeout branch below. + */ + struct timespec quantum = {0}; + if (!futex_quantum_deadline(&deadline, &quantum)) { ret = -LINUX_ETIMEDOUT; break; } + pthread_cond_timedwait(&waiter.cond, &b->lock, &quantum); + if (proc_exit_group_requested() || futex_interrupt_consume()) { + ret = -LINUX_EINTR; + break; + } continue; } @@ -1173,10 +1205,22 @@ static int64_t futex_lock_pi(guest_t *g, uint64_t uaddr, uint64_t timeout_gva) bool owner_died = false; while (!__atomic_load_n(&waiter.woken, __ATOMIC_ACQUIRE)) { if (has_timeout) { - int rc = - pthread_cond_timedwait(&waiter.cond, &b->lock, &deadline); - if (rc != 0 && - !__atomic_load_n(&waiter.woken, __ATOMIC_ACQUIRE)) { + /* Bounded quanta for the same teardown-reachability reason as + * futex_wait: never sleep to a distant guest deadline. + */ + struct timespec quantum = {0}; + bool expired = !futex_quantum_deadline(&deadline, &quantum); + if (!expired) { + pthread_cond_timedwait(&waiter.cond, &b->lock, &quantum); + if (!__atomic_load_n(&waiter.woken, __ATOMIC_ACQUIRE) && + proc_exit_group_requested()) { + /* Mirror the no-timeout exit_group path below. */ + bucket_unlink_locked(b, &waiter); + pthread_mutex_unlock(&b->lock); + pthread_cond_destroy(&waiter.cond); + return -LINUX_EINTR; + } + } else if (!__atomic_load_n(&waiter.woken, __ATOMIC_ACQUIRE)) { /* Timeout: dequeue and return */ bucket_unlink_locked(b, &waiter); /* Only clear WAITERS bit if no waiters for this address */ @@ -1643,9 +1687,11 @@ int64_t sys_futex_waitv(guest_t *g, pthread_mutex_unlock(&bucket_ptrs[i]->lock); /* All enqueued. Block on shared.cond until any wake site signals it. The - * bounded sleep (capped at 500ms or the user deadline, whichever is sooner) - * gives proc_exit_group_requested() and timeout checks a chance to run if - * the cond_signal never arrives. + * bounded sleep (capped at 100 ms or the user deadline, whichever is + * sooner) gives proc_exit_group_requested() and timeout checks a chance to + * run if the cond_signal never arrives. The cap matches the other futex + * wait paths so every futex-parked worker re-checks teardown flags well + * inside thread_join_workers' poll cap. */ int result_idx = -1; pthread_mutex_lock(&shared.lock); @@ -1665,7 +1711,7 @@ int64_t sys_futex_waitv(guest_t *g, } struct timespec wait_ts; - timespec_deadline_in_ms(&wait_ts, 500); + timespec_deadline_in_ms(&wait_ts, 100); if (has_timeout) { if (deadline.tv_sec < wait_ts.tv_sec || (deadline.tv_sec == wait_ts.tv_sec && diff --git a/src/runtime/thread.c b/src/runtime/thread.c index 64b0c3a..3e54163 100644 --- a/src/runtime/thread.c +++ b/src/runtime/thread.c @@ -112,7 +112,11 @@ void thread_register_main(hv_vcpu_t vcpu, t->clear_child_tid = 0; t->sp_el1 = sp_el1; t->sp_el1_slot = 0; /* Main thread always owns slot 0 */ - t->active = 1; + /* All writes to active are atomic releases: thread_join_workers and the + * lock-free tid_alive path acquire-load it without thread_lock, so the + * store/load pair is the only happens-before edge those readers get. + */ + __atomic_store_n(&t->active, 1, __ATOMIC_RELEASE); t->altstack_flags = LINUX_SS_DISABLE; t->on_altstack = false; thread_ptrace_init(t); @@ -153,7 +157,7 @@ thread_entry_t *thread_alloc(int64_t tid, t->stack_map_start = stack_start; t->stack_map_end = stack_end; } - t->active = 1; + __atomic_store_n(&t->active, 1, __ATOMIC_RELEASE); t->altstack_flags = LINUX_SS_DISABLE; thread_ptrace_init(t); atomic_fetch_add(&active_thread_count, 1); @@ -213,7 +217,13 @@ void thread_deactivate(thread_entry_t *t) /* Free SP_EL1 slot so it can be reused by future threads */ thread_free_sp_el1_locked(t); - t->active = 0; + /* Release store: everything this thread did -- including its last guest + * memory access -- must happen-before thread_join_workers' acquire load + * observing 0, or the joiner could green-light guest_destroy's unmap + * while stores are still in flight. The joiner polls lock-free, so the + * mutex held here provides no edge to it. + */ + __atomic_store_n(&t->active, 0, __ATOMIC_RELEASE); atomic_fetch_sub(&active_thread_count, 1); /* Destroy condvars once no waiters still reference them. A tracer woken by @@ -317,7 +327,10 @@ void thread_join_workers(void) { /* Snapshot worker threads under the lock. The code needs the host_thread * handle and a way to check the active flag without re-locking. Storing the - * table entry pointer lets the loop use __atomic_load on active. + * table entry pointer lets the loop use __atomic_load on active. Entries a + * previous pass already detached (join_abandoned) are skipped: joining or + * detaching the same pthread twice is undefined, and main()'s join is + * followed by guest_destroy's internal one. */ struct { pthread_t thr; @@ -327,7 +340,7 @@ void thread_join_workers(void) pthread_mutex_lock(&thread_lock); THREAD_FOR_EACH_ACTIVE (t) { - if (t == current_thread) + if (t == current_thread || t->join_abandoned) continue; workers[nworkers].thr = t->host_thread; workers[nworkers].t = t; @@ -335,25 +348,43 @@ void thread_join_workers(void) } pthread_mutex_unlock(&thread_lock); - /* Poll/join each worker OUTSIDE the lock. Workers that responded to - * hv_vcpus_exit typically finish within microseconds. Threads stuck in - * uninterruptible host calls (blocking read/poll) are given 100ms to - * finish. If still alive, detach and let process exit clean up. The vCPU is - * NOT destroyed here because HVF vCPUs are thread-affine, so cross-thread - * hv_vcpu_destroy while the owning thread may still be inside hv_vcpu_run - * is unsafe. + /* Poll under one shared 500ms deadline OUTSIDE the lock, then join or + * detach each worker. Workers that responded to hv_vcpus_exit typically + * finish within microseconds; the cap only matters for workers parked in + * host blocking calls, and it must exceed the worst-case teardown-flag + * re-check latency of every such state or a worker that WOULD wind down + * cleanly gets abandoned on timing alone: the interruptible io wait + * re-checks every 200ms (io.c wait helper) and the futex paths every 100ms + * (FUTEX_OS_SYNC_POLL_CAP_NS quanta), so 500ms covers the slowest bound + * with margin. One shared deadline keeps worst-case shutdown at the cap + * even with many stuck workers. A worker still alive past the cap is + * detached, marked join_abandoned, and left for process exit to clean up. + * The vCPU is NOT destroyed here because HVF vCPUs are thread-affine, so + * cross-thread hv_vcpu_destroy while the owning thread may still be inside + * hv_vcpu_run is unsafe. */ - for (int w = 0; w < nworkers; w++) { - for (int i = 0; i < 20; i++) { - if (!__atomic_load_n(&workers[w].t->active, __ATOMIC_ACQUIRE)) + for (int i = 0; i < 100; i++) { + bool any_active = false; + for (int w = 0; w < nworkers; w++) { + if (__atomic_load_n(&workers[w].t->active, __ATOMIC_ACQUIRE)) { + any_active = true; break; - usleep(5000); + } } + if (!any_active) + break; + usleep(5000); + } - if (!__atomic_load_n(&workers[w].t->active, __ATOMIC_ACQUIRE)) + for (int w = 0; w < nworkers; w++) { + if (!__atomic_load_n(&workers[w].t->active, __ATOMIC_ACQUIRE)) { pthread_join(workers[w].thr, NULL); - else + } else { pthread_detach(workers[w].thr); + pthread_mutex_lock(&thread_lock); + workers[w].t->join_abandoned = true; + pthread_mutex_unlock(&thread_lock); + } } } @@ -366,7 +397,7 @@ void thread_destroy_all_vcpus(void) hv_vcpu_destroy(t->vcpu); t->vcpu = 0; thread_free_sp_el1_locked(t); - t->active = 0; + __atomic_store_n(&t->active, 0, __ATOMIC_RELEASE); /* Do NOT destroy condvars. Same race as thread_deactivate: a waiter * woken by an earlier broadcast may still reference the condvar. * Process is exiting, so the leak is harmless. @@ -901,7 +932,7 @@ int64_t thread_ptrace_wait(int64_t tracer_tid, * pthread_cond_wait(). */ thread_free_sp_el1_locked(t); - t->active = 0; + __atomic_store_n(&t->active, 0, __ATOMIC_RELEASE); atomic_fetch_sub(&active_thread_count, 1); t->ptrace_cleanup_pending = true; thread_ptrace_cleanup_locked(t); diff --git a/src/runtime/thread.h b/src/runtime/thread.h index f868bb2..4d90a15 100644 --- a/src/runtime/thread.h +++ b/src/runtime/thread.h @@ -47,6 +47,12 @@ typedef struct { * use __atomic_load_n on this field; the 32-bit width keeps * the access pattern predictable across architectures. */ + bool join_abandoned; /* thread_join_workers timed out on this worker + * and pthread_detach()ed it. Later join passes + * (guest_destroy runs one after main()'s) must + * skip the entry: pthread_join/pthread_detach on + * an already-detached thread is undefined. + */ /* Per-thread signal mask (POSIX requires each thread to have its own). * Initialized to the parent's mask on clone, modified via rt_sigprocmask. */ diff --git a/src/syscall/syscall.c b/src/syscall/syscall.c index 44990fc..759fe42 100644 --- a/src/syscall/syscall.c +++ b/src/syscall/syscall.c @@ -802,10 +802,17 @@ static int64_t sc_exit_group(guest_t *g, (void) x4; (void) x5; (void) verbose; + /* Request + interrupt only; do NOT join here. This handler runs on + * whichever thread issued exit_group, so a join from here would snapshot + * the main thread (slot 0, never deactivates) and detach it after the + * poll cap, and would race the main thread's own join over the same + * siblings (double pthread_join is undefined). The kicked workers wind + * down on their own; the single authoritative join is in main() after + * vcpu_run_loop returns, before guest teardown. + */ proc_request_exit_group((int) x0); wakeup_pipe_signal(); thread_for_each(thread_force_exit_cb, NULL); - thread_join_workers(); return SC_EXIT_SENTINEL | ((int) x0 & 0xFF); } diff --git a/tests/manifest.txt b/tests/manifest.txt index 6ae0dad..6647fa3 100644 --- a/tests/manifest.txt +++ b/tests/manifest.txt @@ -96,6 +96,7 @@ test-negative # diff=skip [section] Signal + thread tests test-signal-thread test-fault-signal-mt # diff=skip +test-exit-group-worker [section] Fork edge cases test-clone3 # diff=skip diff --git a/tests/test-exit-group-worker.c b/tests/test-exit-group-worker.c new file mode 100644 index 0000000..1145353 --- /dev/null +++ b/tests/test-exit-group-worker.c @@ -0,0 +1,179 @@ +/* + * exit_group issued by a non-main thread must terminate the process cleanly + * + * Copyright 2026 elfuse contributors + * SPDX-License-Identifier: Apache-2.0 + * + * Regression test for the exit_group teardown race: when a worker thread + * calls exit_group while its siblings are mid-iteration, the main thread + * must join the workers before unmapping guest memory. A regression shows + * up as the process dying of a host-level SIGSEGV (status 139) instead of + * reporting the exit_group code. + * + * Phase 1 forks N children; in each child a worker thread calls + * exit_group(42) while sibling workers occupy every blocking state a thread + * can be torn down from -- spinners hammering guest memory (in hv_vcpu_run), + * a worker parked in a timed FUTEX_WAIT_BITSET with a distant absolute + * deadline (the condvar wait path glibc's sem_timedwait and + * pthread_cond_timedwait sit in), and a worker parked in a blocking pipe + * read (the interruptible io wait). The parent verifies the child exited 42 + * (fork children tear down through guest_destroy). Phase 2 repeats the + * pattern in the top-level process itself, so this test's own exit code 0 is + * produced by a worker-initiated exit_group through main()'s teardown path. + * The parked variants regress the bounded-quantum futex waits: an uncapped + * sleep would outlive the join cap, get detached, and race the unmap. + * + * Syscalls: clone(220), exit_group(94), wait4(260), sched_yield(124), + * futex(98), read(63), pipe2(59) + */ + +#include +#include +#include +#include +#include +#include +#include + +#include "raw-syscall.h" +#include "test-harness.h" + +int passes = 0, fails = 0; + +#define FORK_ITERS 10 +#define N_SPINNERS 3 +#define N_WORKERS (N_SPINNERS + 2) /* spinners + futex parker + read parker */ + +static volatile unsigned long sink[512]; +static volatile int workers_ready; +static int futex_word; +static int park_pipe[2]; + +static void *spinner_fn(void *arg) +{ + (void) arg; + __sync_fetch_and_add(&workers_ready, 1); + /* Keep touching guest memory until exit_group tears the process down. */ + for (unsigned long i = 0;; i++) + sink[i % 512] = i; + return NULL; +} + +/* Park in a timed futex wait far in the future. FUTEX_WAIT_BITSET takes an + * absolute deadline and is what glibc timed waits issue; on elfuse it maps to + * the condvar wait path rather than the os_sync one, so this exercises the + * bounded-quantum teardown re-check. Re-arm on any spurious wake; exit_group + * is the only way out. + */ +static void *futex_parker_fn(void *arg) +{ + (void) arg; + struct timespec deadline; + clock_gettime(CLOCK_REALTIME, &deadline); + deadline.tv_sec += 600; + __sync_fetch_and_add(&workers_ready, 1); + for (;;) + raw_syscall6( + __NR_futex, (long) &futex_word, + FUTEX_WAIT_BITSET | FUTEX_PRIVATE_FLAG | FUTEX_CLOCK_REALTIME, 0, + (long) &deadline, 0, (long) FUTEX_BITSET_MATCH_ANY); + return NULL; +} + +/* Park in a blocking read on a pipe that never gets written (the write end + * stays open so the read cannot see EOF). Exercises the interruptible io + * wait's teardown re-check. + */ +static void *read_parker_fn(void *arg) +{ + (void) arg; + char c; + __sync_fetch_and_add(&workers_ready, 1); + for (;;) + (void) read(park_pipe[0], &c, 1); + return NULL; +} + +static void *killer_fn(void *arg) +{ + int code = (int) (long) arg; + while (__sync_fetch_and_add(&workers_ready, 0) < N_WORKERS) + sched_yield(); + /* Give the parkers a moment to actually enter their blocking waits (the + * ready increment happens just before the call). Not required for + * correctness -- a parker that enters its wait after the exit-group flag + * is set still notices it within one bounded quantum -- but it makes the + * test exercise the parked states rather than the entry races. + */ + usleep(20000); + syscall(SYS_exit_group, code); + return NULL; +} + +/* Spawn the worker set plus a thread that calls exit_group(code), then spin + * on the main thread. Never returns: exit_group ends the process. + */ +static void run_victim(int code) +{ + pthread_t t; + workers_ready = 0; + if (pipe(park_pipe) != 0) + syscall(SYS_exit_group, 99); + for (int i = 0; i < N_SPINNERS; i++) { + if (pthread_create(&t, NULL, spinner_fn, NULL) != 0) + syscall(SYS_exit_group, 99); + } + if (pthread_create(&t, NULL, futex_parker_fn, NULL) != 0) + syscall(SYS_exit_group, 99); + if (pthread_create(&t, NULL, read_parker_fn, NULL) != 0) + syscall(SYS_exit_group, 99); + if (pthread_create(&t, NULL, killer_fn, (void *) (long) code) != 0) + syscall(SYS_exit_group, 99); + for (;;) + sched_yield(); +} + +int main(void) +{ + TEST("exit_group from worker thread (forked children)"); + + int bad_status = -1; + for (int iter = 0; iter < FORK_ITERS && bad_status < 0; iter++) { + pid_t pid = fork(); + if (pid < 0) { + FAIL("fork failed"); + goto summary; + } + if (pid == 0) + run_victim(42); /* does not return */ + + int status; + if (waitpid(pid, &status, 0) != pid) { + FAIL("waitpid failed"); + goto summary; + } + if (!WIFEXITED(status) || WEXITSTATUS(status) != 42) + bad_status = status; + } + if (bad_status >= 0) { + if (WIFSIGNALED(bad_status)) + FAIL("child killed by signal (teardown race)"); + else + FAIL("child reported wrong exit code"); + } else { + PASS(); + } + +summary: + SUMMARY("test-exit-group-worker"); + if (fails) + return 1; + + /* Phase 2: our own exit code 0 must come from a worker-initiated + * exit_group in the top-level process. Flush first: exit_group does not + * flush stdio buffers. + */ + fflush(NULL); + run_victim(0); + return 1; /* unreachable */ +}