Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
39f7656 to
760ff9f
Compare
| * parked instead. A record exists only inside the epilogue that wrote it, | ||
| * so there is no later delivery for this to reach; see sigreturn_x8. | ||
| */ | ||
| if (!el0_preempt && sigreturn_x8.valid && sigreturn_x8.elr == saved_pc) { |
There was a problem hiding this comment.
The park closes the rt_sigreturn route into the drop tail, but the ordinary syscall-return route into it is left open and reaches EL0 the same way. deliver_signal_locked snapshots the live GPRs, and on a page-table syscall tlbi_request_emit_to_vcpu has already put the wire values in X8 through X11, so the frame records X8 as 0/1/3/4 and the handler's own rt_sigreturn hands that back; the note on exec_drop_frame measures all 707 deliveries running the following svc as the wrong syscall. The shim frame is live at SP_EL1 for that delivery too, so the slot this change reads could supply X8 through X11 there as well, with the park winning when it is set. Worth its own issue rather than living only in a source comment, since #379 closes here and the same corruption survives.
There was a problem hiding this comment.
Confirmed, and I agree it belongs in its own issue rather than in a source
comment. Filed as #384. The comment here and the passage in docs/internals.md
now say it is tracked separately, with the number going in once the issue exists.
Nothing on this branch changes for it, as you asked.
Re-measured on the current head for the issue, and the fresh numbers sharpen your
point rather than soften it. Three interleaved rounds per tree, 60000 mprotect
rounds each under a 200 us itimer, two adjacent svcs with the first the
mprotect. Shared machine, 1-minute load average 3.24 to 3.73 throughout,
so the delivery counts are scheduling and only the proportions inside a run
mean anything. In round 1 with the fix the last delivery landed between the two
counters being printed, which is the 760 against 759 there.
deliveries X8 the second svc ran as ucontext X8 wrong
without fix 710/705/715 2 x710 / 2 x705 / 2 x715 100% (all rounds)
(afdcfce)
with fix 760/704/715 0 x462,3 x297 / 0 x410,3 100% (all rounds)
(this branch) x294 / 0 x440,3 x275
The guest held 226 at every svc. Two things follow that I had not stated
plainly enough before:
- The corruption rate is 100 percent of deliveries on both trees. This branch
changes which wrong number arrives, from the marker to the TLBI wire value, and
nothing else.afdcfceis not better here and this branch is not worse. X9throughX11are wrong on exactly the deliveries whereX8is 3, which
isTLBI_RANGE, the only kind that writes them:X9 = 0x200000000,
X10 = 0x1,X11 = 0x0against sentinels0xa9,0xaa,0xab. So the same
issue covers theX9throughX11shadowing this PR declines, since it is one
epilogue and one snapshot.
On the direction. Your observation that the shim frame is live at SP_EL1
for this delivery too is carried into the issue as the suggested starting point
and credited to you, deliberately without being adopted: the issue records that
the same slot could supply X8 through X11 with the park winning when it is
set, and records equally that it has not been implemented or measured. The one
caveat I attached is that shim_publish_frame_x8 is written to be callable only
where the frame is provably live, so extending the reader to a second route means
extending that argument rather than just the call site.
| handler enters with `X8 = 0x2` on the tree before the reload and with | ||
| `0xa5` after it, on both tails. `tests/test-shim-sigreturn-x8` covers | ||
| the pair. The same reload is what a `BRK`-delivered `SIGTRAP` returns | ||
| through. Its handler still enters with the marker, since the frame |
There was a problem hiding this comment.
This one is cheap to close alongside the others rather than leave standing. handle_brk is the only marker consumer that never looks at it: RESTORE_GPRS pops the frame before hvc #10 and the tail is a bare eret, so deliver_signal_locked's X8 = 2 reaches the SIGTRAP handler verbatim. Giving it the handle_el0_fault shape (LOAD_GPRS, then ldr x8, [sp, #64] and add sp, sp, #256 after the HVC) costs the same two instructions this change spends on the other three tails, and JIT translators using BRK as a trampoline are exactly the callers that read X8.
There was a problem hiding this comment.
Confirmed and fixed. The branch had documented this tail as a known gap instead
of closing it, which was the wrong call for two instructions, and your point
about JIT translators is the one that settles it: a BRK trampoline is
precisely the caller that reads X8 in the handler.
handle_brk now has the handle_el0_fault shape:
handle_brk:
LOAD_GPRS /* was RESTORE_GPRS, which popped the frame here */
hvc #10
ldr x8, [sp, #64]
add sp, sp, #256
eretValidation, guest holding 0xc3 in X8 across a BRK #0 with a SIGTRAP
handler installed, three runs each. This machine is shared, so both halves were
measured on the same head back to back.
handler entry X8 X8 after the handler's rt_sigreturn
without fix (old tail) 0x2 0xc3
with fix (new tail) 0xc3 0xc3
The right-hand column was already correct on both, since that half returns
through HVC #5 like any other syscall. The left-hand column is what changes.
tests/test-shim-sigreturn-x8.c gains a fifth phase asserting both halves; it
fails against the old tail with entered with X8 = 0x2, want 0xc3 and passes
now. The gate follows: check_drop_tail is generalized to check_reload_tail
and applied to exec_drop_frame and handle_brk alike, so deleting the ldr
from either is rc=1. Self-test is 28 cases, all passing.
| uint64_t lo = g->shim_data_base; | ||
| uint64_t hi = lo + BLOCK_2MIB; | ||
|
|
||
| if (sp_el1 < lo || sp_el1 > hi - SHIM_FRAME_BYTES) { |
There was a problem hiding this comment.
The accepted window is the whole 2 MiB shim data block, but only the top MAX_THREADS 4 KiB slots of it are EL1 stack; everything below is the shim-globals cache (identity slots, urandom ring, attention bitmask). An SP_EL1 that is wrong but still inside the block passes this check, and the memcpy then rewrites eight bytes of that cache instead of taking the log_error path. Bound it at the slot region thread_alloc_sp_el1 carves from the top of the block so a bad SP_EL1 reports rather than corrupts.
There was a problem hiding this comment.
Confirmed and fixed. You are right that this is the reverse of what the check is
for, and it is the one finding in this round that was a memory-safety hole rather
than a gap in coverage. The branch introduced it.
The bound was the whole 2 MiB block, [shim_data_base, shim_data_base + BLOCK_2MIB - 256). Only the top MAX_THREADS slots are EL1 stack, so any
SP_EL1 landing in the shim-globals cache below them passed and the memcpy
rewrote eight bytes of live shim state.
It is now the slot region, derived rather than written out:
void thread_sp_el1_region(const guest_t *g, uint64_t *lo, uint64_t *hi)
{
uint64_t top = sp_el1_top(g);
*hi = top;
*lo = top - (uint64_t) MAX_THREADS * SP_EL1_SLOT_BYTES;
}sp_el1_top, MAX_THREADS and SP_EL1_SLOT_BYTES are the three constants
thread_alloc_sp_el1 carves the slots with, so the check and the allocator
cannot drift apart. shim_publish_frame_x8 calls it and keeps the same
log_error and return.
Validation. Temporary instrumentation forced SP_EL1 to block + 0x100,
which is inside the urandom ring, and was reverted afterwards.
with the block bound: ring word 0xbd0f39f71fe88c59 -> 0xac, no log line
with the slot bound: rt_sigreturn: SP_EL1 0xeffe00100 is outside the EL1
stack region [0xefffc0000, 0xf00000000)
ring word unchanged
Boundary probed from both sides: block + 0x1bff00 is rejected and
block + 0x1c0000 is the first address accepted, which is the region floor
exactly.
What the new bound rejects that the old one accepted: the low 1835008 bytes of
the block (2097152 minus 64 slots of 4096). Every byte of that window is
shim-globals cache, so there was no address in it the old check should have been
letting through.
| "$bindir/test-wait-process-signal" | ||
| test_check "$runner" "test-wait-sigmask-signal" " - PASS" \ | ||
| "$bindir/test-wait-sigmask-signal" | ||
| test_check "$runner" "test-shim-sigreturn-x8" "0 failed" \ |
There was a problem hiding this comment.
The block above run_unit_tests still reads that test-shim-futex-fast is the exception to test-shim-* staying out of the qemu lane; this line makes a second one. Record it there with its reason (every assertion is plain Linux signal ABI), and note that the lane has not actually been run against it yet, so the next reader does not take the enumeration as complete or the registration as verified.
There was a problem hiding this comment.
Confirmed and fixed. The comment did read as though test-shim-futex-fast were
the only one, and a single recorded exception is exactly what gets the next
registration held out by mistake. Both are named now, each with its reason, and
the text says why they are named together.
On the second half of your request I did the opposite of what you asked, and it
is worth being precise about what that means. Rather than record that the lane
had not been run, I ran it: fixtures fetched, Alpine 6.18.52-0-virt booted,
test-shim-sigreturn-x8 reports 0 failed there. So the comment records the run
instead of the caveat.
What that does not cover, and what the comment also says: this was one binary run
by hand, not a full lane. No EXPECTED_BASELINES floor moves, because an
unobserved floor would assert a run that did not happen.
The EL1 shim is told to drop the exception frame it is holding by a marker the host writes into X8, and exec_drop_frame consumes that marker by popping the frame and ERETing without restoring a register. X8 is therefore the one register that cannot carry guest state across that return, and X8 is where an aarch64 SVC takes its syscall number. A signal delivered while the vCPU is at EL0 with PC on an SVC that has not executed yet is saved with that PC, correctly and without the marker. The handler returns, signal_rt_sigreturn restores all 31 GPRs from the frame and then writes the marker over X8, and the ERET resumes on the SVC, which runs as syscall 2. Dispatch finds nothing and answers ENOSYS to a call the guest made as something else. sysprog21#379. No second register can carry the value instead: every GPR on that tail is meant to be final EL0 state, which is why X7's ptrace request is already left unwritten there. The value goes in the frame's own X8 slot, and exec_drop_frame reloads X8 from it before the pop. That slot is EL1-only memory on this vCPU's own exception stack, unreachable from EL0, unshared with any other vCPU, holding nothing live once the frame is being dropped, and retired by the pop, so it cannot collide with the marker the way a register does. rt_sigreturn is the only publisher: it is the one rebuilder whose X8 differs from the one the frame was entered with, and the only one that can promise the frame is live, since it always arrives through HVC #5. No other path needs a publisher: the slot already holds the X8 its exception was taken with, which is the guest's own. The write is bounded at the EL1 exception-stack region, not at the 2 MiB shim data block that contains it. Only the top MAX_THREADS slots of that block are stack; everything below is the shim-globals cache, the identity slots, the urandom ring and the attention bitmask. A bound at the block accepts an SP_EL1 that is wrong but still inside it and rewrites eight bytes of that cache instead of taking the log_error path, which is the reverse of what the check is for. thread_sp_el1_region derives the slot region from the same constants thread_alloc_sp_el1 carves it with, so the two cannot drift. Probed by forcing SP_EL1 to block+0x100, inside the urandom ring: the block bound writes the published X8 over a ring word, 0xbd0f39f71fe88c59 to 0xac, and logs nothing; the region bound logs and does not write, and the first address it accepts is block+0x1c0000 exactly. The window that closes is the low 1835008 bytes of the block, every byte of it cache. The frame slot alone does not close the window, which was measured rather than argued. A signal delivered after the rt_sigreturn but before the vCPU is resumed snapshots the live registers, and the live X8 there is the marker while ELR_EL1 is the instruction about to resume, which is where an unexecuted SVC sits; the frame that delivery builds records the marker as the guest's X8 and its own rt_sigreturn hands it back. With only the slot in place, the 32 nested rounds of the test below all still fail and the 32 plain ones all pass, so the second hand-off stays: rt_sigreturn parks the value it published and deliver_signal_locked takes it in place of the register. What makes that substitution safe is that the parked value cannot outlive the epilogue that parked it. signal_forget_sigreturn_x8 drops the record, and the run loop calls it before every hv_vcpu_run, so the only reader that can ever see one is a delivery between the rt_sigreturn that wrote it and the guest running again. Without that, a record sits until something consumes it and is then taken by any later delivery that merely lands on the same ELR: a SIGSEGV on an instruction the guest returned to has the faulting PC in ELR_EL1 and a live X8, and got the parked value instead, which is silent register corruption on exactly the fault-driven lazy-JIT shape this epilogue exists to keep honest. Pairing the record with its ELR is kept as an identity check, not as the bound, and decoding the parked ELR to require an SVC there was weighed and dropped: a fault can be taken on an SVC too, so it narrows the stale window without closing it, and it buys nothing once the record cannot survive a resume. The one path that can move the guest inside the epilogue is where that check needs help rather than widening, and it gets it at the end. tests/test-shim-sigreturn-x8.c drives both routes without a race, and then holds the record to its window. A SIGUSR1 handler rewrites its own ucontext so the return lands on a stub that reads X8 and then issues the SVC that X8 names, and every second round raises a SIGUSR2 that the handler's sa_mask holds pending until rt_sigreturn unblocks it. A third phase returns to a stub the same way, lets the guest run on, makes the page the stub loads from unreadable, and branches back with a different X8 live, so the fault that follows is an ordinary delivery landing on the PC the earlier return came back to. A fourth phase leaves the syscall route entirely: it branches to a page with no PROT_EXEC and stores to a page with no PROT_WRITE, both of which reach the same drop tail from HVC #9, and asserts the handler enters with the guest's X8. A fifth does the same for BRK, asserting both halves: the X8 the SIGTRAP handler enters with, and the X8 the guest resumes with once the handler has stepped the saved PC past the trap and returned. All 64 rounds fail on afdcfce, the third phase passes there, having nothing to go stale, and the two fourth-phase rounds fail, entering with 0x2 where the guest held 0xa5; with only the frame slot in place the 32 nested rounds fail and nothing else does; with the slot and the park but no forget, the rounds pass and the third phase fails, reporting 0x55 where the guest faulted with 0x99; the fifth phase fails with handle_brk in its old shape, reading 0x2 where the guest held 0xc3; all five phases pass here. Every assertion is plain Linux signal ABI, so the test is registered in tests/test-matrix.sh rather than exempted, and the comment above run_unit_tests now records it there beside test-shim-futex-fast, each with its reason. That comment read as though futex-fast were the only test-shim-* in the lane, which would have had the next reader hold this one out; two recorded exceptions read as a rule. It was run against the qemu reference lane by hand, on the fixture kernel of the day, Alpine 6.18.52-0-virt, and passes there. No EXPECTED_BASELINES floor moves: a full lane was not run, and an unobserved floor asserts a run that did not happen. The reproduction the issue carries, a raw FUTEX_WAIT loop with a signal landing on it, was run interleaved so both binaries met the same machine: 5 failures in 400 runs on afdcfce against 0 in 400 here, and 161 in 400 against 0 in 400 under eight concurrent compile jobs. This machine is shared, so the first column is a floor rather than a rate: the same pair read 53 in 400 when another build was running. The loaded column is the one to read. check-svc-tails.py gains the two rules that hold the hand-off in place. A frame-dropping tail must load X8 from the frame before it pops, at the offset the host publishes to, which the gate reads out of signal.c rather than trusting the two sides to match by eye, and it must still pop: dropping the load and dropping the pop both leave a tail that assembles and ERETs. Both exec_drop_frame and handle_brk are held to that rule, handle_brk because the same deletion there is the bug it arrived with. And every hv_vcpu_run call site must forget the parked X8 first, since deleting that call is as invisible as deleting the load and leaves a worse symptom behind it; a call commented out is a call deleted, so the search that looks for one skips comments the way the resume scan already does. Fifteen self-test cases cover the two, including deleting the load, moving it below the pop, pointing it at the wrong slot, deleting the pop, and the same four on the BRK tail plus restoring its frame before the HVC again, and deleting the forget, commenting it out, moving it below the resume, and adding a second resume without one. The substitution itself is held by the test and not by a gate. Deleting it, or reading the live register in its place, reads to a text gate like any other assignment, and the test settles it anyway: the 32 nested rounds fail on that change every round, with no race to lose, and the test runs in the matrix. The forget is listed in the gate for the one failure no test can reach, a resume added later somewhere else with nothing above it. What that listing establishes is textual and no more: a call is written above every resume, not that the call runs. Wrapping the existing call in a condition leaves the gate at rc=0 with its summary line unchanged, while the record outlives the resume; the test is what fails there, its third phase reporting the same 0x55. The gate's docstring now says that, rather than claiming the forget is on the path to the resume, which no text scan of C can show. The rule stays, because a call that was never written is how it actually breaks. Three shim tails branch to that drop tail, not one, so this repairs more than the syscall path and docs/internals.md now says which. Both HVC #9 W^X tails branch there, because the host answers a flip request with a SIGSEGV when the region never had the permission asked for, and a guest holding 0xa5 in X8 entered its handler with 0x2 on afdcfce and with 0xa5 here, on each of the two. A fourth tail needed the same repair and reaches EL0 without passing through that label: handle_brk. It popped its frame before HVC sysprog21#10 and ERETed bare, so the marker a SIGTRAP delivery leaves in X8 reached the handler verbatim, making it the one marker consumer that never looked at the marker. JIT translators use BRK as a patching trampoline and read X8 in the handler, which is exactly the caller that notices. It now takes the handle_el0_fault shape: LOAD_GPRS in place of RESTORE_GPRS so the frame outlives the HVC, then the same ldr x8, [sp, sysprog21#64] and add sp, sp, sysprog21#256 after it. Measured on this head with a guest holding 0xc3 in X8 across a BRK #0 with a SIGTRAP handler installed: handler entry reads 0x2 with the old shape and 0xc3 with this one, while the X8 the guest resumes with once its rt_sigreturn has run is 0xc3 either way, that half having gone through HVC #5 like any other. What this tail still shadows on the ordinary syscall-return path is left for its own change, and docs/internals.md now measures it rather than excusing it. A delivery there snapshots the TLBI wire value as the guest's X8 and hands X9 through X11 out as TLBI operands, which syscall/proc.h already names for X8. The excuse this first wrote, that the saved PC is past the SVC, does not survive being run: the instruction after an SVC can be another SVC. Two adjacent SVCs with the first an mprotect, under a 200 us itimer over 60000 rounds, interleaved so both binaries met the same machine: all 707 deliveries here left the second SVC running as syscall 0 or 3 rather than the 226 the guest held, against 703 of 704 running as syscall 2 on afdcfce. The handler's ucontext reads X8 = 0x3 on both. So it is pre-existing, it is not the register this commit moves, and afdcfce is not better; the repair is to substitute the saved frame's X8 on that path too, which is a second publisher and the X9 through X11 shadowing beside it, and belongs in its own commit rather than under this one's claim. Since sysprog21#379 closes here and that does not, it is tracked as its own issue rather than living only in a source comment; the number goes into the comment once it is filed. Two neighbors turned up while measuring the above. The first is recorded rather than fixed: it is pre-existing, it measures identically on afdcfce, and it is not a register this moves. The frame an HVC #9 SEGV_ACCERR delivery builds records X0 and X1 as the shim left them for the HVC, so a guest faulting with x0=0xc7 and x1=0xb6 reads FAR_EL1 and the W^X type out of its ucontext instead. And PTRACE_INTERRUPT is taken inline on this same tail, where the live registers are final except for X8, so a self-signaling tracee kicked 4000 times reports the marker through PTRACE_GETREGSET at 3015 to 3458 of its stops on afdcfce and 2677 to 3124 here, over three interleaved runs on each. The second is the ptrace stop, and it is where the parked record needs help rather than only a note, so the record now follows the guest through it. A tracer that writes a new PC at such a stop and resumes with an injected signal makes the delivery's ELR differ from the one the record was parked for, so the identity check misses the one delivery the record is genuinely for and the frame takes the marker. signal_repark_sigreturn_x8 re-keys the record on the PC the stop left behind, which is all that moved: same guest, same epilogue, same X8 owed. Deleting the comparison instead would have closed this half too, at 0 of 122 and 0 of 215 landings, and it stays, because it is what keeps a record off every delivery that did not come through that stop. Interleaved on the same machine, the same tracee: 312 of 542, 494 of 986 and 431 of 776 stub landings read the marker on afdcfce, 1 of 251, 1 of 612 and 1 of 378 with the frame slot and the park but no re-key, and 0 of 672, 0 of 134 and 0 of 291 with it. No test drives it, because the landing that matters is one stop in a few hundred and the shape is a race; the probe is what measures it. Two halves of that stop stay. PTRACE_GETREGSET still reports the marker where the guest's X8 belongs, measured above. And an X8 the tracer writes back is not merely dropped: the shim dispatches on that register after HVC #5, so a value that is neither the marker nor a TLBI kind takes the conservative tail, which restores the stale frame and then tests the X7 this tail leaves holding guest state. The same tracee editing X8 to 0x99 at every stop kills the vCPU with an HVC sysprog21#13 and no stop armed, on afdcfce and here alike. Honoring such an edit means taking it out of the live register as well as into the frame slot, which is a change to the dispatch and not to this record.
760ff9f to
a83f704
Compare
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/core/shim.S">
<violation number="1" location="src/core/shim.S:1709">
P2: When a traced BRK resumes after `PTRACE_SETREGSET` changes X8, this reload discards the tracer’s value and returns the old pre-BRK X8 to EL0. Preserve tracer-updated X8, or publish the post-ptrace value into the frame and reload only for a signal-delivery marker.</violation>
</file>
<file name="docs/internals.md">
<violation number="1" location="docs/internals.md:351">
P3: For a traced `BRK` that resumes without an injected signal, HVC #10 does not include the drop-frame marker: `vcpu_handle_brk()` only enters `thread_ptrace_stop()` and returns. Qualify this statement to signal-frame delivery so the protocol description does not imply that every BRK/ptrace path carries `X8 = 2`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| * does; nothing publishes there on this path, so the value is the one the | ||
| * BRK was taken with. Then drop the frame and ERET to the new PC. | ||
| */ | ||
| ldr x8, [sp, #64] |
There was a problem hiding this comment.
P2: When a traced BRK resumes after PTRACE_SETREGSET changes X8, this reload discards the tracer’s value and returns the old pre-BRK X8 to EL0. Preserve tracer-updated X8, or publish the post-ptrace value into the frame and reload only for a signal-delivery marker.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/core/shim.S, line 1709:
<comment>When a traced BRK resumes after `PTRACE_SETREGSET` changes X8, this reload discards the tracer’s value and returns the old pre-BRK X8 to EL0. Preserve tracer-updated X8, or publish the post-ptrace value into the frame and reload only for a signal-delivery marker.</comment>
<file context>
@@ -1687,15 +1687,27 @@ restore_and_bad:
+ * does; nothing publishes there on this path, so the value is the one the
+ * BRK was taken with. Then drop the frame and ERET to the new PC.
*/
+ ldr x8, [sp, #64]
+ add sp, sp, #256
eret
</file context>
| A fourth tail carries the same reload in its own body. `handle_brk` | ||
| forwards a `BRK` from EL0 to the host through `HVC #10`, and the host | ||
| delivers `SIGTRAP` there the way it delivers on any other path, | ||
| marker included. The tail used to pop its frame before the `HVC` and |
There was a problem hiding this comment.
P3: For a traced BRK that resumes without an injected signal, HVC #10 does not include the drop-frame marker: vcpu_handle_brk() only enters thread_ptrace_stop() and returns. Qualify this statement to signal-frame delivery so the protocol description does not imply that every BRK/ptrace path carries X8 = 2.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/internals.md, line 351:
<comment>For a traced `BRK` that resumes without an injected signal, HVC #10 does not include the drop-frame marker: `vcpu_handle_brk()` only enters `thread_ptrace_stop()` and returns. Qualify this statement to signal-frame delivery so the protocol description does not imply that every BRK/ptrace path carries `X8 = 2`.</comment>
<file context>
@@ -343,11 +343,26 @@ with a guest holding `0xa5` in `X8` across a branch to a page with no
+A fourth tail carries the same reload in its own body. `handle_brk`
+forwards a `BRK` from EL0 to the host through `HVC #10`, and the host
+delivers `SIGTRAP` there the way it delivers on any other path,
+marker included. The tail used to pop its frame before the `HVC` and
+`ERET` bare, so the marker was the `X8` the handler entered with: the
+one marker consumer that never looked at it. It now takes the
</file context>
| marker included. The tail used to pop its frame before the `HVC` and | |
| marker included when the host builds a signal frame. The tail used to pop its frame before the `HVC` and |
2 participants
Fixes #379.
A signal delivered while the vCPU is at EL0 with PC on an
svcthat has notexecuted yet is saved with that PC, correctly and without the drop-frame marker.
The handler returns,
signal_rt_sigreturnrestores all 31 GPRs from the frameand then writes the marker
2overX8, andexec_drop_framepops andERETswithout restoring a register -- so the
svcresumes withX8 = 2and runs assyscall 2. Dispatch finds nothing and answers
-ENOSYSto a call the guest madeas something else.
The value goes in the frame's own
X8slot, andexec_drop_framereloadsX8from it before the pop. That slot is EL1-only memory on this vCPU's exception
stack, unshared, holding nothing live once the frame is being dropped, and
retired by the pop, so it cannot collide the way a register does. No second
register can carry it instead: every GPR on that tail is meant to be final EL0
state, which is why
X7's ptrace request is already left unwritten there.The slot alone does not close the window, which was measured rather than argued.
A signal delivered after the
rt_sigreturnbut before the vCPU resumessnapshots the live registers, where
X8is still the marker andELR_EL1isthe unexecuted
svc.rt_sigreturntherefore also parks the value it publishedand
deliver_signal_lockedtakes it in place of the register, bounded by aforget at the single
hv_vcpu_runcall site. With only the slot in place the 32nested rounds fail and nothing else does; with the slot and the park but no
forget, the rounds pass and the stale-park phase fails.
tests/test-shim-sigreturn-x8.cis deterministic and needs no race: 64 roundson the syscall route, a stale-park phase, and two rounds that reach the same
drop tail from HVC #9 instead. All 64 rounds and both HVC #9 rounds fail on
afdcfce, entering with0x2where the guest held0xa5; all four phases passhere. Every assertion is plain Linux signal ABI, so it is registered in
tests/test-matrix.shrather than exempted -- the qemu lane needs a referencekernel and was not run here, so neither
EXPECTED_BASELINESfloor moves.The reproduction the issue carries, a raw
FUTEX_WAITloop with a signallanding on it, was run interleaved so both binaries met the same machine: 5
failures in 400 runs on
afdcfceagainst 0 in 400 here, and 161 in 400 against0 in 400 under eight concurrent compile jobs. That machine is shared, so the
unloaded column is a floor rather than a rate -- the same pair read 53 in 400
while another build was running. The loaded column is the one to read.
scripts/check-svc-tails.pygains the two structural rules: the drop tail mustload
X8from the frame before it pops, at the offset the gate reads out ofsignal.crather than trusting the two sides to match by eye, and everyhv_vcpu_runcall site must forget the parked value first. It cannot gate thesubstitution itself, and the commit says so -- that half rests on the test,
which fails deterministically on it.
Two pre-existing neighbours are recorded rather than fixed: the same tail hands
X9throughX11out to EL0 after a page-table syscall in the same epilogue,and a ptrace stop taken inside the
rt_sigreturnepilogue already shows thetracer the marker through
PTRACE_GETREGSET.Summary by cubic
Fixes #379: a signal delivered while the vCPU has PC on an unexecuted
svcno longer resumes thatsvcwith the drop-frame marker (2) inX8, so the guest's syscall runs as its own number instead of as syscall 2 and returning-ENOSYS.Bug Fixes
exec_drop_framenow reloadsX8from the frame's ownX8slot before popping;signal_rt_sigreturnpublishes the restored value there.signal_rt_sigreturnalso parks the restoredX8; a delivery in the same epilogue takes the parked value, the run loop forgets it before everyhv_vcpu_run(), andsignal_repark_sigreturn_x8()re-keys it after aPTRACE_INTERRUPTstop changes the resume PC.handle_brknow keeps its frame live across theHVC, reloadsX8from the slot, then pops, so aSIGTRAPhandler enters with the guest'sX8instead of the marker.HVC #9W^X tails branch to the same drop tail, so theirSIGSEGVhandlers now enter with the guest'sX8rather than the marker.X9throughX11to EL0, and ptrace stops on this tail still show the marker viaPTRACE_GETREGSET.Validation
tests/test-shim-sigreturn-x8.ccovering plain and nested returns, the stale-park window, both W^X tails, and the BRK tail.scripts/check-svc-tails.pynow checks theX8reload offset againstsignal.c, requiressignal_forget_sigreturn_x8()above everyhv_vcpu_run(), and holds bothexec_drop_frameandhandle_brkto the reload.FUTEX_WAITloop failed 5/400 on base vs 0/400 here, and 161/400 vs 0/400 under concurrent build load.EXPECTED_BASELINESis unchanged.Written for commit a83f704. Summary will update on new commits.