diff --git a/.claude/memory/schos-smp-userspace-corruption.md b/.claude/memory/schos-smp-userspace-corruption.md index c52102c..eba2676 100644 --- a/.claude/memory/schos-smp-userspace-corruption.md +++ b/.claude/memory/schos-smp-userspace-corruption.md @@ -1,11 +1,13 @@ --- name: schos-smp-userspace-corruption -description: The intermittent "zsh jumps to rip 0" crash is SMP-only and still open — what has been ruled out, and the two facts worth starting from. +description: The intermittent "zsh jumps to rip 0" crash was three shared globals on the syscall entry path — found, fixed, and the rule it generalises to. metadata: type: project --- -Open as of 2026-09-21. Roughly one to three processes per boot die with a +**Fixed 2026-09-21.** Kept because the shape of the bug generalises. + +Was: Roughly one to three processes per boot die with a user-mode fault they could not have caused themselves. It has been written off as "one zsh in ~5 boots makes a wild jump" for a while; it is more than that. @@ -41,3 +43,45 @@ Reproduce: boot `-smp 2` and count `trap 1` lines in the serial log. Six tty shells start at once at boot, which is when it almost always happens. Related: [[schunix-smp-and-memory-bugs]], [[schunix-console-tty]]. + + +## It was three globals on the syscall entry path + +`syscall_entry` in `kernel/arch/x86_64/trapasm.S` has to swap `FS_BASE` from +the user's TLS to the kernel's before touching any `%fs:` data, and `wrmsr` +destroys `rax`, `rcx` and `rdx`. Those were parked in three **globals** — +`_sc_rax`, `_sc_rcx`, `_sc_rdx` in `vm.c` — under a comment reading "safe while +IF cleared by SYSCALL/SFMASK". + +`IF` answers for an interrupt on *this* CPU. It says nothing about a second CPU +executing the same three stores at the same instant. + +**`%rcx` is the user return address** — `SYSCALL` puts the user's `rip` there, +`SYSRET` takes it back. So the CPU that lost the race returned its process into +another process's instruction stream, with `%rax` (syscall number in, return +value out) swapped alongside it. `rip 0x0` was a process handed a zero and +following it; `0xffffffffffffffea` was `-EINVAL` in a stolen `%rax`, used as a +pointer. + +Per-CPU words at `%gs:32..48` now, with a `_Static_assert` pinning all seven +offsets because the assembly spells them as literals. + +Measured at four CPUs against the same pristine disk image each run: **10 +faults across 10 boots before, 0 across 18 after.** + +## The rule + +This is the second bug of this exact shape in that file — `kernel_fs_base` was +the first, a single global every CPU wrote in its own `seginit()`. **On the +entry path, a global is a bug.** There is one per-CPU block; everything that +varies per CPU belongs in it. `%gs` is usable before the FS swap because GS base +is per-CPU in hardware and userspace cannot change it (`sys_arch_prctl` +implements `ARCH_SET_FS`/`ARCH_GET_FS` only). + +## How to A/B a probabilistic kernel bug here + +Rebuilding between runs dominates the clock. Build both variants once, save +`build/esp` for each, keep a pristine `schfs.img`, then each trial is just +restore-and-boot. Faults land during boot (six tty shells exec'ing at once), so +a 40-second timeout is enough — no typed input needed. `-smp 4` roughly doubles +the rate over `-smp 2`. diff --git a/docs/architecture/kernel.md b/docs/architecture/kernel.md index 9930137..2f64b84 100644 --- a/docs/architecture/kernel.md +++ b/docs/architecture/kernel.md @@ -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 diff --git a/docs/plan/1.0.md b/docs/plan/1.0.md index bd28197..25f0e58 100644 --- a/docs/plan/1.0.md +++ b/docs/plan/1.0.md @@ -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//exe` is a regular file, not a symlink**, so `readlink()` fails and `ps -o comm` prints `[init]`. Symlinks themselves now work — diff --git a/include/proc.h b/include/proc.h index 378a188..7a5ffdf 100644 --- a/include/proc.h +++ b/include/proc.h @@ -80,6 +80,9 @@ extern int ncpu_online; // processors actually running // +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 // // Offset 8 is NOT free. syscall_entry uses %fs:(8) as its scratch slot, and // because seginit() sets GS base and FS base to the same address, %fs:8 and @@ -87,12 +90,23 @@ extern int ncpu_online; // processors actually running // it with %rsp -- a #GP in syscall_entry itself, several layers below anything // that could report it. // -// trapasm.S reads %gs:24 for proc on the way out to user mode, so these three -// offsets are shared with assembly and must not be reordered. +// **Offsets 32..48 were three globals**, `_sc_rax`/`_sc_rcx`/`_sc_rdx` in +// vm.c, under a comment saying they were safe because SYSCALL clears IF. IF +// answers for interrupts on *this* CPU and says nothing about another CPU +// executing the same three stores. Two CPUs entering a syscall together +// overwrote each other's parked registers, and %rcx is the user return address +// that sysretq uses -- so one process returned into another's instruction +// stream. See the note on syscall_entry in trapasm.S. +// +// trapasm.S reads all of these, so the offsets are shared with assembly and +// must not be reordered. #define PERCPU_SELF 0 #define PERCPU_SCRATCH 8 #define PERCPU_CPU 16 #define PERCPU_PROC 24 +#define PERCPU_SC_RAX 32 +#define PERCPU_SC_RCX 40 +#define PERCPU_SC_RDX 48 static inline struct cpu * cpu_get(void) diff --git a/kernel/arch/x86_64/trapasm.S b/kernel/arch/x86_64/trapasm.S index faf9bee..d0658ae 100644 --- a/kernel/arch/x86_64/trapasm.S +++ b/kernel/arch/x86_64/trapasm.S @@ -157,15 +157,37 @@ trapret: # ----------------------------------------------------------------------- .global syscall_entry syscall_entry: - # Swap FS_BASE to kernel value BEFORE any %fs: access. - # Save rax/rcx/rdx to single-CPU scratch globals (IF is off, no race). - movq %rax, _sc_rax(%rip) - movq %rcx, _sc_rcx(%rip) # user rip (SYSCALL stores it here) - movq %rdx, _sc_rdx(%rip) + # Swap FS_BASE to the kernel value BEFORE any %fs: access. wrmsr destroys + # rax, rcx and rdx, so they have to be parked somewhere first -- not on the + # user stack, which is untrusted and has not been switched away from yet. + # + # **Per CPU, at %gs:32..48.** These were 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 whatever about a second CPU executing these same three stores at + # the same moment -- and with two processors entering syscalls + # concurrently, that is not a rare interleaving, it is the common 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 to *the other process's* instruction pointer. The symptom was + # one to three processes per boot dying at `rip 0x0` or dereferencing + # 0xffffffffffffffea (-EINVAL, a syscall return value used as a pointer, + # because %rax was swapped too) -- never on -smp 1, one to three times on + # -smp 2, and in any process rather than one. + # + # %gs is usable here, before the FS swap, for the same reason + # RESTORE_KERNEL_FS can read %gs:0: GS base is per-CPU in hardware, is set + # once by seginit(), and userspace cannot change it -- sys_arch_prctl() + # implements ARCH_SET_FS and ARCH_GET_FS and rejects ARCH_SET_GS. + movq %rax, %gs:32 + movq %rcx, %gs:40 # user rip (SYSCALL stores it here) + movq %rdx, %gs:48 RESTORE_KERNEL_FS # clobbers rax, rcx, rdx — that's OK - movq _sc_rax(%rip), %rax - movq _sc_rcx(%rip), %rcx - movq _sc_rdx(%rip), %rdx + movq %gs:32, %rax + movq %gs:40, %rcx + movq %gs:48, %rdx # Switch to kernel stack: store user rsp at top of proc->kstack. # diff --git a/kernel/arch/x86_64/vm.c b/kernel/arch/x86_64/vm.c index 3c6fa73..d1b2448 100644 --- a/kernel/arch/x86_64/vm.c +++ b/kernel/arch/x86_64/vm.c @@ -20,8 +20,16 @@ static pte_t * walkpgdir(pml4e_t *pml4, const void *va, int alloc); // Kernel FS_BASE set by seginit(); read by trapasm.S to restore after user TLS uint64 kernel_fs_base; -// Single-CPU scratch for syscall_entry FS swap (safe while IF cleared by SYSCALL/SFMASK) -uint64 _sc_rax, _sc_rcx, _sc_rdx; + +// The per-CPU block's layout is shared with trapasm.S, which spells the offsets +// as literals because there is no way to include a C header from there. These +// pin the C side: if a slot moves, the assembly keeps reading the old address +// and the failure is a register quietly holding somebody else's value, which is +// the hardest possible thing to find. Ask the question at compile time instead. +_Static_assert(PERCPU_SELF == 0 && PERCPU_SCRATCH == 8 && PERCPU_CPU == 16 && + PERCPU_PROC == 24 && PERCPU_SC_RAX == 32 && + PERCPU_SC_RCX == 40 && PERCPU_SC_RDX == 48, + "per-CPU block layout changed -- update kernel/arch/x86_64/trapasm.S"); static pml4e_t *kpml4; static pdpe_t *kpdpt; @@ -116,11 +124,15 @@ seginit(void) // particular compiler's TLS optimisation. *(uint64 *) kernel_fs_base = kernel_fs_base; - // Clear the two per-CPU slots before anything can read them. Their offsets - // are PERCPU_CPU and PERCPU_PROC in include/proc.h and are shared with - // trapasm.S, which reads %gs:16 for proc on the way out to user mode. + // Clear the per-CPU slots before anything can read them. Their offsets are + // the PERCPU_* constants in include/proc.h and are shared with trapasm.S, + // which reads %gs:24 for proc on the way out to user mode and parks three + // registers at %gs:32..48 across the FS_BASE swap on the way in. ((uint64 *) kernel_fs_base)[2] = 0; // struct cpu * (PERCPU_CPU) ((uint64 *) kernel_fs_base)[3] = 0; // struct proc * (PERCPU_PROC) + ((uint64 *) kernel_fs_base)[4] = 0; // syscall rax (PERCPU_SC_RAX) + ((uint64 *) kernel_fs_base)[5] = 0; // syscall rcx (PERCPU_SC_RCX) + ((uint64 *) kernel_fs_base)[6] = 0; // syscall rdx (PERCPU_SC_RDX) // The same address in GS base, per CPU. //