syscall: three globals on the entry path, shared by every CPU
One to four processes died per boot with a fault they could not have caused:
`rip 0x0` -- an instruction fetch at zero, which a process does not branch to
-- or a dereference of `0xffffffffffffffea`, which is `-EINVAL`. It hit shells
and schinitd alike and had been carried for a long time as "zsh sometimes jumps
into .data".
`syscall_entry` has to swap FS_BASE from the user's TLS to the kernel's before
it can touch any `%fs:` data, and `wrmsr` destroys rax, rcx and rdx. Those have
to be parked somewhere first -- not on the user stack, which is untrusted and
has not been switched away from yet. They were parked in three globals:
// Single-CPU scratch for syscall_entry FS swap
// (safe while IF cleared by SYSCALL/SFMASK)
uint64 _sc_rax, _sc_rcx, _sc_rdx;
IF being clear answers for an interrupt arriving on *this* CPU. It says nothing
whatever about a second CPU executing the same three stores at the same
instant, and with more than one processor entering syscalls that is not a rare
interleaving -- it is the ordinary one.
**%rcx is the user return address.** SYSCALL puts the user's rip there and
SYSRET takes it back from there. So the CPU that lost the race returned its
process into *another process's* instruction stream, with %rax -- the syscall
number on the way in, the return value on the way out -- swapped alongside it.
Both symptoms fall straight out of that: a zero handed over and followed, and
an errno used as a pointer.
Per-CPU words at %gs:32..48 now. %gs is usable there, before the FS swap, for
exactly the reason RESTORE_KERNEL_FS can already read %gs:0: GS base is per-CPU
in hardware, seginit() sets it once, and userspace cannot change it --
sys_arch_prctl() implements ARCH_SET_FS and ARCH_GET_FS and rejects the rest.
Measured rather than argued. Both variants built once, `build/esp` saved for
each, the same pristine schfs.img restored before every boot, -smp 4:
three globals 10 boots 10 faults, in five of the ten
three per-CPU 18 boots 0 faults
A _Static_assert pins all seven per-CPU offsets, because trapasm.S spells them
as literals and cannot include a header -- a slot that moves would leave a
register quietly holding somebody else's value, which is the hardest possible
thing to find.
**This is the second bug of this exact shape in this file.** The first was
`kernel_fs_base`, a single global every CPU wrote during its own seginit(), so
the last writer won and every CPU restored one CPU's per-CPU block;
RESTORE_KERNEL_FS reads %gs:0 for that reason. The lesson generalises and is
now written down in both places: **on the entry path, a global is a bug.**
Closes the row added to Gate 0 four commits ago. It was the right row to put
first -- a compositor built on this would have failed in ways indistinguishable
from its own bugs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
374ed80096
commit
3185685997
6 changed files with 173 additions and 36 deletions
|
|
@ -45,15 +45,63 @@ The per-CPU block's base is in **both** `FS_BASE` and `GS_BASE`, set by
|
|||
+8 syscall scratch trapasm.S parks %rsp here across the stack switch
|
||||
+16 struct cpu *
|
||||
+24 struct proc *
|
||||
+32 syscall rax the three registers wrmsr destroys, parked across
|
||||
+40 syscall rcx the FS_BASE swap at the top of syscall_entry
|
||||
+48 syscall rdx
|
||||
```
|
||||
|
||||
**These offsets are shared with assembly and must not be reordered.** Offset 8
|
||||
is not free: `syscall_entry` uses `%fs:(8)`, and because FS and GS base point at
|
||||
the same address, that is the same memory as `%gs:8`.
|
||||
the same address, that is the same memory as `%gs:8`. A `_Static_assert` in
|
||||
`vm.c` pins all seven, because assembly spells them as literals and a slot that
|
||||
moves leaves a register quietly holding somebody else's value.
|
||||
|
||||
Reached through `mycpu()` and `myproc()`, which compile to a single
|
||||
`movq %gs:16` / `%gs:24` with no cached base.
|
||||
|
||||
### The three words at +32, and what they cost before they were here
|
||||
|
||||
`syscall_entry` has to swap `FS_BASE` from the user's TLS to the kernel's
|
||||
before it can touch any `%fs:` data, and `wrmsr` destroys `rax`, `rcx` and
|
||||
`rdx`. Those three have to be parked somewhere first — not on the user stack,
|
||||
which is untrusted and has not been switched away from yet.
|
||||
|
||||
They were parked in **three globals**, `_sc_rax`, `_sc_rcx` and `_sc_rdx`,
|
||||
under a comment reading:
|
||||
|
||||
> Single-CPU scratch for syscall_entry FS swap (safe while IF cleared by
|
||||
> SYSCALL/SFMASK)
|
||||
|
||||
`IF` being clear answers for an interrupt arriving on *this* CPU. It says
|
||||
nothing about a second CPU executing the same three stores at the same instant,
|
||||
and with more than one processor entering syscalls that is not a rare
|
||||
interleaving — it is the ordinary one.
|
||||
|
||||
**`%rcx` is the user return address.** `SYSCALL` puts the user's `rip` there and
|
||||
`SYSRET` takes it back from there. So the CPU that lost the race returned its
|
||||
process into *another process's* instruction stream, and `%rax` — the syscall
|
||||
number on the way in, the return value on the way out — was swapped with it.
|
||||
|
||||
The symptom was one to four processes per boot dying at `rip 0x0` (an
|
||||
instruction fetch at zero, which a process does not branch to; it is handed a
|
||||
zero and follows it) or dereferencing `0xffffffffffffffea`, which is `-EINVAL`:
|
||||
a syscall return value used as a pointer. It hit shells and `schinitd` alike,
|
||||
and it had been written off as a mysterious "zsh sometimes jumps into .data".
|
||||
|
||||
Measured, at four CPUs, with the same pristine disk image each run:
|
||||
|
||||
| | boots | faults |
|
||||
|---|---|---|
|
||||
| three globals | 10 | **10**, in five of the ten boots |
|
||||
| three per-CPU words | 18 | **0** |
|
||||
|
||||
This is the *second* bug of exactly this shape in this file. The first was
|
||||
`kernel_fs_base`, a single global that every CPU wrote during its own
|
||||
`seginit()`, so the last writer won and every CPU restored one CPU's block.
|
||||
`RESTORE_KERNEL_FS` reads `%gs:0` now for that reason. The lesson generalises
|
||||
and is worth stating plainly: **on the entry path, a global is a bug.** There is
|
||||
one per-CPU block and everything that changes per CPU belongs in it.
|
||||
|
||||
> **`__thread` is wrong for per-CPU kernel data**, and this is the single most
|
||||
> expensive lesson in the tree. `__thread` means per *thread*, and the compiler
|
||||
> is entitled to read the thread pointer once and keep it in a register across
|
||||
|
|
|
|||
|
|
@ -191,24 +191,21 @@ right then.
|
|||
started reading the RTC; the comments outlived the condition. Two errors,
|
||||
not one — `uptime()` counts 10 ms ticks and they were used as seconds, so
|
||||
the clocks also ran 100× fast.
|
||||
- [ ] **User processes are corrupted on SMP.** One to three per boot die with a
|
||||
fault they could not have caused: `rip 0x0` — an instruction fetch at
|
||||
zero, which a process does not branch to, it is handed and follows — or a
|
||||
dereference of `0xffffffffffffffea`, which is `-EINVAL`. **Zero across
|
||||
three boots at `-smp 1`; one, three and one at `-smp 2`.** It hits
|
||||
`schinitd` as well as shells.
|
||||
- [x] ~~**User processes are corrupted on SMP.**~~ **Found and fixed.**
|
||||
`syscall_entry` parked `%rax`, `%rcx` and `%rdx` in three *globals*
|
||||
across the `FS_BASE` swap, under a comment arguing that `SYSCALL`
|
||||
clearing `IF` made it safe. `IF` answers for an interrupt on *this* CPU
|
||||
and says nothing about a second CPU running the same three stores — and
|
||||
`%rcx` is the user return address `SYSRET` uses, so the CPU that lost the
|
||||
race returned its process into another process's instruction stream.
|
||||
|
||||
This is now the most important row in Gate 0, and it is the row Gate 0
|
||||
was written for: a compositor built on a kernel that randomly corrupts
|
||||
user processes will fail in ways indistinguishable from its own bugs.
|
||||
That is the whole of it: `rip 0x0` was a process handed a zero and
|
||||
following it, and `0xffffffffffffffea` was `-EINVAL` in a swapped `%rax`
|
||||
used as a pointer. Per-CPU words at `%gs:32..48` now. Measured at four
|
||||
CPUs against the same pristine image: **10 faults in 10 boots before, 0
|
||||
in 18 after.** See [../architecture/kernel.md](../architecture/kernel.md),
|
||||
"Per-CPU state".
|
||||
|
||||
Ruled out: kernel stack depth. That was the leading theory and it was
|
||||
wrong — raising `KSTACKSIZE` to 8192 changed nothing. The search was
|
||||
still worth it (the high-water mark really was 3736 of 4096 bytes, and
|
||||
the AP scheduler stack really was being handed a page it did not own),
|
||||
but neither was this. Not yet examined: TLB coherence when a page table
|
||||
is changed on one CPU while the process runs on another, `kalloc`'s free
|
||||
list under contention, and the buffer cache.
|
||||
- [ ] **`getrusage` does not distinguish `RUSAGE_CHILDREN`** from `RUSAGE_SELF`.
|
||||
- [ ] **`/proc/<pid>/exe` is a regular file, not a symlink**, so `readlink()`
|
||||
fails and `ps -o comm` prints `[init]`. Symlinks themselves now work —
|
||||
|
|
|
|||
Loading…
Reference in a new issue