Join worker vCPU threads before tearing down guest memory#85
Conversation
b981ccc to
34bb04c
Compare
34bb04c to
53d8e7f
Compare
(A) Paths: every teardown path establishes the precondition before unmapping
gdb_stub_shutdown(); /* must precede the join: a worker parked in
gdb_stub_handle_stop() stays active until
resume_cond broadcasts */
thread_join_workers();
...
if (shim_globals_stats_enabled())
shim_globals_counters_dump(&g); /* reads guest memory — must sit
between join and destroy */
syscall_hist_dump();
cleanup_main_resources(...); /* → guest_destroy() */
if (!proc_exit_group_requested())
proc_request_exit_group(0);
futex_interrupt_request();
wakeup_pipe_signal();
thread_interrupt_all();
thread_join_workers();On the (B) the join terminates — every blocking state has a wake-up boundedbelow the join cap
The 200 ms io bound ( for (;;) {
if (proc_exit_group_requested() || futex_interrupt_consume() ||
signal_pending_interruption(NULL))
return -LINUX_EINTR;
int ret = poll(fds, 2, 200);
...The unbounded timed-futex state and its fix ( 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 */
+ struct timespec quantum;
+ 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;
}
Join cap was 100 ms per worker — below the 200 ms io bound and vacuous against 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;
}
if (!any_active)
break;
usleep(5000);
}(C) the join itself is UB-freeDouble join/detach: a worker detached by a timed-out pass stayed if (t == current_thread || t->join_abandoned) /* snapshot skip */
continue;
...
if (!__atomic_load_n(&workers[w].t->active, __ATOMIC_ACQUIRE)) {
pthread_join(workers[w].thr, NULL);
} else {
pthread_detach(workers[w].thr);
pthread_mutex_lock(&thread_lock);
workers[w].t->join_abandoned = true;
pthread_mutex_unlock(&thread_lock);
}
__atomic_store_n(&t->active, 0, __ATOMIC_RELEASE);so: every guest access happens-before the ResidualHost condvar parks outside the syscall wait set ( Evidence (necessary, not sufficient)
|
53d8e7f to
40270cd
Compare
40270cd to
37b0db1
Compare
jserv
left a comment
There was a problem hiding this comment.
Rebase latest main branch, resolve conflicts, and refine per review messages.
On exit_group the main thread leaves vcpu_run_loop as soon as it observes the exit-group flag and proceeds to cleanup_main_resources(), which unmaps the guest slab via guest_destroy(). Sibling vCPU threads may still be mid-iteration in their own run loops (e.g. in shim_globals_recompute_attention, which touches guest memory). A worker that reads the slab after the main thread frees it faults at the host level and the elfuse process dies with SIGSEGV, so a guest that requested exit_group(0) is reported as exit 139. This was masked until the fault-delivery fix let multi-threaded JVM workloads reach the exit_group teardown; javac exposed the race. Have the main thread call thread_join_workers() after vcpu_run_loop() returns and before any teardown; it is a no-op once the workers have wound down. Call gdb_stub_shutdown() before the join rather than after: a worker parked in gdb_stub_handle_stop() stays active (not deactivated) until the shutdown broadcasts resume_cond, so joining first would time out and detach it while it is still paused, reintroducing the freed-memory race whenever a GDB session is attached. Drop the thread_join_workers() call from sc_exit_group so the join in main() is the single authoritative one. That call ran on whichever thread issued exit_group, with two defects: its snapshot included the main thread (slot 0, which never deactivates), so it burned the full poll cap and then pthread_detach()ed the host process's main pthread; and it raced the main thread's join over the same siblings, letting both call pthread_join() on the same pthread (undefined per POSIX). The handler still sets the exit-group flag and kicks every sibling out of hv_vcpu_run via thread_force_exit_cb; main reaps them exactly once. The join is only as strong as the guarantee that every parked worker notices the exit-group request before the join gives up on it. Auditing each blocking state a worker can occupy against that cap found three gaps, and a ThreadSanitizer pass found a fourth: - Timed futex waits slept to the guest deadline. futex_wait's and futex_lock_pi's has_timeout branches issued one pthread_cond_timedwait to the full deadline with no teardown-flag check in between, so a worker parked in FUTEX_WAIT_BITSET with a distant deadline -- what glibc's sem_timedwait, pthread_cond_timedwait, and the JVM's parkNanos issue, and a path that bypasses the bounded os_sync route -- was unreachable by exit_group for the wait's whole duration. Both branches now sleep in quanta capped at FUTEX_OS_SYNC_POLL_CAP_NS (100ms) via the futex_quantum_deadline helper and re-check proc_exit_group_requested / futex_interrupt each quantum, mirroring their untimed branches; futex_waitv's 500ms quantum drops to the same 100ms. Guest-visible timeout accuracy is unchanged (each quantum is capped at the remaining deadline). - The join cap (100ms) sat below the worst-case re-check latency of the states it waits on: the interruptible io wait re-polls its flags every 200ms when the wakeup-pipe byte is drained by a sibling, so a worker that WOULD wind down cleanly could be abandoned on timing alone. thread_join_workers now polls under one shared 500ms deadline -- above every per-state bound with margin, and no longer multiplied per worker. The common case is unchanged: the poll breaks the moment all workers deactivate. - Detached workers stayed joinable in the thread table. main()'s join runs before guest_destroy's internal one; if the first pass timed out and detached a worker, the second pass would snapshot the same entry and pthread_join or pthread_detach it again, undefined per POSIX for a detached thread. A worker abandoned by a timed-out join is now marked join_abandoned under thread_lock and skipped by later passes. - All writes to thread_entry_t.active become atomic release stores. thread_deactivate wrote the flag as a plain store under thread_lock, but thread_join_workers polls it with a lock-free acquire load, so the mutex provides no happens-before edge to the joiner -- the store/load pair on active is the only edge there is, and without release ordering the compiler may sink the worker's final guest-memory stores past the flag write the joiner acts on. ThreadSanitizer flagged exactly this pair on the teardown test; with the release stores in place the test runs clean under TSan. Add test-exit-group-worker: a non-main pthread issues exit_group while siblings occupy each blocking state the join relies on -- spinner threads hammering guest memory, a worker parked in a timed FUTEX_WAIT_BITSET six hundred seconds out (the condvar path above), and a worker parked in a blocking pipe read whose write end never produces -- both in forked children (exit code 42 must survive the fork-child teardown) and in the top-level process (the test's own exit 0 rides the main() join path). Review found the join itself was still reachable by paths that never request exit_group or kick siblings out of hv_vcpu_run, and three more worker states with no wake channel at all: - main()'s join ran unconditionally on every vcpu_run_loop return, but the alarm timeout (exit_code 124), a fatal default-disposition signal, and ELR_EL1==0 all bail out with a bare break -- nobody requested exit_group or interrupted the siblings on those paths. The join then burned its full cap, detached every worker, and guest_destroy's own request-interrupt-join skipped them (it honors join_abandoned), regressing the pre-PR behavior where guest_destroy always ran request-interrupt-join. Give this join the same prefix guest_destroy uses: request exit_group if not already requested, then futex_interrupt_request/wakeup_pipe_signal/thread_interrupt_all. - sys_wait4's specific-pid branch issued a bare blocking wait4() with no re-check point; nl_wait_readable_locked polled its private pipe with an untimed poll(fd, -1); fuse_dev_read blocked on an untimed pthread_cond_wait. A worker parked in any of these is invisible to thread_join_workers' poll cap and touches guest memory on an eventual delayed return, well after guest_destroy may have unmapped it. wait4 now retries with WNOHANG under a pid_cond timedwait (mirroring the existing pid==-1 loop in the same function); the netlink read now goes through the existing io_wait_fd_or_interrupted helper (already used by net.c/net-msg.c); the FUSE daemon read now uses a bounded timedwait with an exit_group check. - futex_wait's and futex_lock_pi's timed branches re-checked exit_group/futex_interrupt each quantum but never signal_check_timer()/signal_pending(), so a thread parked in a timed FUTEX_WAIT_BITSET or FUTEX_LOCK_PI deferred an expired guest itimer or a queued deliverable signal until wake or the full guest deadline. futex_lock_pi's own untimed branch had the identical gap (it copied the exit_group-check shape but never the signal re-check). Mirror futex_wait's untimed-branch pattern -- drop the bucket lock, run signal_check_timer()/signal_pending(), reacquire, re-check waiter.woken -- into all three spots.
37b0db1 to
1fbbebb
Compare
On exit_group the main thread left vcpu_run_loop and unmapped the guest slab
via guest_destroy() while sibling vCPU threads could still be mid-iteration
touching guest memory: the host process died with SIGSEGV and a guest
exit_group(0) was reported as exit 139. Masked until the fault-delivery fix
let multi-threaded JVM workloads (javac) reach the exit_group teardown.
gdb_stub_shutdown() runs first so a worker parked in a GDB stop can
deactivate instead of timing out the join.
never-deactivating slot 0, burned the poll cap, detached the host main
pthread, and raced main()'s join into a double pthread_join. It now only
requests + interrupts; main() holds the single authoritative join.
the wait's whole duration (the FUTEX_WAIT_BITSET condvar path that glibc
timed waits and JVM parkNanos issue never routes to os_sync; same in
futex_lock_pi and futex_waitv). They now sleep in 100 ms quanta and re-check
exit_group/futex_interrupt each quantum; guest-visible timeout accuracy is
unchanged.
100 ms per-worker cap sat below the io wait's 200 ms flag re-check bound —
and marks timed-out workers join_abandoned so guest_destroy()'s later pass
cannot join/detach the same pthread twice (UB for a detached thread).
lock-free, so the store/load pair on active is its only happens-before edge
(flagged by ThreadSanitizer on the teardown test; clean under TSan with the
fix).
pinned in each blocking state the join relies on (memory spinners, a timed
futex wait 600 s out, a blocking pipe read) through 10 fork-child teardowns
plus the top-level path.
A correctness argument — teardown paths, per-state wake bounds vs the join
cap, and residual risk — follows in a comment.
Summary by cubic
Join worker vCPU threads in main before tearing down guest memory to prevent host SIGSEGVs and keep correct exit codes. Add a pre-join exit/interrupt kick and bound long waits (futex, FUSE, netlink, wait4) so workers always notice teardown; shut down the debugger first to avoid join timeouts.
Bug Fixes
Tests
Written for commit 1fbbebb. Summary will update on new commits.