Repository navigation
Serialize timed_run registration and exit transitions - #15
Conversation
| // module is still unwinding, so hold its counters to keep this run's exit guard from clearing | ||
| // the flag or restoring the pages. | ||
| REQUIRE(allocator.disable_code()); | ||
| sysio::vm::timed_run_has_timed_out.store(true, std::memory_order_release); |
There was a problem hiding this comment.
This sets timed_run_has_timed_out before entering timed_run, so the pre-existing check at backend.hpp:415-416 throws before the new enable/re-check block at lines 421-426. The other added test reaches that block only while the flag remains false; deleting the post-enable recheck/revoke would leave both tests green. Please trigger the timeout deterministically after the initial flag check, then assert that the body is not executed and the pages are re-revoked.
There was a problem hiding this comment.
Reworked in 9493a47. The timeout is now fired from inside the run, after the entry check and before the module's first instruction, through a watchdog the test drives. Asserted: timeout_exception; the body never executed (host-call witness); the pages revoked at that point and still revoked, with the flag set, after the exit while another run of the module is registered; the next run finds the timeout in force and, as the last registered run, restores both. The entry re-enable block itself is gone (other thread), so there is no re-check left to delete.
| // timeout that fired meanwhile is not undone: the pages go back to revoked and this run | ||
| // reports the timeout. | ||
| if (mod->allocator.code_disabled()) { | ||
| SYS_VM_ASSERT(mod->allocator.enable_code(Impl::is_jit), wasm_bad_alloc, "mprotect failed"); |
There was a problem hiding this comment.
An older timed_run exit guard can pass the zero-count checks at lines 401/404, pause, and then perform its stale timeout-flag clear or enable after this run has registered and its watchdog has fired. This block can consequently restore execute permission, observe false, and invoke f() after the one-shot timeout. Since the x86 path has no cooperative in-loop poll, infinite WASM can then run indefinitely. I reproduced the stale-clear case with a barrier-controlled watchdog against this head: the callback fired, but the body executed and returned normally. Please make run registration, timeout publication, flag clearing, and page-protection transitions linearizable, for example with a generation/state machine, and cover the forced interleaving.
There was a problem hiding this comment.
Agreed, and the stale re-enable has the same shape. In 9493a47 registration and the exit guard take timed_run_transition_mutex (normal thread context only: before the watchdog is armed, after it is disarmed); the timeout publication stays lock-free in signal context and fires only for a registered run, whose counts keep any exit from clearing or re-enabling. The counters are plain integers under the lock; the flag and the page state stay atomic. The entry block is removed: a registered run can no longer observe the flag clear while the pages are revoked.
On the forced interleaving: the window now sits inside one critical section, so no black-box test can pause a thread there. The new cases pin the orderings that remain observable, including a newer run timing out and exiting while the older run is still registered, in the shared-module / per-thread-backend shape nodeop uses. Cost is +7 ns per timed_run uncontended; the PR body has the contention numbers.
060e8af to
a3ca475
Compare
Review of PR #15 (huangminghuang) found a race in the exit guard: it reads the in-progress counters and then clears the timeout flag or re-enables the module's pages, and a new run can register in between. A stale clear or re-enable that lands after that run's watchdog has fired leaves it executing with no timeout in force, and generated code has no cooperative poll to stop it. The first version's entry re-enable block made the stale clear worse: a spurious wasm_memory_exception became an unbounded run. Registration and the exit guard now take timed_run_transition_mutex. Both run in normal thread context, before the watchdog is armed and after it is disarmed, never in a signal handler; the timeout publication (flag store and page revoke) is the only signal-context work and takes no lock. It fires only for a registered run, whose counts keep any exit from clearing or re-enabling while it is in force. The counters become plain integers guarded by the mutex; the flag and the page state stay atomic because the signal path writes them. The entry re-enable block is gone: the state it repaired cannot be observed by a registered run any more. The lock is taken twice per timed_run, at entry and exit, around a few counter updates, and never while the body runs, so runs still execute concurrently. Measured: 30 → 37 ns per timed_run uncontended; with 16 threads doing nothing but enter and exit, 0.9 → 2.8 µs. Tests drive the timeout from the test instead of racing for it. Fired after the entry check, a run reports timeout_exception with the body never executed and the pages revoked, which stay revoked with the flag set while another run of the module is registered; the next run finds the timeout in force and, as the last registered run, restores both on exit. A second case runs one compiled module through a per-thread backend and context each, as nodeop's read-only threads do: a newer run timing out and exiting while the older run is registered leaves the timeout in force, and the older run's exit restores it.
heifner
changed the title
a3ca475 to
9493a47
Compare
| } catch (...) { | ||
| } | ||
| } }; | ||
| worker.join(); |
There was a problem hiding this comment.
[P2] Add a regression that fails without the transition lock
worker.join() completes the newer timed_run before on_disarm returns, and the older run's exit transition starts only afterward. The first new test likewise uses a sequential fake peer. Consequently, both tests still pass if the two new lock_guards are removed, so the stale-clear/stale-reenable race remains untested even though it can erase a watchdog interrupt and leave JIT code running unbounded. Please add a deterministic guard for the serialization itself—for example, hold timed_run_transition_mutex in the test and use promises to prove that a real run cannot arm on entry or report completion on exit until it is released; reproducing the exact stale transition would require a test-only barrier inside that transition.
There was a problem hiding this comment.
Added in 210dbcb. The test holds timed_run_transition_mutex: a real run on another thread cannot arm its watchdog (an on_arm hook on the test watchdog) until the lock is released, and once inside its body it cannot report completion until the lock is released again. Promise-gated, so the negative waits only trip if the run gets through the lock. Checked against a copy of backend.hpp with both lock_guards stripped: exactly this case fails on its two CHECKs (entry arm, exit completion); everything else passes.
Review of PR #15 (huangminghuang): the two timed_run cases pass with both lock_guards removed, because each orders its runs sequentially. This case holds timed_run_transition_mutex from the test: a run on another thread cannot arm its watchdog until the lock is released, since registration sits behind it, and once inside its body it cannot report completion until the lock is released again, since so does the exit transition. Each half fails without its lock_guard.
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
Reviewed current head 210dbcb. The transition serialization is sound, the follow-up regression exercises both lock sites, and the current local and CI test suites pass.
Review of PR #15 (huangminghuang) found a race in the exit guard: it reads the in-progress counters and then clears the timeout flag or re-enables the module's pages, and a new run can register in between. A stale clear or re-enable that lands after that run's watchdog has fired leaves it executing with no timeout in force, and generated code has no cooperative poll to stop it. The first version's entry re-enable block made the stale clear worse: a spurious wasm_memory_exception became an unbounded run. Registration and the exit guard now take timed_run_transition_mutex. Both run in normal thread context, before the watchdog is armed and after it is disarmed, never in a signal handler; the timeout publication (flag store and page revoke) is the only signal-context work and takes no lock. It fires only for a registered run, whose counts keep any exit from clearing or re-enabling while it is in force. The counters become plain integers guarded by the mutex; the flag and the page state stay atomic because the signal path writes them. The entry re-enable block is gone: the state it repaired cannot be observed by a registered run any more. The lock is taken twice per timed_run, at entry and exit, around a few counter updates, and never while the body runs, so runs still execute concurrently. Measured: 30 → 37 ns per timed_run uncontended; with 16 threads doing nothing but enter and exit, 0.9 → 2.8 µs. Tests drive the timeout from the test instead of racing for it. Fired after the entry check, a run reports timeout_exception with the body never executed and the pages revoked, which stay revoked with the flag set while another run of the module is registered; the next run finds the timeout in force and, as the last registered run, restores both on exit. A second case runs one compiled module through a per-thread backend and context each, as nodeop's read-only threads do: a newer run timing out and exiting while the older run is registered leaves the timeout in force, and the older run's exit restores it.
This replaces the previous content of PR #15 after review; the branch is rewritten to this single commit on top of #14 (
a5b4ae2) and force-pushed.Follow-on to #14 (stacked on
fix/readonly-shared-code-segv).The race
On the x86_64 shared-code path,
timed_run's exit guard clearstimed_run_has_timed_outwhen the last run in the process leaves and re-enables the module's code pages when the last run of that module leaves. It reads the in-progress counters and then acts, and a new run can register in between, so a stale clear or re-enable can land after that run's watchdog has fired:Generated code has no cooperative poll, so an unbounded body then runs until it returns on its own. The first version of this PR (re-enable revoked pages on entry, then re-check the flag) turned the stale-clear outcome from a spurious
wasm_memory_exceptioninto exactly that.The change
backend.hpp: registration and the exit guard taketimed_run_transition_mutex. Both run in normal thread context, before the watchdog is armed and after it is disarmed, never in a signal handler. The timeout publication (flag store +mprotect) is the only signal-context work and takes no lock: it fires only for a registered run, whose counts keep any exit from clearing or re-enabling while it is in force. A run that registers before an exiting run's transition prevents it; one that registers after it finds the flag clear and the pages executable, never one without the other. The counters (total_timed_run_in_progress,growable_allocator::timed_run_in_progress) become plain integers guarded by the mutex;timed_run_has_timed_outand_code_disabledstay atomic because the signal path writes them. The entry re-enable block is gone; the state it repaired (pages revoked, flag already clear) can no longer be observed by a registered run.Overhead
The lock is taken twice per
timed_run(entry, exit) around a few counter updates, plus onemprotecton the last exit after a timeout, and never while the body runs: it serializes the bookkeeping, not execution. It is observed only when threads enter or exit at the same instant. Measured on a 24-core host,timed_runof an empty body and of a call to an empty export, before (#14, shared atomics) and after (this change):The multi-thread rows are the worst case, every thread doing nothing but enter and exit. One
timed_runwraps one action'sapply, which runs for tens of microseconds and up, so coinciding transitions are rare and the per-action cost stays near the uncontended number. The shared counters contended before this change too (thebeforerows).Tests (
tests/jit_revoked_code_tests.cpp,[timed_run])The watchdog is driven by the test, so each interleaving is chosen rather than raced:
timeout_exception, the module body (a host-call witness) never executes, the pages are revoked and stay revoked with the flag set while another run of the module is registered. The next run finds the timeout in force, does not execute, and as the last registered run restores the flag and the pages on exit; the module then runs again.share,set_context, ownwasm_allocator), as nodeop's read-only threads hold it: a newer run that times out and exits while the older run is still registered leaves the timeout in force; the older run's exit restores it, and both backends run again.timed_run_transition_mutex; a run on another thread cannot arm its watchdog until the lock is released, and once inside its body it cannot report completion until the lock is released again. Each half fails with itslock_guardremoved.The window the race lived in is now inside one critical section, so no black-box interleaving can be forced into it; the first two cases pin the orderings that remain observable and the third guards the lock that closes the window.
Validation
eos-vm
unit_tests(gcc-14, vcpkg softfloat): the new cases,[revoked_code_recovery], the signals suite, then the full suite (265 cases / 1480 assertions).