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 <noreply@anthropic.com>

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

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)

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);

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){
+  // The stack that belongs to this slot. Cannot fail, which is why there is no
-    p->state = UNUSED;
+  // failure branch here any more -- see kstacks[] above.
-    return 0;
+  p->kstack = kstacks[p - ptable.proc];
-  }
+  // Canary at the very bottom: a slow walk past the end clobbers this first,
-  // Canary at the very bottom of the kstack page: if a deep/nested trap
+  // and it is checked on every trap()/syscall() entry. It does *not* catch a
-  // ever overflows this 4096-byte stack, this is the first thing clobbered.
+  // single frame large enough to step over it, which is the case kstacks[]
-  // Checked on every trap()/syscall() entry (kstack_check_canary()).
+  // 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;   // the slot keeps its stack; see kstacks[]
-    np->kstack = 0;
     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;   // the slot keeps its stack; see kstacks[]
-      np->kstack = 0;
       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;   // the slot keeps its stack; see kstacks[]
-        p->kstack = 0;
         // 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;   // the slot keeps its stack; see kstacks[]
-    p->kstack = 0;
     p->state = UNUSED;
     return -1;
   }