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