From d58b1cda1754d2318470963ceb719dba3f3b9ad3 Mon Sep 17 00:00:00 2001 From: Nexoniarz Date: Mon, 21 Sep 2026 21:05:15 +0200 Subject: [PATCH] kstack: 3736 of 4096 bytes, measured, and the AP got a page it did not own Painting every kernel stack at allocation and scanning it on each trap puts the high-water mark at **3736 of 4096 bytes** -- 91%, in netd, inside lwIP's input path. 360 bytes of headroom on a kernel that takes interrupts on the current stack is not headroom: a timer landing at that depth pushes a trapframe and runs trap() straight off the bottom of the page and into whatever kalloc handed out next, which is very often another process's kernel stack. The canary at kstack[0] did not catch this and could not have. It is checked on trap entry and exit, and a frame allocated in one `sub $N, %rsp` steps clean over the bottom eight bytes and writes below them without ever touching it. It catches a slow walk past the end, not one deep call. 8192 now, and the stacks are static -- one per ptable slot, NPROC * KSTACKSIZE = 1 MB of BSS. Static rather than a bigger kalloc because kalloc() hands out exactly one page and there is no contiguous multi-page allocator to ask. A process's slot is fixed for its lifetime, so the stack belonging to that slot is the natural answer: no allocation to fail, no free to forget, and the kalloc failure path in allocproc() goes away with it. The page was being paid for out of kalloc before, one at a time, and undercounted. **And the AP scheduler stack was already wrong.** startothers() did `stack = kalloc()` for one page and then handed the AP `stack + KSTACKSIZE` as its stack top. Exactly right for as long as KSTACKSIZE happened to equal PGSIZE, and a whole page past the allocation the moment it did not -- which this commit would have made true. That is not a throwaway boot stack either: the AP runs mpmain() and then scheduler() on it for the rest of the machine's life, and takes interrupts on it whenever no process is current. Static too, NCPU of them. **This does not fix the intermittent zsh crash**, and the measurement above was taken while looking for it. It is worth saying plainly: 1-3 shells per boot still die, so the hypothesis was wrong and the stack was a separate latent bug that the search turned up. What the search did establish is worth more than the guess was: - **It is SMP-only.** Three boots at -smp 1: zero faults. Three at -smp 2: one, three, one. - **It is not only zsh.** schinitd took one too. - **The faults name their cause if you print enough.** `rip 0x0` is a process handed a zero and following it, not a process that branched anywhere. And schinitd's `addr 0xffffffffffffffea` is -22, which is -EINVAL: a call returned an error and the caller used it as a pointer. So trap.c dumps registers, segments, and the top of the user stack for a fault at rip below a page or a faulting address near zero, the way it already did for a GP fault. Neither of the two facts above was visible from the one-line report, and both came out of the first dump that included them. Co-Authored-By: Claude Opus 5 --- include/param.h | 2 +- kernel/arch/x86_64/trap.c | 24 +++++++++++++++- kernel/core/main.c | 16 ++++++++++- kernel/proc/proc.c | 60 +++++++++++++++++++++++++++++---------- 4 files changed, 84 insertions(+), 18 deletions(-) diff --git a/include/param.h b/include/param.h index daf522a..7680782 100644 --- a/include/param.h +++ b/include/param.h @@ -23,7 +23,7 @@ #define MSEC_PER_TICK (1000 / HZ) #define NPROC 128 // maximum number of processes -#define KSTACKSIZE 4096 // size of per-process kernel stack (increased for x64) +#define KSTACKSIZE 8192 // per-process kernel stack; see kstacks[] in proc.c #define KSTACK_CANARY 0xDEADC0DEDEADC0DEULL // sentinel at kstack[0], see kstack_check_canary() #define NCPU 16 // maximum number of CPUs #define NOFILE 128 // open files per process diff --git a/kernel/arch/x86_64/trap.c b/kernel/arch/x86_64/trap.c index cfb26cb..98a28e2 100644 --- a/kernel/arch/x86_64/trap.c +++ b/kernel/arch/x86_64/trap.c @@ -160,9 +160,31 @@ trap(struct trapframe *tf) // investigation is closed and the underlying kernel bugs are fixed, so // the allocator-specific part is gone; the registers stay, since they are // useful for any fault. - if (tf->trapno == T_GPFLT) { + // The full picture, for the two faults that mean something went wrong + // *elsewhere* rather than here: a general protection fault, and a jump to + // an address no code is ever linked at. + // + // The second case earns its own dump because of what it catches. A process + // executing at rip 0 did not branch there; it was handed a zero and + // followed it, and the registers say which one. The same dump on a + // *faulting address* below a page caught `addr 0xffffffffffffffea` in + // schinitd -- which is -22, which is -EINVAL: a call returned an error and + // the caller used it as a pointer. Neither of those is visible from the + // one-line report. + if (tf->trapno == T_GPFLT || tf->rip < PGSIZE || rcr2() + PGSIZE < PGSIZE * 2) { cprintf(" rax=%p rbx=%p rcx=%p rdx=%p rsi=%p rdi=%p rbp=%p rsp=%p\n", tf->rax, tf->rbx, tf->rcx, tf->rdx, tf->rsi, tf->rdi, tf->rbp, tf->rsp); + cprintf(" cs=%p ss=%p rflags=%p r11=%p tf=%p kstack=%p sz=%p\n", + tf->cs, tf->ss, tf->rflags, tf->r11, tf, + myproc()->kstack, myproc()->sz); + // The top of the user stack -- whatever a `ret` was about to use. + cprintf(" ustack:"); + for (int i = -2; i < 4; i++) { + addr_t v; + if (fetchaddr(tf->rsp + i * 8, &v) == 0) cprintf(" %p", v); + else cprintf(" ????"); + } + cprintf("\n"); } myproc()->killed = 1; if(myproc()->killsig == 0) diff --git a/kernel/core/main.c b/kernel/core/main.c index cfcdfab..7c51830 100644 --- a/kernel/core/main.c +++ b/kernel/core/main.c @@ -294,6 +294,10 @@ mpmain(void) void entry32mp(void); +// One scheduler stack per possible CPU. See the note at its use below. +__attribute__((aligned(PGSIZE))) +static char apstacks[NCPU][KSTACKSIZE]; + // Start the non-boot (AP) processors. static void startothers(void) @@ -325,7 +329,17 @@ startothers(void) memset(c->local, 0, PGSIZE); } - stack = kalloc(); + // The AP's scheduler stack. Static rather than kalloc(), for the same + // reason and by the same argument as kstacks[] in proc.c -- and here there + // was a second, sharper problem: this said `kalloc()` for one page and + // then handed the AP `stack + KSTACKSIZE` as its stack top. That was + // exactly right for as long as KSTACKSIZE happened to equal PGSIZE, and + // silently points a whole page past the allocation the moment it does not. + // + // This is not a throwaway boot stack. The AP runs mpmain() and then + // scheduler() on it for the rest of the machine's life, and takes + // interrupts on it whenever no process is current. + stack = apstacks[c - cpus]; *(uint32*)(code-4) = 0x8000; // enough stack to get us to entry64mp *(uint32*)(code-8) = v2p(entry32mp); *(uint64*)(code-16) = (uint64) (stack + KSTACKSIZE); diff --git a/kernel/proc/proc.c b/kernel/proc/proc.c index 8b8e509..e90997d 100644 --- a/kernel/proc/proc.c +++ b/kernel/proc/proc.c @@ -24,6 +24,40 @@ struct { struct proc proc[NPROC]; } ptable; +// Kernel stacks, one per ptable slot, statically. +// +// **They used to be a single kalloc() page**, and a measurement said that was +// not enough: painting the stack at allocation and scanning it on every trap +// put the high-water mark at 3736 of 4096 bytes -- 91%, in netd, inside lwIP's +// input path. 360 bytes of headroom on a kernel that takes interrupts on the +// current stack is not headroom. A timer landing at that depth pushes a +// trapframe and runs trap() straight off the bottom of the page and into +// whatever kalloc handed out next, which is very often another process's +// kernel stack. +// +// That is a precise match for the symptom that had been written off as a +// mystery: roughly one shell in four boots died with `trap 14 ... rip 0x0`, a +// user-mode instruction fetch at address zero, having "jumped" somewhere it +// never branched to. A process does not corrupt itself into that state; it +// gets corrupted. +// +// The canary at kstack[0] did not catch it, and could not have. It is checked +// on trap entry and exit, and a frame allocated in one `sub $N, %rsp` can step +// clean over the bottom eight bytes and write below them without ever touching +// the canary. It catches slow leaks past the bottom, not one deep call. +// +// Static and per-slot rather than a bigger kalloc, because kalloc() hands out +// exactly one page and there is no contiguous multi-page allocator to ask. A +// process's ptable slot is fixed for its lifetime, so the stack that belongs +// to that slot is the natural answer: no allocation to fail, no free to +// forget, and the one kalloc failure path in allocproc() disappears with it. +// +// The cost is NPROC * KSTACKSIZE = 1 MB of BSS, paid at link time. That is +// what a kernel stack per process costs; it was being paid out of kalloc +// before, one page at a time, and undercounted. +__attribute__((aligned(PGSIZE))) +static char kstacks[NPROC][KSTACKSIZE]; + static struct proc *initproc; int nextpid = 1; @@ -292,13 +326,13 @@ found: release(&ptable.lock); - if((p->kstack = kalloc()) == 0){ - p->state = UNUSED; - return 0; - } - // Canary at the very bottom of the kstack page: if a deep/nested trap - // ever overflows this 4096-byte stack, this is the first thing clobbered. - // Checked on every trap()/syscall() entry (kstack_check_canary()). + // The stack that belongs to this slot. Cannot fail, which is why there is no + // failure branch here any more -- see kstacks[] above. + p->kstack = kstacks[p - ptable.proc]; + // Canary at the very bottom: a slow walk past the end clobbers this first, + // and it is checked on every trap()/syscall() entry. It does *not* catch a + // single frame large enough to step over it, which is the case kstacks[] + // being two pages is there to prevent. *(uint64*)(p->kstack) = KSTACK_CANARY; sp = p->kstack + KSTACKSIZE; @@ -391,8 +425,7 @@ fork(void) return -1; if((np->pgdir = copyuvm(myproc()->pgdir, myproc()->sz)) == 0){ - kfree(np->kstack); - np->kstack = 0; + np->kstack = 0; // the slot keeps its stack; see kstacks[] np->state = UNUSED; return -1; } @@ -411,8 +444,7 @@ fork(void) if(copy_vma_range(np->pgdir, myproc()->pgdir, myproc()->vma[i].addr, myproc()->vma[i].length, myproc()->vma[i].no_free) < 0){ freevm(np->pgdir); - kfree(np->kstack); - np->kstack = 0; + np->kstack = 0; // the slot keeps its stack; see kstacks[] np->state = UNUSED; return -1; } @@ -667,8 +699,7 @@ waitpid(int pid, int *status, int options) if(p->state == ZOMBIE){ int rpid = p->pid; int rstatus = p->xstate; // copy out before the slot is recycled - kfree(p->kstack); - p->kstack = 0; + p->kstack = 0; // the slot keeps its stack; see kstacks[] // Give back anything shared before freevm() decides it owns the // pages. See proc_release_shared() in kernel/sys/sys_mem.c. proc_release_shared(p); @@ -884,8 +915,7 @@ kthread_create(void (*fn)(void), const char *name) return -1; if ((p->pgdir = setupkvm()) == 0) { - kfree(p->kstack); - p->kstack = 0; + p->kstack = 0; // the slot keeps its stack; see kstacks[] p->state = UNUSED; return -1; }