Fix cross-process signal delivery to EL0-preempted guests#76
Conversation
There was a problem hiding this comment.
1 issue found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } else if (vexit->reason == HV_EXIT_REASON_CANCELED || | ||
| vexit->reason == HV_EXIT_REASON_UNKNOWN) { |
jserv
left a comment
There was a problem hiding this comment.
Core fix looks right -- the UNKNOWN/CANCELED unification and the EL0-preempt redirect via HV_REG_PC/CPSR both match the bug description. One follow-up worth folding in:
src/syscall/proc.c:1995 -- rseq IP fixup has the same root cause this PR fixes in signal.c. The rseq_try_abort call in the CANCELED branch reads ELR_EL1 unconditionally, so for an EL0-preempted vCPU it sees the stale syscall-return PC and won't fire the IP fixup for a critical section actually interrupted at EL0. Partial cover: when signal_pending() is also true on the same exit, signal_deliver re-runs rseq_try_abort with the el0_preempt-corrected PC, so the abort still happens. The gap is exits with no queued signal (fork-barrier wakeups, future ptrace wake paths). Mirror the CPSR-based PC selection here for symmetry:
uint64_t cur_pc, cur_cpsr = 0;
hv_vcpu_get_reg(vcpu, HV_REG_CPSR, &cur_cpsr);
bool el0 = (cur_cpsr & 0xfULL) == 0;
if (el0)
hv_vcpu_get_reg(vcpu, HV_REG_PC, &cur_pc);
else
hv_vcpu_get_sys_reg(vcpu, HV_SYS_REG_ELR_EL1, &cur_pc);
int rseq_rc = rseq_try_abort(g, current_thread->rseq_gva,
current_thread->rseq_signature, &cur_pc);
if (rseq_rc == 1) {
if (el0)
hv_vcpu_set_reg(vcpu, HV_REG_PC, cur_pc);
else
hv_vcpu_set_sys_reg(vcpu, HV_SYS_REG_ELR_EL1, cur_pc);
}See inline note on the UNKNOWN routing.
| } | ||
| } else if (vexit->reason == HV_EXIT_REASON_CANCELED) { | ||
| } else if (vexit->reason == HV_EXIT_REASON_CANCELED || | ||
| vexit->reason == HV_EXIT_REASON_UNKNOWN) { |
There was a problem hiding this comment.
Routing UNKNOWN through the same fall-through as CANCELED is broad: if HVF ever returns UNKNOWN for a genuine fault (not the SIGUSR2 race), the loop will silently retry instead of taking the unexpected exit reason crash path at the end of this switch. Consider gating it on something actionable being present after the drain, e.g.:
} else if (vexit->reason == HV_EXIT_REASON_CANCELED ||
(vexit->reason == HV_EXIT_REASON_UNKNOWN &&
(signal_pending() ||
proc_exit_group_requested() ||
(is_main && g_timed_out)))) {Per the PR description the queued guest signal is already drained before we reach the switch, so signal_pending() is true for the cross-process SIGUSR2 race this is targeting -- but a genuine HVF error with no signal queued would still crash visibly instead of looping.
jserv
left a comment
There was a problem hiding this comment.
Rebase latest main branch and resolve conflicts.
test-fork's phase-2 signal child spins in `while (!got_usr1) usleep()` waiting for a SIGUSR1 sent cross-process by its parent. The signal was delivered only ~35% of the time and lost the rest, so `make check` hung at test-fork until the 60s per-test timeout -- often longer, as leaked `elfuse --fork-child` orphans kept the driver's stdout pipe open. Two complementary defects: 1. vcpu_run_loop treated HV_EXIT_REASON_UNKNOWN as fatal. A host SIGUSR2 (the cross-process guest-signal transport) that interrupts hv_vcpu_run mid-execution aborts the run with UNKNOWN rather than the clean CANCELED that hv_vcpus_exit() produces for a vCPU caught between runs. Route UNKNOWN through the same cancellation handling so the already-queued guest signal is delivered instead of crashing the child. Gate UNKNOWN on an actionable event actually being present (a queued signal, a pending exit_group, or a fired timeout) so a genuine HVF fault with nothing pending still falls through to the "unexpected exit reason" crash path instead of silently retrying the run. 2. signal_deliver redirected to the handler only via ELR_EL1, which takes effect solely on an ERET from EL1 (the syscall-return path, gated by the shim's X8==2 exec_drop_frame marker). When the signal is delivered from the cancellation branch -- i.e. the vCPU was preempted while running EL0 code (cross-process SIGUSR2, or SIGALRM in a tight loop) -- there is no pending ERET, the resume uses HV_REG_PC, and the ELR_EL1 write is a no-op: the handler never runs and only the X0=signum clobber lands, re-running the interrupted nanosleep with a bogus arg and spinning forever. Detect EL0 preemption from the live PSTATE (CPSR M[3:0]==0), save the interrupted PC from HV_REG_PC instead of the stale ELR_EL1, and redirect HV_REG_PC/CPSR directly; skip the X8==2 marker since there is no shim frame to drop. The rseq IP fixup in the same cancellation branch had the identical stale-ELR_EL1 hazard: rseq_try_abort read and wrote ELR_EL1 unconditionally, so a critical section interrupted at EL0 with no queued signal (fork-barrier/ptrace wakeups) would never abort. Select HV_REG_PC vs ELR_EL1 from the live PSTATE there too, mirroring signal_deliver. test-fork now passes 20/20; `make check` is green with no hang.
c21d882 to
3bf61cd
Compare
To fix #67. test-fork's phase-2 signal child spins in
while (!got_usr1) usleep()waiting for a SIGUSR1 sent cross-process by its parent. The signal was delivered only ~35% of the time and lost the rest, somake checkhung at test-fork until the 60s per-test timeout -- often longer, as leakedelfuse --fork-childorphans kept the driver's stdout pipe open.Two complementary defects:
vcpu_run_loop treated HV_EXIT_REASON_UNKNOWN as fatal. A host SIGUSR2 (the cross-process guest-signal transport) that interrupts hv_vcpu_run mid-execution aborts the run with UNKNOWN rather than the clean CANCELED that hv_vcpus_exit() produces for a vCPU caught between runs. Route UNKNOWN through the same cancellation handling so the already-queued guest signal is delivered instead of crashing the child.
signal_deliver redirected to the handler only via ELR_EL1, which takes effect solely on an ERET from EL1 (the syscall-return path, gated by the shim's X8==2 exec_drop_frame marker). When the signal is delivered from the cancellation branch -- i.e. the vCPU was preempted while running EL0 code (cross-process SIGUSR2, or SIGALRM in a tight loop) -- there is no pending ERET, the resume uses HV_REG_PC, and the ELR_EL1 write is a no-op: the handler never runs and only the X0=signum clobber lands, re-running the interrupted nanosleep with a bogus arg and spinning forever. Detect EL0 preemption from the live PSTATE (CPSR M[3:0]==0), save the interrupted PC from HV_REG_PC instead of the stale ELR_EL1, and redirect HV_REG_PC/CPSR directly; skip the X8==2 marker since there is no shim frame to drop.
test-fork now passes 20/20 (was ~7/20);
make checkis green with no hang.