Conversation
A job-control shell's main loop calls this once there's nothing left to do but wait for a child to change state. Unimplemented, it hit the generic unimplemented-syscall stub, which returns immediately with no actual wait -- turning what should be an idle block into a tight 100%-CPU spin. Implemented following the same shape as this file's existing rt_sigprocmask: atomically install the new mask (with SIGKILL/SIGSTOP force-unmaskable, matching resume_sigreturn's existing sanitization), block via the ordinary scheduler dequeue/yield cycle -- sendsig() already re-enqueues a blocked thread on any pending signal, which is what actually wakes this loop -- restore the original mask, and always return EINTR, since POSIX defines this call as never "succeeding" in the normal sense. ## Verification Confirmed live: with a real Alpine zsh as init, the busy-loop this otherwise causes is gone, the shell blocks correctly in rt_sigsuspend, and the rest of boot (real zsh syscalls: sigaction, sigprocmask, fork, wait, read/write, kill) proceeds with zero faults or panics. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
medvednikov
left a comment
There was a problem hiding this comment.
Reviewed 6edbfeeac6659f02d7137a051129fd90a51def3e. CI results were not considered. Three findings in the new syscall_rt_sigsuspend() path:
[P1] Copy the mask from userspace before dereferencing it
kernel/userland/userland.v, t.masked_signals = unsafe { *mask } ...; also the new Linux wrapper
The wrapper casts the userspace address directly to &u64, and the handler only rejects null. An unmapped non-null address, an eight-byte value crossing into an unmapped page, or a noncanonical address can therefore fault while executing kernel code rather than return EFAULT. This repository already provides usercopy.copy_from_user() specifically to avoid that failure mode. Copy the complete mask into a kernel-local u64 and reject failed copies before changing the thread's state. Include unmapped and page-boundary pointer cases.
[P1] Close the lost-wakeup window between checking signals and dequeuing
kernel/userland/userland.v, the new for ... pending_signals ... { sched.dequeue_and_yield() } loop
A signal can arrive after the condition observes no pending unmasked signal but before dequeue_and_yield() removes the caller from the run queue. sendsig() sets the pending bit and calls enqueue_thread(), which returns immediately because the caller is still queued. The caller then dequeues and sleeps with the signal already pending and no wakeup left to consume. Without another signal it can remain blocked indefinitely. The wait transition needs synchronization/rechecking coordinated with signal enqueue; making the load atomic, or merely disabling local interrupts, does not close a cross-CPU race. Add a test that injects a signal specifically between the check and dequeue.
[P1] Restore the old mask after signal handling, not before dispatch
kernel/userland/userland.v, t.masked_signals = old_mask before returning EINTR
The ordinary use case blocks a signal, performs work, and temporarily unblocks it with sigsuspend. Here that signal makes the loop finish, but the old mask is immediately restored. On syscall exit, leave() calls dispatch_a_signal(), whose scan skips signals blocked by that restored mask. The syscall consequently returns EINTR without running the waking signal's handler; repeatedly suspending can then spin on the still-pending signal. Keep the temporary mask active through dispatch and arrange for the pre-suspend mask to be restored by the signal-return path, including in the saved signal-frame state. Test an initially blocked signal that is delivered during suspend and verify the handler runs before the original mask is restored.
Validation: inspected the new wrapper/handler, sendsig, scheduler enqueue/dequeue, syscall exit dispatch, signal-frame mask handling, and the checked-user-copy helper. Small deterministic state models confirmed the lost-wakeup and premature-mask-restoration sequences. No full kernel build or live kernel reproduction was run.
Addresses medvednikov's review of 6edbfee (three P1 findings): 1. Unchecked userspace pointer deref. syscall_rt_sigsuspend took mask as a raw &u64 and dereferenced it directly after only a null check; the Linux wrapper cast the userspace address straight to &u64 with no validation at all. An unmapped, page-straddling, or noncanonical pointer could fault in kernel mode instead of returning EFAULT. Changed the signature to take the raw address (mask u64, matching syscall_sigreturn's own pattern) and copy it through usercopy.copy_from_user(), rejecting a failed copy with EFAULT. 2. Lost-wakeup race between checking pending_signals and dequeuing. sendsig() sets the pending bit and calls enqueue_thread(t, true), which no-ops the actual queue-slot claim if the target thread looks still queued -- exactly the case when it races between the wait loop's condition check and its dequeue_and_yield() call, so the thread can end up dequeued with a signal already pending and no wakeup left to consume. Replaced the loop with the same dequeue-then-recheck pattern event.await_internal() already uses elsewhere in this codebase: dequeue first, then check enqueued_by_signal (which enqueue_thread(..., true) sets unconditionally, even on its no-op path) before deciding whether to actually yield or re-enqueue and retry. 3. Mask restored before the signal it woke for gets dispatched. dispatch_signal() runs at syscall exit and skips any signal masked_signals currently blocks; restoring old_mask immediately before returning EINTR made the just-delivered signal invisible to that scan again, so sigsuspend's entire purpose -- temporarily unblocking a signal so its handler runs -- silently never happened. Added a per-thread sigsuspend_restore_mask/has_sigsuspend_restore_mask pair: rt_sigsuspend leaves the temporary mask active and records old_mask there instead of restoring it directly; dispatch_signal() reads and clears it, using it as previous_mask (so sigreturn restores the true pre-suspend mask once the handler completes) instead of capturing the temporary mask that happened to be live at dispatch time. Every dispatch_signal() return path (no sigentry, nothing to dispatch, or a real dispatch) consumes it, so it's never left dangling with the temporary mask stuck active. Verified: clean V compile, C generation, and link on amd64 (temporarily applying the two already-fixed, unrelated pre-existing build blockers from vlang#229/vlang#230 to reach a clean compile locally, not included in this commit). Live boot verification of the actual race/dispatch timing was not completed -- this userland image takes longer to unpack under this dev loop's QEMU/TCG setup than was practical to wait out -- so, like the review itself, this is verified by careful reasoning through each race and state transition rather than a live reproduction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the thorough review — all three are real, and now fixed in 8d92e7d. 1. Unchecked userspace pointer. Changed 2. Lost-wakeup race. You're right that disabling local interrupts doesn't help here since 3. Premature mask restoration. Confirmed — restoring Verification, stated plainly: clean V compile, C generation, and link on amd64. I was not able to complete a live boot reproduction of the actual race/dispatch timing in this environment — the available userland image takes longer to unpack under QEMU/TCG here than was practical to wait out — so, like your own review, this fix is verified by tracing through each race and state transition by hand rather than a live repro. Flagging that limitation directly rather than implying more confidence than the testing actually supports. One related but out-of-scope observation from reading around this: 🤖 Generated with Claude Code |
medvednikov
left a comment
There was a problem hiding this comment.
Re-reviewed 8d92e7db9f9e6380d5c5da6c76124285952414fd. Two earlier findings are addressed: the mask is copied through usercopy.copy_from_user() before thread state changes, and the pre-suspend mask is now carried through dispatch for restoration by sigreturn instead of being restored before signal selection.
[P1] Prevent preemption between dequeue and the new wakeup recheck
kernel/userland/userland.v:377–393, the revised syscall_rt_sigsuspend() wait transition
The new recheck closes the original interleaving only if this thread actually reaches it. Unlike event.await_internal(), which executes cli before dequeuing, this path calls sched.dequeue_thread(t) with local interrupts still enabled. dequeue_thread() does not disable them itself.
The remaining sequence is: the loop observes no pending signal; another CPU sends one, setting pending_signals and enqueued_by_signal but skipping queue insertion because the caller is still queued; the caller dequeues itself; a timer preempts it before the flag recheck/re-enqueue. The scheduler then has no queue entry for this thread. Its pending signal and repair flag are both set, but the thread cannot run the code that would put itself back into the queue. Without a later wakeup, the suspend still hangs.
Please protect the dequeue/recheck/re-enqueue-or-yield transition against local preemption, while retaining the cross-CPU wakeup recheck. These are complementary requirements: disabling local interrupts alone did not fix the original cross-CPU race, but copying the event wait's recheck without its interrupt-disabled transition is also insufficient.
Validation: compared the revised loop with event.await_internal(), dequeue_thread(), dequeue_and_yield(), signal enqueue and scheduler behavior. A reduced state model exercised this preemption window and a protected-transition control; another checked the saved-mask handoff. These are not live Vinix reproductions. No full kernel build or boot test was run. CI results were not considered.
medvednikov (PR review, 8d92e7d re-review): dequeue_thread() doesn't disable interrupts itself, so a timer tick landing between the dequeue and the enqueued_by_signal recheck could preempt this thread while it's off the run queue but before it's decided whether to re-enqueue itself. sendsig(), seeing the thread still queued at that instant, would have skipped re-adding it to the queue -- so nothing would ever schedule it again. Wrap the dequeue-through-decision window in cli/sti, matching the identical pattern event.await_internal() already uses for the same race. yield(true) is called with interrupts still disabled, same as await_internal's own cli-wrapped yield -- this thread's saved flags carry that state across the context switch, so interrupts stay logically off for it until this sti runs, once it resumes here after being woken. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Fixed in 4c7562d: wrapped the dequeue → Verification
No CI or boot results are being claimed here, consistent with how this PR's earlier findings were addressed. |
Summary
A job-control shell's main loop calls this once there's nothing left to do but wait for a child to change state. Unimplemented, it hit the generic unimplemented-syscall stub, which returns immediately with no actual wait — turning what should be an idle block into a tight 100%-CPU spin.
Implemented following the same shape as this file's existing
rt_sigprocmask: atomically install the new mask (with SIGKILL/SIGSTOP force-unmaskable, matchingresume_sigreturn's existing sanitization), block via the ordinary scheduler dequeue/yield cycle —sendsig()already re-enqueues a blocked thread on any pending signal, which is what actually wakes this loop — restore the original mask, and always return EINTR, since POSIX defines this call as never "succeeding" in the normal sense.Verification
Confirmed live: with a real Alpine zsh as init, the busy-loop this otherwise causes is gone, the shell blocks correctly in
rt_sigsuspend, and the rest of boot (real zsh syscalls: sigaction, sigprocmask, fork, wait, read/write, kill) proceeds with zero faults or panics.🤖 Generated with Claude Code