From 28e3a60f431a471d12cda2fc81b83aa47ae068bb Mon Sep 17 00:00:00 2001 From: Thomas Harrison Date: Sat, 5 Sep 2026 10:55:05 -0400 Subject: [PATCH 01/10] feat: grow process stack on demand via page faults Remove the eager 1022-page stack pre-allocation loop in exec.c and loader.c so a new process starts with only the single stack page process_create maps. Instead, the page fault handler now grows the stack by one page when a user-mode fault lands on a non-present page immediately below the process' current stack end (the next page process_grow_stack would map), then retries the faulting instruction. Any other fault (protection fault, supervisor fault, or a fault further away) still falls through to the existing diagnostic print and panic. --- src/cpu/i386/src/isr.c | 46 +++++++++++++++++++++++++++++++++++++++++ src/kernel/src/exec.c | 12 ----------- src/kernel/src/loader.c | 9 -------- 3 files changed, 46 insertions(+), 21 deletions(-) diff --git a/src/cpu/i386/src/isr.c b/src/cpu/i386/src/isr.c index 10375b6..5b484e7 100644 --- a/src/cpu/i386/src/isr.c +++ b/src/cpu/i386/src/isr.c @@ -2,8 +2,13 @@ #include "cpu/isr.h" +#include + +#include "addr.h" #include "cpu/idt.h" +#include "cpu/mmu.h" #include "cpu/ports.h" +#include "defs.h" #include "kernel.h" #include "kernel/logs.h" #include "libc/proc.h" @@ -124,7 +129,48 @@ char * exception_messages[] = { "Reserved", }; +/** + * @brief Try to handle a page fault by growing the faulting process' user + * stack by one page. + * + * Only handles the case where a user-mode access faults on a non-present + * page immediately below the process' current stack end (ie. the next page + * `process_grow_stack` would map) - anything else (protection faults, + * supervisor faults, or a fault further away, eg. from a wild pointer) is + * left for the caller to report and panic on. + * + * @param r page fault registers (int_no == 14) + * @return int 0 if the fault was handled by growing the stack, non-zero otherwise + */ +static int handle_stack_growth_fault(registers_t * r) { + bool user_mode = (r->err_code & MMU_DIR_FLAG_USER_SUPERVISOR) != 0; + bool page_present = (r->err_code & MMU_DIR_FLAG_PRESENT) != 0; + + if (!user_mode || page_present) { + return -1; + } + + process_t * proc = get_current_process(); + if (!proc) { + return -1; + } + + size_t fault_page = ADDR2PAGE(r->cr2); + size_t next_stack_page = ADDR2PAGE(VADDR_USER_STACK) - proc->stack_page_count; + + if (fault_page != next_stack_page) { + return -1; + } + + return process_grow_stack(proc); +} + void isr_handler(registers_t r) { + if (r.int_no == 14 && !handle_stack_growth_fault(&r)) { + // Stack grown by one page, retry the faulting instruction. + return; + } + print_trace(&r); printf("ISR %u (err 0x%X)\n", r.int_no, r.err_code); printf("%s\n", exception_messages[r.int_no]); diff --git a/src/kernel/src/exec.c b/src/kernel/src/exec.c index 6ca1f8d..0213689 100644 --- a/src/kernel/src/exec.c +++ b/src/kernel/src/exec.c @@ -54,18 +54,6 @@ int command_exec(uint8_t * buff, const char * filepath, size_t size, size_t argc return -1; } - KLOG_TRACE("Increasing stack by 1022 pages"); - for (size_t i = 0; i < 1022; i++) { - if (process_grow_stack(proc)) { - KLOG_DEBUG("Failed to add page %u", i); - if (process_free(proc)) { - KPANIC("Failed to free process while handling stack grow error"); - } - kfree(proc); - return -1; - } - } - if (process_copy_args(proc, filepath, argc, argv)) { KLOG_DEBUG("Failed to copy args"); if (process_free(proc)) { diff --git a/src/kernel/src/loader.c b/src/kernel/src/loader.c index 29ad658..d6aecc7 100644 --- a/src/kernel/src/loader.c +++ b/src/kernel/src/loader.c @@ -237,15 +237,6 @@ static process_t * load_init() { return 0; } - for (size_t i = 0; i < 1022; i++) { - if (process_grow_stack(proc)) { - KLOG_DEBUG("Failed to grow process stack"); - return 0; - } - } - - KLOG_TRACE("Stack allocated for process %u", proc->pid); - process_copy_args(proc, filename, 0, 0); KLOG_TRACE("Copied filename %s to process %u", filename, proc->pid); From 3a4b0036bac407322a8aa507392d012140c5d67b Mon Sep 17 00:00:00 2001 From: Thomas Harrison Date: Sat, 12 Sep 2026 13:58:53 -0400 Subject: [PATCH 02/10] fix: missing filename arg in kernel_exec log call KLOG_DEBUG("Process for exec %d finished for filename %s", pid) was missing the filename argument for its %s specifier, so it read garbage off the stack as a string pointer. This corrupted/misread memory when formatting the log line and was part of the crash chain reproduced by running 'cat data/a.txt' in the shell (still investigating whether a further root cause remains). --- src/kernel/src/kernel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kernel/src/kernel.c b/src/kernel/src/kernel.c index 184827d..34c5068 100644 --- a/src/kernel/src/kernel.c +++ b/src/kernel/src/kernel.c @@ -167,7 +167,7 @@ int kernel_exec(const char * filename, size_t argc, char ** argv) { return -1; } - KLOG_DEBUG("Process for exec %d finished for filename %s", pid); + KLOG_DEBUG("Process for exec %d finished for filename %s", pid, filename); io_device_close(file); KLOG_TRACE("File device closed"); From 15f936a3aef7116b6ae41d8be2e581ff734ddcaf Mon Sep 17 00:00:00 2001 From: Thomas Harrison Date: Sat, 12 Sep 2026 15:47:26 -0400 Subject: [PATCH 03/10] fix: grow all missing stack pages per fault, add configurable max stack size A single stack frame can move esp down by more than one page before its first access (eg. a large local array), and the toolchain doesn't emit a stack probe for every page in between. handle_stack_growth_fault() only grew exactly one page when the fault was immediately below the current stack end, so a fault further below incorrectly fell through to a panic instead of growing the stack. - isr.c: validate the fault against the saved user ESP (a legitimate stack access is always at or above esp) instead of requiring an exact match, then loop process_grow_stack() to map every missing page down to the fault. - process.h/process.c: add a per-process max_stack_pages field (seeded from a new KERNEL_MAX_STACK_PAGES kernel parameter, default 8192 KB / 2048 pages) and enforce it in process_grow_stack, so the new multi-page loop still correctly stops and fails instead of growing unbounded. Add process_set_max_stack_pages to change the limit per-process at runtime; it rejects shrinking below the currently allocated page count (with a TODO to base that on actual stack usage instead, once tracked, so shrinking can free pages outside the new region). - Mirror the new kernel parameter in the test build's config.h so process.c still compiles under test, and add/extend test_process.cpp coverage for the new field, limit check, and setter function. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CMakeLists.txt | 7 +++++ cmake/config.h.in | 1 + design/kernel_parameters.md | 1 + src/cpu/i386/src/isr.c | 52 ++++++++++++++++++++++++++----- src/kernel/include/process.h | 31 ++++++++++++++++-- src/kernel/src/process.c | 27 ++++++++++++++++ tests/CMakeLists.txt | 3 ++ tests/config/config.h.in | 2 ++ tests/src/kernel/test_process.cpp | 48 ++++++++++++++++++++++++++-- 9 files changed, 161 insertions(+), 11 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e3d534d..2b4e0e7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10,6 +10,13 @@ set(CMAKE_TOOLCHAIN_FILE cmake/toolchain.cmake) # Extra checks in malloc set(KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED 1) +# Default maximum number of pages a process' stack is allowed to grow to +# (8192 KB / 8 MB, converted to pages since PAGE_SIZE is 4096 bytes). Each +# process stores its own copy of this limit (process_t.max_stack_pages), +# seeded from this default in process_create, so it can be changed +# per-process at runtime via process_set_max_stack_pages. +math(EXPR KERNEL_MAX_STACK_PAGES "8192 * 1024 / 4096") + execute_process( COMMAND git describe --tags --abbrev=0 OUTPUT_VARIABLE OS_VERSION diff --git a/cmake/config.h.in b/cmake/config.h.in index e4ec1ac..f712e0a 100644 --- a/cmake/config.h.in +++ b/cmake/config.h.in @@ -11,6 +11,7 @@ #define CPU_ARCH "@CPU_ARCH@" #define FILE_PREFIX_LENGTH @FILE_PREFIX_LENGTH@ #define KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED @KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED@ +#define KERNEL_MAX_STACK_PAGES @KERNEL_MAX_STACK_PAGES@ #define PROJECT_DESCRIPTION PROJECT_NAME \ " version=" PROJECT_VERSION \ diff --git a/design/kernel_parameters.md b/design/kernel_parameters.md index 9b9b0ea..99ad269 100644 --- a/design/kernel_parameters.md +++ b/design/kernel_parameters.md @@ -16,4 +16,5 @@ generated from `cmake/config.h.in`). | CPU_ARCH | CPU Architecture | | FILE_PREFIX_LENGTH | Length of absolute path to `src/`, used in logger macro to isolate relative file path | | KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED | Perform extra checks during malloc operations | +| KERNEL_MAX_STACK_PAGES | Default maximum number of pages a process' stack may grow to (seeds `process_t.max_stack_pages`, which can be changed per-process at runtime via `process_set_max_stack_pages`) | | PROJECT_DESCRIPTION | String that combines name, version and arguments to the kernel | diff --git a/src/cpu/i386/src/isr.c b/src/cpu/i386/src/isr.c index 5b484e7..ab29c21 100644 --- a/src/cpu/i386/src/isr.c +++ b/src/cpu/i386/src/isr.c @@ -131,13 +131,26 @@ char * exception_messages[] = { /** * @brief Try to handle a page fault by growing the faulting process' user - * stack by one page. + * stack down to (and including) the faulting page. * * Only handles the case where a user-mode access faults on a non-present - * page immediately below the process' current stack end (ie. the next page - * `process_grow_stack` would map) - anything else (protection faults, - * supervisor faults, or a fault further away, eg. from a wild pointer) is - * left for the caller to report and panic on. + * page at or below the process' current stack end, at or above the + * process' current (already adjusted) ring 3 stack pointer. A single stack + * frame can fault more than one page below the previous stack end if it + * moves esp down by more than a page before its first access (eg. a large + * local array) and the toolchain does not emit a stack probe for every + * page in between - so every missing page down to the fault is grown, not + * just one. + * + * The saved user esp check rejects wild/unrelated pointers that happen to + * land on some other unmapped page below the stack region: any legitimate + * access within a stack frame is always at or above esp, since the frame + * occupies [esp, esp + frame size). + * + * Anything else (protection faults, supervisor faults, a fault above the + * saved esp or below the current stack end, or a `process_grow_stack` + * failure eg. from the heap collision guard or the max stack size limit) + * is left for the caller to report and panic on. * * @param r page fault registers (int_no == 14) * @return int 0 if the fault was handled by growing the stack, non-zero otherwise @@ -158,11 +171,36 @@ static int handle_stack_growth_fault(registers_t * r) { size_t fault_page = ADDR2PAGE(r->cr2); size_t next_stack_page = ADDR2PAGE(VADDR_USER_STACK) - proc->stack_page_count; - if (fault_page != next_stack_page) { + // The fault must be at or below the current stack end (ie. actually + // extending the stack, not touching already-mapped memory or memory + // above the stack region). + if (fault_page > next_stack_page) { return -1; } - return process_grow_stack(proc); + // The fault must be at or above the saved user stack pointer - a + // legitimate stack access can never be below esp, since the CPU only + // ever dereferences addresses within [esp, esp + frame size) for stack + // memory. A fault below esp is a wild/unrelated pointer, not stack + // growth. + if (fault_page < ADDR2PAGE(r->useresp)) { + return -1; + } + + // Grow every missing page from the current stack end down to (and + // including) the faulting page. process_grow_stack enforces the + // heap/stack collision guard and the max stack size limit on each + // individual page it adds, so this correctly stops and fails if + // growth would collide with the heap or exceed the limit partway + // through. + while (next_stack_page >= fault_page) { + if (process_grow_stack(proc)) { + return -1; + } + next_stack_page = ADDR2PAGE(VADDR_USER_STACK) - proc->stack_page_count; + } + + return 0; } void isr_handler(registers_t r) { diff --git a/src/kernel/include/process.h b/src/kernel/include/process.h index 5a41717..30c6a32 100644 --- a/src/kernel/include/process.h +++ b/src/kernel/include/process.h @@ -68,6 +68,10 @@ typedef struct _process { uint32_t next_heap_page; /// Number of pages allocated to the stack uint32_t stack_page_count; + /// Maximum number of pages the stack may grow to. Seeded from + /// KERNEL_MAX_STACK_PAGES at process creation; change with + /// process_set_max_stack_pages, not directly. + uint32_t max_stack_pages; /// PID of parent process (0 is no parent) uint32_t parent_pid; @@ -164,13 +168,36 @@ void * process_add_pages(process_t * proc, size_t count); * * Fails without allocating anything if the new page would collide with (or * come within HEAP_STACK_GUARD_PAGES of) the process' own heap, so the stack - * and heap can never overlap. + * and heap can never overlap. Also fails without allocating anything if the + * stack has already reached proc->max_stack_pages. * * @param proc pointer to the process object - * @return int 0 for success, -1 if it would collide with the heap or on error + * @return int 0 for success, -1 if it would collide with the heap, would + * exceed proc->max_stack_pages, or on error */ int process_grow_stack(process_t * proc); +/** + * @brief Change the maximum number of pages a process' stack may grow to. + * + * Fails without changing anything if `max_stack_pages` is smaller than the + * number of pages currently allocated to the stack + * (proc->stack_page_count) - shrinking below what's already mapped is not + * supported. + * + * @todo This currently checks against *allocated* stack pages + * (stack_page_count), not how much of the stack is actually *in use*. Once + * that's tracked, this should instead check against actual stack usage, and + * shrinking should free any allocated pages that fall outside the new, + * smaller limit. + * + * @param proc pointer to the process object + * @param max_stack_pages new maximum number of stack pages + * @return int 0 for success, -1 if null process or max_stack_pages is + * smaller than the currently allocated stack page count + */ +int process_set_max_stack_pages(process_t * proc, uint32_t max_stack_pages); + /** * @brief Allocate pages in the heap and copy data from `buff` into the pages. * diff --git a/src/kernel/src/process.c b/src/kernel/src/process.c index d2138c4..9e18b45 100644 --- a/src/kernel/src/process.c +++ b/src/kernel/src/process.c @@ -2,6 +2,7 @@ #include "process.h" +#include "config.h" #include "cpu/gdt.h" #include "cpu/mmu.h" #include "cpu/tss.h" @@ -93,6 +94,7 @@ int process_create(process_t * proc) { proc->pid = next_pid(); proc->next_heap_page = ADDR2PAGE(VADDR_USER_MEM); proc->stack_page_count = 1; + proc->max_stack_pages = KERNEL_MAX_STACK_PAGES; // TODO parent process pid @@ -426,6 +428,11 @@ int process_grow_stack(process_t * proc) { return -1; } + if (proc->stack_page_count >= proc->max_stack_pages) { + KLOG_WARNING("Cannot grow stack past %u pages (max_stack_pages)", proc->max_stack_pages); + return -1; + } + // Stack pages grow down starting immediately below the first user stack // page allocated by process_create (at ADDR2PAGE(VADDR_USER_STACK)). // proc->stack_page_count starts at 1 (that first page), so the Nth call @@ -463,6 +470,26 @@ int process_grow_stack(process_t * proc) { return 0; } +int process_set_max_stack_pages(process_t * proc, uint32_t max_stack_pages) { + if (!proc) { + KLOG_WARNING("Process struct is null pointer"); + return -1; + } + + // TODO this should check against actual stack usage rather than pages + // allocated so far (proc->stack_page_count) - once usage is tracked, + // shrinking below the current allocation should free the pages that + // fall outside the new, smaller limit instead of being rejected outright. + if (max_stack_pages < proc->stack_page_count) { + KLOG_WARNING("Cannot set max stack pages to %u, %u pages are already allocated", max_stack_pages, proc->stack_page_count); + return -1; + } + + proc->max_stack_pages = max_stack_pages; + + return 0; +} + /** * @brief Copy `size` bytes from `buff` (kernel memory) into `count` pages of * the process' own address space, starting at page index `page_start`. The diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 20c8b9c..0e7fad0 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -12,6 +12,9 @@ set(CMAKE_C_STANDARD 17) # Set the global FFF_GCC_FUNCTION_ATTRIBUTES for config.h set(FFF_GCC_FUNCTION_ATTRIBUTES "__attribute__((weak))") +# Mirrors the default in the root CMakeLists.txt so process.c (which reads +# this to seed process_t.max_stack_pages) compiles the same way under test. +math(EXPR KERNEL_MAX_STACK_PAGES "8192 * 1024 / 4096") configure_file(${CMAKE_CURRENT_SOURCE_DIR}/config/config.h.in ${CMAKE_BINARY_DIR}/include/config.h) include_directories(PRIVATE ${CMAKE_BINARY_DIR}/include) diff --git a/tests/config/config.h.in b/tests/config/config.h.in index 30121ce..948dbcd 100644 --- a/tests/config/config.h.in +++ b/tests/config/config.h.in @@ -3,3 +3,5 @@ */ #cmakedefine FFF_GCC_FUNCTION_ATTRIBUTES @FFF_GCC_FUNCTION_ATTRIBUTES@ + +#define KERNEL_MAX_STACK_PAGES @KERNEL_MAX_STACK_PAGES@ diff --git a/tests/src/kernel/test_process.cpp b/tests/src/kernel/test_process.cpp index 3398ce8..3e008fa 100644 --- a/tests/src/kernel/test_process.cpp +++ b/tests/src/kernel/test_process.cpp @@ -5,6 +5,7 @@ extern "C" { #include "addr.h" +#include "config.h" #include "cpu/gdt.h" #include "cpu/mmu.h" #include "libc/datastruct/array.h" @@ -63,7 +64,8 @@ class Process : public ::testing::Test { heap_data[i] = i % 0xff; } - proc.next_heap_page = 2; + proc.next_heap_page = 2; + proc.max_stack_pages = KERNEL_MAX_STACK_PAGES; mmu_dir_set_fake.custom_fake = custom_mmu_dir_set; mmu_table_set_fake.custom_fake = custom_mmu_table_set; @@ -160,6 +162,7 @@ TEST_F(Process, process_create) { EXPECT_EQ(12, proc.pid); EXPECT_EQ(1024, proc.next_heap_page); EXPECT_EQ(1, proc.stack_page_count); + EXPECT_EQ((uint32_t)KERNEL_MAX_STACK_PAGES, proc.max_stack_pages); EXPECT_EQ(0xffffffff, proc.esp); EXPECT_EQ(0xffffffff, proc.esp0); @@ -518,8 +521,11 @@ TEST_F(Process, process_grow_stack_CollidesWithHeap) { // Simulate a stack that has already grown down to meet the process' // current heap top (next_heap_page defaults to 2 in SetUp). The next // grown page would land exactly on next_heap_page, so it must be - // rejected instead of overlapping the heap. + // rejected instead of overlapping the heap. max_stack_pages is set high + // enough here that only the heap collision check can be responsible for + // the rejection. proc.stack_page_count = (uint32_t)ADDR2PAGE(VADDR_USER_STACK) - proc.next_heap_page; + proc.max_stack_pages = proc.stack_page_count + 1; paging_temp_map_fake.return_val = &dir; uint32_t stack_page_count_before = proc.stack_page_count; @@ -531,6 +537,44 @@ TEST_F(Process, process_grow_stack_CollidesWithHeap) { ASSERT_TEMP_MAP_BALANCED(); } +TEST_F(Process, process_grow_stack_AtMaxStackPages) { + // Stack has already grown to the process' configured limit, so the next + // grow must be rejected without touching paging at all. + proc.max_stack_pages = 4; + proc.stack_page_count = proc.max_stack_pages; + paging_temp_map_fake.return_val = &dir; + + EXPECT_NE(0, process_grow_stack(&proc)); + EXPECT_EQ(0, paging_temp_map_fake.call_count); + EXPECT_EQ(0, paging_add_pages_fake.call_count); + EXPECT_EQ(4u, proc.stack_page_count); +} + +// Process Set Max Stack Pages + +TEST_F(Process, process_set_max_stack_pages_InvalidParameters) { + EXPECT_NE(0, process_set_max_stack_pages(0, 100)); +} + +TEST_F(Process, process_set_max_stack_pages_TooSmall) { + proc.stack_page_count = 5; + proc.max_stack_pages = KERNEL_MAX_STACK_PAGES; + + // Cannot shrink the limit below the number of pages already allocated + EXPECT_NE(0, process_set_max_stack_pages(&proc, 4)); + EXPECT_EQ((uint32_t)KERNEL_MAX_STACK_PAGES, proc.max_stack_pages); +} + +TEST_F(Process, process_set_max_stack_pages) { + proc.stack_page_count = 5; + + EXPECT_EQ(0, process_set_max_stack_pages(&proc, 5)); + EXPECT_EQ(5u, proc.max_stack_pages); + + EXPECT_EQ(0, process_set_max_stack_pages(&proc, 100)); + EXPECT_EQ(100u, proc.max_stack_pages); +} + // Process Load Heap TEST_F(Process, process_load_heap_InvalidParameters) { From b4ee08a223dd1ad4ba99d96fe46bb3093972cb19 Mon Sep 17 00:00:00 2001 From: Thomas Harrison Date: Sat, 12 Sep 2026 16:38:00 -0400 Subject: [PATCH 04/10] docs: drop return type from @return in new docstrings @return int 0 for success / @return int 0 if ... -> @return 0 for success / @return 0 if ..., matching a cleaner style without repeating the function's C return type in the doc text. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cpu/i386/src/isr.c | 2 +- src/kernel/include/process.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cpu/i386/src/isr.c b/src/cpu/i386/src/isr.c index ab29c21..041720c 100644 --- a/src/cpu/i386/src/isr.c +++ b/src/cpu/i386/src/isr.c @@ -153,7 +153,7 @@ char * exception_messages[] = { * is left for the caller to report and panic on. * * @param r page fault registers (int_no == 14) - * @return int 0 if the fault was handled by growing the stack, non-zero otherwise + * @return 0 if the fault was handled by growing the stack, non-zero otherwise */ static int handle_stack_growth_fault(registers_t * r) { bool user_mode = (r->err_code & MMU_DIR_FLAG_USER_SUPERVISOR) != 0; diff --git a/src/kernel/include/process.h b/src/kernel/include/process.h index 30c6a32..1dd6be4 100644 --- a/src/kernel/include/process.h +++ b/src/kernel/include/process.h @@ -172,7 +172,7 @@ void * process_add_pages(process_t * proc, size_t count); * stack has already reached proc->max_stack_pages. * * @param proc pointer to the process object - * @return int 0 for success, -1 if it would collide with the heap, would + * @return 0 for success, -1 if it would collide with the heap, would * exceed proc->max_stack_pages, or on error */ int process_grow_stack(process_t * proc); @@ -193,7 +193,7 @@ int process_grow_stack(process_t * proc); * * @param proc pointer to the process object * @param max_stack_pages new maximum number of stack pages - * @return int 0 for success, -1 if null process or max_stack_pages is + * @return 0 for success, -1 if null process or max_stack_pages is * smaller than the currently allocated stack page count */ int process_set_max_stack_pages(process_t * proc, uint32_t max_stack_pages); From 1926654dc3d4c437dac7287e46f7a9efd3ee0cd6 Mon Sep 17 00:00:00 2001 From: Thomas Harrison Date: Sat, 12 Sep 2026 16:41:44 -0400 Subject: [PATCH 05/10] Refactor ISR handling in i386 CPU code Remove unnecessary code and improve readability in isr.c. --- src/cpu/i386/src/isr.c | 23 ++--------------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/src/cpu/i386/src/isr.c b/src/cpu/i386/src/isr.c index 041720c..11aabba 100644 --- a/src/cpu/i386/src/isr.c +++ b/src/cpu/i386/src/isr.c @@ -130,27 +130,8 @@ char * exception_messages[] = { }; /** - * @brief Try to handle a page fault by growing the faulting process' user - * stack down to (and including) the faulting page. - * - * Only handles the case where a user-mode access faults on a non-present - * page at or below the process' current stack end, at or above the - * process' current (already adjusted) ring 3 stack pointer. A single stack - * frame can fault more than one page below the previous stack end if it - * moves esp down by more than a page before its first access (eg. a large - * local array) and the toolchain does not emit a stack probe for every - * page in between - so every missing page down to the fault is grown, not - * just one. - * - * The saved user esp check rejects wild/unrelated pointers that happen to - * land on some other unmapped page below the stack region: any legitimate - * access within a stack frame is always at or above esp, since the frame - * occupies [esp, esp + frame size). - * - * Anything else (protection faults, supervisor faults, a fault above the - * saved esp or below the current stack end, or a `process_grow_stack` - * failure eg. from the heap collision guard or the max stack size limit) - * is left for the caller to report and panic on. + * @brief Try to handle a page fault by growing the faulting process' stack to + * (and including) the faulting page. * * @param r page fault registers (int_no == 14) * @return 0 if the fault was handled by growing the stack, non-zero otherwise From adc098eb36893e5dd2c1a617297e5f1cb67cf8dd Mon Sep 17 00:00:00 2001 From: Thomas Harrison Date: Sat, 12 Sep 2026 16:49:19 -0400 Subject: [PATCH 06/10] refactor: express max stack size kernel param in KB, remove test casts Per review feedback: KERNEL_MAX_STACK_PAGES (a raw page count computed at cmake configure time) is now KERNEL_MAX_STACK_SIZE_KB, expressed in KB like a size a person would actually configure. process_t.max_stack_pages still stores the limit as a page count internally; process_create converts the new kernel default once via a new stack_size_kb_to_pages() helper (rounding up to a whole page). process_set_max_stack_pages is renamed to process_set_max_stack_size_kb and now takes the new limit in KB, rounding up to a whole page the same way, instead of requiring the caller to already know/compute a page count. Also removed the (uint32_t) casts flagged in test_process.cpp - EXPECT_EQ compares fine across the two integer types without help, so the casts were unnecessary. The affected expected-value expressions were rewritten using a small test-local kb_to_pages() helper (mirroring the kernel-side conversion) instead of directly against the pages macro, since the parameter itself is no longer a page count. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CMakeLists.txt | 12 ++++----- cmake/config.h.in | 2 +- design/kernel_parameters.md | 2 +- src/kernel/include/process.h | 19 +++++++------ src/kernel/src/process.c | 27 ++++++++++++++----- tests/CMakeLists.txt | 2 +- tests/config/config.h.in | 2 +- tests/src/kernel/test_process.cpp | 44 ++++++++++++++++++++++--------- 8 files changed, 73 insertions(+), 37 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 2b4e0e7..4ad54d5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10,12 +10,12 @@ set(CMAKE_TOOLCHAIN_FILE cmake/toolchain.cmake) # Extra checks in malloc set(KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED 1) -# Default maximum number of pages a process' stack is allowed to grow to -# (8192 KB / 8 MB, converted to pages since PAGE_SIZE is 4096 bytes). Each -# process stores its own copy of this limit (process_t.max_stack_pages), -# seeded from this default in process_create, so it can be changed -# per-process at runtime via process_set_max_stack_pages. -math(EXPR KERNEL_MAX_STACK_PAGES "8192 * 1024 / 4096") +# Default maximum size (in KB) a process' stack is allowed to grow to. Each +# process stores its own copy of this limit as a page count +# (process_t.max_stack_pages), seeded from this default (rounded up to a +# whole number of pages) in process_create, so it can be changed per-process +# at runtime via process_set_max_stack_size_kb. +set(KERNEL_MAX_STACK_SIZE_KB 8192) execute_process( COMMAND git describe --tags --abbrev=0 diff --git a/cmake/config.h.in b/cmake/config.h.in index f712e0a..e36e56c 100644 --- a/cmake/config.h.in +++ b/cmake/config.h.in @@ -11,7 +11,7 @@ #define CPU_ARCH "@CPU_ARCH@" #define FILE_PREFIX_LENGTH @FILE_PREFIX_LENGTH@ #define KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED @KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED@ -#define KERNEL_MAX_STACK_PAGES @KERNEL_MAX_STACK_PAGES@ +#define KERNEL_MAX_STACK_SIZE_KB @KERNEL_MAX_STACK_SIZE_KB@ #define PROJECT_DESCRIPTION PROJECT_NAME \ " version=" PROJECT_VERSION \ diff --git a/design/kernel_parameters.md b/design/kernel_parameters.md index 99ad269..bb76346 100644 --- a/design/kernel_parameters.md +++ b/design/kernel_parameters.md @@ -16,5 +16,5 @@ generated from `cmake/config.h.in`). | CPU_ARCH | CPU Architecture | | FILE_PREFIX_LENGTH | Length of absolute path to `src/`, used in logger macro to isolate relative file path | | KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED | Perform extra checks during malloc operations | -| KERNEL_MAX_STACK_PAGES | Default maximum number of pages a process' stack may grow to (seeds `process_t.max_stack_pages`, which can be changed per-process at runtime via `process_set_max_stack_pages`) | +| KERNEL_MAX_STACK_SIZE_KB | Default maximum size in KB a process' stack may grow to (seeds `process_t.max_stack_pages`, rounded up to a whole page; can be changed per-process at runtime via `process_set_max_stack_size_kb`) | | PROJECT_DESCRIPTION | String that combines name, version and arguments to the kernel | diff --git a/src/kernel/include/process.h b/src/kernel/include/process.h index 1dd6be4..1b746b6 100644 --- a/src/kernel/include/process.h +++ b/src/kernel/include/process.h @@ -69,8 +69,8 @@ typedef struct _process { /// Number of pages allocated to the stack uint32_t stack_page_count; /// Maximum number of pages the stack may grow to. Seeded from - /// KERNEL_MAX_STACK_PAGES at process creation; change with - /// process_set_max_stack_pages, not directly. + /// KERNEL_MAX_STACK_SIZE_KB (rounded up to a whole page) at process + /// creation; change with process_set_max_stack_size_kb, not directly. uint32_t max_stack_pages; /// PID of parent process (0 is no parent) @@ -178,10 +178,13 @@ void * process_add_pages(process_t * proc, size_t count); int process_grow_stack(process_t * proc); /** - * @brief Change the maximum number of pages a process' stack may grow to. + * @brief Change the maximum size (in KB) a process' stack may grow to. * - * Fails without changing anything if `max_stack_pages` is smaller than the - * number of pages currently allocated to the stack + * `max_stack_size_kb` is rounded up to the next whole page (proc->max_stack_pages + * is always a page count) if it is not already page aligned. + * + * Fails without changing anything if, after rounding, the new limit is + * smaller than the number of pages currently allocated to the stack * (proc->stack_page_count) - shrinking below what's already mapped is not * supported. * @@ -192,11 +195,11 @@ int process_grow_stack(process_t * proc); * smaller limit. * * @param proc pointer to the process object - * @param max_stack_pages new maximum number of stack pages - * @return 0 for success, -1 if null process or max_stack_pages is + * @param max_stack_size_kb new maximum stack size in KB + * @return 0 for success, -1 if null process or the rounded page count is * smaller than the currently allocated stack page count */ -int process_set_max_stack_pages(process_t * proc, uint32_t max_stack_pages); +int process_set_max_stack_size_kb(process_t * proc, uint32_t max_stack_size_kb); /** * @brief Allocate pages in the heap and copy data from `buff` into the pages. diff --git a/src/kernel/src/process.c b/src/kernel/src/process.c index 9e18b45..74cdfee 100644 --- a/src/kernel/src/process.c +++ b/src/kernel/src/process.c @@ -17,13 +17,26 @@ static int open_stdio_handles(process_t * proc); -static char * copy_string(const char * str); -static int copy_to_process_pages(process_t * proc, uint32_t page_start, size_t count, const char * buff, size_t size); -static int write_process_dwords(process_t * proc, uint32_t first_addr, const uint32_t * values, size_t count); +static char * copy_string(const char * str); +static int copy_to_process_pages(process_t * proc, uint32_t page_start, size_t count, const char * buff, size_t size); +static int write_process_dwords(process_t * proc, uint32_t first_addr, const uint32_t * values, size_t count); +static uint32_t stack_size_kb_to_pages(uint32_t size_kb); static uint32_t next_pid(); static uint32_t next_handle_id(); +/** + * @brief Convert a stack size in KB to a whole number of pages, rounding up + * if `size_kb` is not already page aligned. + * + * @param size_kb stack size in KB + * @return number of pages + */ +static uint32_t stack_size_kb_to_pages(uint32_t size_kb) { + uint32_t size_bytes = size_kb * 1024; + return (size_bytes + PAGE_SIZE - 1) / PAGE_SIZE; +} + int process_create(process_t * proc) { if (!proc) { KLOG_WARNING("Process struct is null pointer"); @@ -94,7 +107,7 @@ int process_create(process_t * proc) { proc->pid = next_pid(); proc->next_heap_page = ADDR2PAGE(VADDR_USER_MEM); proc->stack_page_count = 1; - proc->max_stack_pages = KERNEL_MAX_STACK_PAGES; + proc->max_stack_pages = stack_size_kb_to_pages(KERNEL_MAX_STACK_SIZE_KB); // TODO parent process pid @@ -470,18 +483,20 @@ int process_grow_stack(process_t * proc) { return 0; } -int process_set_max_stack_pages(process_t * proc, uint32_t max_stack_pages) { +int process_set_max_stack_size_kb(process_t * proc, uint32_t max_stack_size_kb) { if (!proc) { KLOG_WARNING("Process struct is null pointer"); return -1; } + uint32_t max_stack_pages = stack_size_kb_to_pages(max_stack_size_kb); + // TODO this should check against actual stack usage rather than pages // allocated so far (proc->stack_page_count) - once usage is tracked, // shrinking below the current allocation should free the pages that // fall outside the new, smaller limit instead of being rejected outright. if (max_stack_pages < proc->stack_page_count) { - KLOG_WARNING("Cannot set max stack pages to %u, %u pages are already allocated", max_stack_pages, proc->stack_page_count); + KLOG_WARNING("Cannot set max stack size to %u KB (%u pages), %u pages are already allocated", max_stack_size_kb, max_stack_pages, proc->stack_page_count); return -1; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 0e7fad0..60098f3 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -14,7 +14,7 @@ set(CMAKE_C_STANDARD 17) set(FFF_GCC_FUNCTION_ATTRIBUTES "__attribute__((weak))") # Mirrors the default in the root CMakeLists.txt so process.c (which reads # this to seed process_t.max_stack_pages) compiles the same way under test. -math(EXPR KERNEL_MAX_STACK_PAGES "8192 * 1024 / 4096") +set(KERNEL_MAX_STACK_SIZE_KB 8192) configure_file(${CMAKE_CURRENT_SOURCE_DIR}/config/config.h.in ${CMAKE_BINARY_DIR}/include/config.h) include_directories(PRIVATE ${CMAKE_BINARY_DIR}/include) diff --git a/tests/config/config.h.in b/tests/config/config.h.in index 948dbcd..59bdf90 100644 --- a/tests/config/config.h.in +++ b/tests/config/config.h.in @@ -4,4 +4,4 @@ #cmakedefine FFF_GCC_FUNCTION_ATTRIBUTES @FFF_GCC_FUNCTION_ATTRIBUTES@ -#define KERNEL_MAX_STACK_PAGES @KERNEL_MAX_STACK_PAGES@ +#define KERNEL_MAX_STACK_SIZE_KB @KERNEL_MAX_STACK_SIZE_KB@ diff --git a/tests/src/kernel/test_process.cpp b/tests/src/kernel/test_process.cpp index 3e008fa..b4107ff 100644 --- a/tests/src/kernel/test_process.cpp +++ b/tests/src/kernel/test_process.cpp @@ -46,6 +46,13 @@ int custom_mmu_table_set(mmu_table_t * table, size_t i, uint32_t addr, uint32_t std::array temp_page; std::array heap_data; +// Mirrors process.c's static stack_size_kb_to_pages helper, so tests can +// compute the expected page count for a given KB size without depending on +// kernel-internal (non-exported) code. +static uint32_t kb_to_pages(uint32_t size_kb) { + return (size_kb * 1024 + PAGE_SIZE - 1) / PAGE_SIZE; +} + class Process : public ::testing::Test { protected: void SetUp() override { @@ -65,7 +72,7 @@ class Process : public ::testing::Test { } proc.next_heap_page = 2; - proc.max_stack_pages = KERNEL_MAX_STACK_PAGES; + proc.max_stack_pages = kb_to_pages(KERNEL_MAX_STACK_SIZE_KB); mmu_dir_set_fake.custom_fake = custom_mmu_dir_set; mmu_table_set_fake.custom_fake = custom_mmu_table_set; @@ -162,7 +169,7 @@ TEST_F(Process, process_create) { EXPECT_EQ(12, proc.pid); EXPECT_EQ(1024, proc.next_heap_page); EXPECT_EQ(1, proc.stack_page_count); - EXPECT_EQ((uint32_t)KERNEL_MAX_STACK_PAGES, proc.max_stack_pages); + EXPECT_EQ(kb_to_pages(KERNEL_MAX_STACK_SIZE_KB), proc.max_stack_pages); EXPECT_EQ(0xffffffff, proc.esp); EXPECT_EQ(0xffffffff, proc.esp0); @@ -550,31 +557,42 @@ TEST_F(Process, process_grow_stack_AtMaxStackPages) { EXPECT_EQ(4u, proc.stack_page_count); } -// Process Set Max Stack Pages +// Process Set Max Stack Size (KB) -TEST_F(Process, process_set_max_stack_pages_InvalidParameters) { - EXPECT_NE(0, process_set_max_stack_pages(0, 100)); +TEST_F(Process, process_set_max_stack_size_kb_InvalidParameters) { + EXPECT_NE(0, process_set_max_stack_size_kb(0, 100)); } -TEST_F(Process, process_set_max_stack_pages_TooSmall) { +TEST_F(Process, process_set_max_stack_size_kb_TooSmall) { proc.stack_page_count = 5; - proc.max_stack_pages = KERNEL_MAX_STACK_PAGES; + proc.max_stack_pages = kb_to_pages(KERNEL_MAX_STACK_SIZE_KB); - // Cannot shrink the limit below the number of pages already allocated - EXPECT_NE(0, process_set_max_stack_pages(&proc, 4)); - EXPECT_EQ((uint32_t)KERNEL_MAX_STACK_PAGES, proc.max_stack_pages); + // 16 KB is exactly 4 pages, less than the 5 pages already allocated + EXPECT_NE(0, process_set_max_stack_size_kb(&proc, 16)); + EXPECT_EQ(kb_to_pages(KERNEL_MAX_STACK_SIZE_KB), proc.max_stack_pages); } -TEST_F(Process, process_set_max_stack_pages) { +TEST_F(Process, process_set_max_stack_size_kb) { proc.stack_page_count = 5; - EXPECT_EQ(0, process_set_max_stack_pages(&proc, 5)); + // 20 KB is exactly 5 pages + EXPECT_EQ(0, process_set_max_stack_size_kb(&proc, 20)); EXPECT_EQ(5u, proc.max_stack_pages); - EXPECT_EQ(0, process_set_max_stack_pages(&proc, 100)); + // 400 KB is exactly 100 pages + EXPECT_EQ(0, process_set_max_stack_size_kb(&proc, 400)); EXPECT_EQ(100u, proc.max_stack_pages); } +TEST_F(Process, process_set_max_stack_size_kb_RoundsUpToPageBoundary) { + proc.stack_page_count = 0; + + // 5 KB is not page aligned (PAGE_SIZE is 4 KB), so it must round up to 2 + // whole pages (8 KB) rather than truncate to 1. + EXPECT_EQ(0, process_set_max_stack_size_kb(&proc, 5)); + EXPECT_EQ(2u, proc.max_stack_pages); +} + // Process Load Heap TEST_F(Process, process_load_heap_InvalidParameters) { From 4c4dd4e3667fb457440fd84082436f05840130ef Mon Sep 17 00:00:00 2001 From: Thomas Harrison Date: Sat, 12 Sep 2026 17:00:55 -0400 Subject: [PATCH 07/10] Refactor process stack growth conditions Remove unused code and clean up CMakeLists.txt, ISR implementation, and process header. --- CMakeLists.txt | 6 +----- src/cpu/i386/src/isr.c | 14 -------------- src/kernel/include/process.h | 17 +++-------------- 3 files changed, 4 insertions(+), 33 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4ad54d5..80d0c19 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10,11 +10,7 @@ set(CMAKE_TOOLCHAIN_FILE cmake/toolchain.cmake) # Extra checks in malloc set(KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED 1) -# Default maximum size (in KB) a process' stack is allowed to grow to. Each -# process stores its own copy of this limit as a page count -# (process_t.max_stack_pages), seeded from this default (rounded up to a -# whole number of pages) in process_create, so it can be changed per-process -# at runtime via process_set_max_stack_size_kb. +# Default maximum size (in KB) a process' stack set(KERNEL_MAX_STACK_SIZE_KB 8192) execute_process( diff --git a/src/cpu/i386/src/isr.c b/src/cpu/i386/src/isr.c index 11aabba..561071e 100644 --- a/src/cpu/i386/src/isr.c +++ b/src/cpu/i386/src/isr.c @@ -152,28 +152,14 @@ static int handle_stack_growth_fault(registers_t * r) { size_t fault_page = ADDR2PAGE(r->cr2); size_t next_stack_page = ADDR2PAGE(VADDR_USER_STACK) - proc->stack_page_count; - // The fault must be at or below the current stack end (ie. actually - // extending the stack, not touching already-mapped memory or memory - // above the stack region). if (fault_page > next_stack_page) { return -1; } - // The fault must be at or above the saved user stack pointer - a - // legitimate stack access can never be below esp, since the CPU only - // ever dereferences addresses within [esp, esp + frame size) for stack - // memory. A fault below esp is a wild/unrelated pointer, not stack - // growth. if (fault_page < ADDR2PAGE(r->useresp)) { return -1; } - // Grow every missing page from the current stack end down to (and - // including) the faulting page. process_grow_stack enforces the - // heap/stack collision guard and the max stack size limit on each - // individual page it adds, so this correctly stops and fails if - // growth would collide with the heap or exceed the limit partway - // through. while (next_stack_page >= fault_page) { if (process_grow_stack(proc)) { return -1; diff --git a/src/kernel/include/process.h b/src/kernel/include/process.h index 1b746b6..21ce939 100644 --- a/src/kernel/include/process.h +++ b/src/kernel/include/process.h @@ -68,9 +68,7 @@ typedef struct _process { uint32_t next_heap_page; /// Number of pages allocated to the stack uint32_t stack_page_count; - /// Maximum number of pages the stack may grow to. Seeded from - /// KERNEL_MAX_STACK_SIZE_KB (rounded up to a whole page) at process - /// creation; change with process_set_max_stack_size_kb, not directly. + /// Maximum number of pages the stack may grow to uint32_t max_stack_pages; /// PID of parent process (0 is no parent) @@ -168,8 +166,7 @@ void * process_add_pages(process_t * proc, size_t count); * * Fails without allocating anything if the new page would collide with (or * come within HEAP_STACK_GUARD_PAGES of) the process' own heap, so the stack - * and heap can never overlap. Also fails without allocating anything if the - * stack has already reached proc->max_stack_pages. + * and heap can never overlap. * * @param proc pointer to the process object * @return 0 for success, -1 if it would collide with the heap, would @@ -178,15 +175,7 @@ void * process_add_pages(process_t * proc, size_t count); int process_grow_stack(process_t * proc); /** - * @brief Change the maximum size (in KB) a process' stack may grow to. - * - * `max_stack_size_kb` is rounded up to the next whole page (proc->max_stack_pages - * is always a page count) if it is not already page aligned. - * - * Fails without changing anything if, after rounding, the new limit is - * smaller than the number of pages currently allocated to the stack - * (proc->stack_page_count) - shrinking below what's already mapped is not - * supported. + * @brief Change the maximum size (in KB) a process' stack may grow to rounding up be page size aligned. * * @todo This currently checks against *allocated* stack pages * (stack_page_count), not how much of the stack is actually *in use*. Once From 0650f4c9dbfeb98622e601382c46218437eb9b54 Mon Sep 17 00:00:00 2001 From: Thomas Harrison Date: Sat, 12 Sep 2026 17:03:44 -0400 Subject: [PATCH 08/10] Update kernel_parameters.md to clarify stack growth conditions --- design/kernel_parameters.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/design/kernel_parameters.md b/design/kernel_parameters.md index bb76346..728bdf5 100644 --- a/design/kernel_parameters.md +++ b/design/kernel_parameters.md @@ -16,5 +16,5 @@ generated from `cmake/config.h.in`). | CPU_ARCH | CPU Architecture | | FILE_PREFIX_LENGTH | Length of absolute path to `src/`, used in logger macro to isolate relative file path | | KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED | Perform extra checks during malloc operations | -| KERNEL_MAX_STACK_SIZE_KB | Default maximum size in KB a process' stack may grow to (seeds `process_t.max_stack_pages`, rounded up to a whole page; can be changed per-process at runtime via `process_set_max_stack_size_kb`) | +| KERNEL_MAX_STACK_SIZE_KB | Default maximum size in KB a process' stack may grow | | PROJECT_DESCRIPTION | String that combines name, version and arguments to the kernel | From cdcd66360d3ad25321fbabd774b1923f7cced745 Mon Sep 17 00:00:00 2001 From: Thomas Harrison Date: Sat, 12 Sep 2026 17:13:59 -0400 Subject: [PATCH 09/10] Fix comment for maximum stack size function Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/kernel/include/process.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kernel/include/process.h b/src/kernel/include/process.h index 21ce939..83ee70a 100644 --- a/src/kernel/include/process.h +++ b/src/kernel/include/process.h @@ -175,7 +175,7 @@ void * process_add_pages(process_t * proc, size_t count); int process_grow_stack(process_t * proc); /** - * @brief Change the maximum size (in KB) a process' stack may grow to rounding up be page size aligned. + * @brief Change the maximum stack size of a process (in KB), rounding up to page-size alignment. * * @todo This currently checks against *allocated* stack pages * (stack_page_count), not how much of the stack is actually *in use*. Once From a944127ef422ff5dd82d9c181410e637e0a8fd54 Mon Sep 17 00:00:00 2001 From: Thomas Harrison Date: Sat, 12 Sep 2026 17:26:50 -0400 Subject: [PATCH 10/10] fix: correct ESP slack, bound multi-page stack growth, fix KB overflow Addresses several PR review findings on the stack-growth page fault path: - isr.c: a faulting PUSH/PUSHA is restartable, so the CPU rolls back to the pre-instruction useresp rather than wherever the instruction had reached. That means a legitimate fault can land up to 32 bytes (PUSHA's worst case) below useresp, which the previous exact useresp check incorrectly rejected at page boundaries. Now allows that slack via MAX_STACK_FAULT_SLACK_BYTES. - isr.c: bound how many pages a single fault can grow the stack by (MAX_STACK_GROWTH_PAGES_PER_FAULT) instead of looping all the way to the faulting page unconditionally - a single fault could otherwise claim the process' entire configured stack limit in one uninterruptible pass (the ISR runs with interrupts disabled). If the cap isn't enough to resolve the fault, the instruction simply re-faults and growth continues from where it left off on the next call. - isr.c: moved handle_stack_growth_fault's definition below all non-static/externally visible functions (with a forward declaration near the top, by the other file-level state), per the static function section of the code style guide. - process.c: same static-function-ordering fix for the new stack_size_kb_to_pages helper, moved to the static helpers section at the end of the file. - process.c: rewrote stack_size_kb_to_pages to do the KB->page conversion entirely in KB (dividing/modulo by the page size in KB) instead of first converting to bytes - the old size_kb * 1024 multiplication (and the + PAGE_SIZE - 1 rounding add) could each silently overflow uint32_t for large inputs, turning a large configured stack size limit into a much smaller (or zero) page count. - process.h: fixed broken grammar in process_set_max_stack_size_kb's brief description. Verified: kernel builds cleanly, full ctest suite passes (13/13), lint (clang-format/header-guards/log-service-names) is clean, and a headless QEMU boot reaches the shell prompt normally with no panics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cpu/i386/src/isr.c | 118 ++++++++++++++++++++++------------- src/kernel/include/process.h | 4 +- src/kernel/src/process.c | 36 +++++++---- 3 files changed, 103 insertions(+), 55 deletions(-) diff --git a/src/cpu/i386/src/isr.c b/src/cpu/i386/src/isr.c index 561071e..cfbcc9d 100644 --- a/src/cpu/i386/src/isr.c +++ b/src/cpu/i386/src/isr.c @@ -18,6 +18,8 @@ static isr_t __interrupt_handlers[256]; +static int handle_stack_growth_fault(registers_t * r); + /* Can't do this with a loop because we need the address * of the function names */ void isr_install() { @@ -129,50 +131,10 @@ char * exception_messages[] = { "Reserved", }; -/** - * @brief Try to handle a page fault by growing the faulting process' stack to - * (and including) the faulting page. - * - * @param r page fault registers (int_no == 14) - * @return 0 if the fault was handled by growing the stack, non-zero otherwise - */ -static int handle_stack_growth_fault(registers_t * r) { - bool user_mode = (r->err_code & MMU_DIR_FLAG_USER_SUPERVISOR) != 0; - bool page_present = (r->err_code & MMU_DIR_FLAG_PRESENT) != 0; - - if (!user_mode || page_present) { - return -1; - } - - process_t * proc = get_current_process(); - if (!proc) { - return -1; - } - - size_t fault_page = ADDR2PAGE(r->cr2); - size_t next_stack_page = ADDR2PAGE(VADDR_USER_STACK) - proc->stack_page_count; - - if (fault_page > next_stack_page) { - return -1; - } - - if (fault_page < ADDR2PAGE(r->useresp)) { - return -1; - } - - while (next_stack_page >= fault_page) { - if (process_grow_stack(proc)) { - return -1; - } - next_stack_page = ADDR2PAGE(VADDR_USER_STACK) - proc->stack_page_count; - } - - return 0; -} - void isr_handler(registers_t r) { if (r.int_no == 14 && !handle_stack_growth_fault(&r)) { - // Stack grown by one page, retry the faulting instruction. + // Stack grown enough to retry the faulting instruction (possibly + // not all the way to the fault yet - see handle_stack_growth_fault). return; } @@ -315,6 +277,78 @@ void print_trace(registers_t * r) { print_cr0(r->cr0); putc('\n'); } + +// A faulting PUSH/PUSHA is restartable: on fault, the CPU rolls back to the +// pre-instruction ESP (the useresp saved on the ring3->ring0 transition), not +// to wherever the in-flight instruction had reached. So the lowest address an +// otherwise-legitimate stack write can fault on is up to this many bytes +// below useresp - PUSHA being the worst case (8 dwords). Without this slack, +// a fault landing just below a page boundary (CR2 one page lower than +// useresp) would be wrongly rejected as illegitimate. +#define MAX_STACK_FAULT_SLACK_BYTES 32 + +// Caps how many pages a single page fault will grow the stack by. Larger +// requests (eg. a huge single access after `sub esp, `) are only +// grown up to this many pages per fault; if that isn't enough to reach the +// faulting page, the faulting instruction is retried, re-faults at the same +// address, and growth continues from there on the next call - bounding how +// much work (and RAM) one fault can commit while interrupts are disabled, +// instead of one fault being able to claim the whole configured stack size +// limit at once. Either way, total stack growth remains bounded by the +// process' own proc->max_stack_pages, enforced in process_grow_stack. +#define MAX_STACK_GROWTH_PAGES_PER_FAULT 16 + +/** + * @brief Try to handle a page fault by growing the faulting process' stack + * down to (or partway to, see MAX_STACK_GROWTH_PAGES_PER_FAULT) the faulting + * page. + * + * Only handles a user-mode fault on a non-present page that is at or below + * the current stack end and at or above (allowing for + * MAX_STACK_FAULT_SLACK_BYTES) the saved user ESP - anything else + * (protection fault, supervisor fault, or a fault outside that range) is + * left for the caller to report and panic on. + * + * @param r page fault registers (int_no == 14) + * @return 0 if the fault was handled (stack grown far enough to retry, even + * if not all the way to the fault - see MAX_STACK_GROWTH_PAGES_PER_FAULT), + * non-zero otherwise + */ +static int handle_stack_growth_fault(registers_t * r) { + bool user_mode = (r->err_code & MMU_DIR_FLAG_USER_SUPERVISOR) != 0; + bool page_present = (r->err_code & MMU_DIR_FLAG_PRESENT) != 0; + + if (!user_mode || page_present) { + return -1; + } + + process_t * proc = get_current_process(); + if (!proc) { + return -1; + } + + size_t fault_page = ADDR2PAGE(r->cr2); + size_t next_stack_page = ADDR2PAGE(VADDR_USER_STACK) - proc->stack_page_count; + + if (fault_page > next_stack_page) { + return -1; + } + + size_t lowest_valid_page = ADDR2PAGE(r->useresp - MAX_STACK_FAULT_SLACK_BYTES); + if (fault_page < lowest_valid_page) { + return -1; + } + + for (int i = 0; i < MAX_STACK_GROWTH_PAGES_PER_FAULT && next_stack_page >= fault_page; i++) { + if (process_grow_stack(proc)) { + return -1; + } + next_stack_page = ADDR2PAGE(VADDR_USER_STACK) - proc->stack_page_count; + } + + return 0; +} + /* Bit Label Description 0 PE Protected Mode Enable diff --git a/src/kernel/include/process.h b/src/kernel/include/process.h index 21ce939..d40b500 100644 --- a/src/kernel/include/process.h +++ b/src/kernel/include/process.h @@ -175,7 +175,9 @@ void * process_add_pages(process_t * proc, size_t count); int process_grow_stack(process_t * proc); /** - * @brief Change the maximum size (in KB) a process' stack may grow to rounding up be page size aligned. + * @brief Change the maximum size (in KB) a process' stack may grow to, + * rounding up to a whole page if `max_stack_size_kb` is not already page + * aligned. * * @todo This currently checks against *allocated* stack pages * (stack_page_count), not how much of the stack is actually *in use*. Once diff --git a/src/kernel/src/process.c b/src/kernel/src/process.c index 74cdfee..2415e5c 100644 --- a/src/kernel/src/process.c +++ b/src/kernel/src/process.c @@ -25,18 +25,6 @@ static uint32_t stack_size_kb_to_pages(uint32_t size_kb); static uint32_t next_pid(); static uint32_t next_handle_id(); -/** - * @brief Convert a stack size in KB to a whole number of pages, rounding up - * if `size_kb` is not already page aligned. - * - * @param size_kb stack size in KB - * @return number of pages - */ -static uint32_t stack_size_kb_to_pages(uint32_t size_kb) { - uint32_t size_bytes = size_kb * 1024; - return (size_bytes + PAGE_SIZE - 1) / PAGE_SIZE; -} - int process_create(process_t * proc) { if (!proc) { KLOG_WARNING("Process struct is null pointer"); @@ -923,3 +911,27 @@ void set_next_handle_id(uint32_t next) { next_handle_id(); // Force handle_id_set to true so it doesn't override this value __handle_id = next; } + +/** + * @brief Convert a stack size in KB to a whole number of pages, rounding up + * if `size_kb` is not already page aligned. + * + * Works entirely in KB (never converting to bytes) so that a large + * `size_kb` cannot overflow uint32_t during the conversion - multiplying by + * 1024 (or adding PAGE_SIZE - 1 to the result) can wrap for inputs + * approaching UINT32_MAX, which would silently turn a large configured + * limit into a much smaller (or zero) page count. + * + * @param size_kb stack size in KB + * @return number of pages + */ +static uint32_t stack_size_kb_to_pages(uint32_t size_kb) { + const uint32_t page_size_kb = PAGE_SIZE / 1024; + + uint32_t pages = size_kb / page_size_kb; + if (size_kb % page_size_kb != 0) { + pages++; + } + + return pages; +}