From 83e044cf68d2d11ee96671d5c5492e4f804b8598 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 01:18:17 +0000 Subject: [PATCH 01/14] Let kernel handle its own page faults --- src/userprog/exception.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/userprog/exception.c b/src/userprog/exception.c index 0a20b53..8a800b7 100644 --- a/src/userprog/exception.c +++ b/src/userprog/exception.c @@ -145,6 +145,14 @@ page_fault (struct intr_frame *f) write = (f->error_code & PF_W) != 0; user = (f->error_code & PF_U) != 0; + /* Kernel page fault is further handled by the kernel itself. */ + if (!user) + { + f->eip = (void *)f->eax; + f->eax = 0xffffffff; + return; + } + /* To implement virtual memory, delete the rest of the function body, and replace it with code that brings in the page to which fault_addr refers. */ -- 2.49.1 From 44f6a85163ee33cf09ec9180fea3a4f9a06b0e52 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 01:21:20 +0000 Subject: [PATCH 02/14] Add get_user and put_user provided by spec. --- src/userprog/syscall.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index ccb02f3..b162253 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -11,6 +11,7 @@ #include "userprog/process.h" #include "userprog/pagedir.h" #include +#include #include static struct lock filesys_lock; @@ -47,6 +48,8 @@ static void syscall_close (int fd); static struct open_file *fd_get_file (int fd); static void *validate_user_pointer (const void *ptr, size_t size); +static int get_user (const uint8_t *); +static bool put_user (uint8_t *, uint8_t); /* A struct defining a syscall_function pointer along with its arity. */ typedef struct @@ -415,3 +418,29 @@ validate_user_pointer (const void *ptr, size_t size) return (void *) ptr; } + +/* PROVIDED BY SPEC. + Reads a byte at user virtual address UADDR. + UADDR must be below PHYS_BASE. + Returns the byte value if successful, -1 if a segfault occurred. */ +static int +get_user (const uint8_t *uaddr) +{ + int result; + asm ("movl $1f, %0; movzbl %1, %0; 1:" : "=&a"(result) : "m"(*uaddr)); + return result; +} + +/* PROVIDED BY SPEC. + Writes BYTE to user address UDST. + UDST must be below PHYS_BASE. + Returns true if successful, false if a segfault occurred. */ +static bool +put_user (uint8_t *udst, uint8_t byte) +{ + int error_code; + asm ("movl $1f, %0; movb %b2, %1; 1:" + : "=&a"(error_code), "=m"(*udst) + : "q"(byte)); + return error_code != -1; +} \ No newline at end of file -- 2.49.1 From 9a6abab95ed569c98cb7e0cf87b0428ee023de71 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 01:23:45 +0000 Subject: [PATCH 03/14] Check access to user memory using page fault method (via get_user and put_user). --- src/userprog/syscall.c | 52 ++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index b162253..ca6f116 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -47,7 +47,8 @@ static unsigned syscall_tell (int fd); static void syscall_close (int fd); static struct open_file *fd_get_file (int fd); -static void *validate_user_pointer (const void *ptr, size_t size); +static void *validate_user_pointer (const void *ptr, size_t size, + bool check_write); static int get_user (const uint8_t *); static bool put_user (uint8_t *, uint8_t); @@ -99,8 +100,8 @@ static void syscall_handler (struct intr_frame *f) { /* First, read the system call number from the stack. */ - validate_user_pointer (f->esp, 1); - unsigned syscall_number = *(int *) f->esp; + validate_user_pointer (f->esp, 1, false); + unsigned syscall_number = *(int *)f->esp; /* Ensures the number corresponds to a system call that can be handled. */ if (syscall_number >= LOOKUP_SIZE) @@ -110,10 +111,10 @@ syscall_handler (struct intr_frame *f) /* Next, read and copy the arguments from the stack pointer. */ validate_user_pointer (f->esp + sizeof (uintptr_t), - syscall.arity * sizeof (uintptr_t)); - uintptr_t args[3] = {0}; - for (int i=0; i < syscall.arity; i++) - args[i] = *(uintptr_t *) (f->esp + sizeof (uintptr_t) * (i + 1)); + syscall.arity * sizeof (uintptr_t), false); + uintptr_t args[3] = { 0 }; + for (int i = 0; i < syscall.arity; i++) + args[i] = *(uintptr_t *)(f->esp + sizeof (uintptr_t) * (i + 1)); /* Call the function that handles this system call with the arguments. When there is a return value it is stored in f->eax. */ @@ -144,7 +145,7 @@ syscall_exit (int status) static pid_t syscall_exec (const char *cmd_line) { - validate_user_pointer (cmd_line, 1); + validate_user_pointer (cmd_line, 1, false); lock_acquire (&filesys_lock); pid_t pid = process_execute(cmd_line); @@ -167,7 +168,7 @@ syscall_wait (pid_t pid) static bool syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) { - validate_user_pointer (file, 1); + validate_user_pointer (file, 1, false); lock_acquire (&filesys_lock); bool status = filesys_create (file, initial_size); @@ -182,7 +183,7 @@ syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) static bool syscall_remove (const char *file) { - validate_user_pointer (file, 1); + validate_user_pointer (file, 1, false); lock_acquire (&filesys_lock); bool status = filesys_remove (file); @@ -198,7 +199,7 @@ syscall_remove (const char *file) static int syscall_open (const char *file) { - validate_user_pointer (file, 1); + validate_user_pointer (file, 1, false); lock_acquire (&filesys_lock); struct file *ptr = filesys_open (file); @@ -253,7 +254,7 @@ syscall_read (int fd, void *buffer, unsigned size) if (fd < 0 || fd == STDOUT_FILENO) return -1; - validate_user_pointer (buffer, size); + validate_user_pointer (buffer, size, true); if (fd == STDIN_FILENO) { @@ -290,7 +291,7 @@ syscall_write (int fd, const void *buffer, unsigned size) if (fd <= 0) return 0; - validate_user_pointer (buffer, size); + validate_user_pointer (buffer, size, false); if (fd == STDOUT_FILENO) { @@ -404,19 +405,26 @@ fd_get_file (int fd) } /* Validates if a block of memory starting at PTR and of size SIZE bytes is - fully contained within user virtual memory. Kills the thread (by calling - thread_exit) if the memory is invalid. Otherwise, returns the PTR given. + fully contained within user virtual memory. Returns NULL if the memory + is invalid. Otherwise, returns the PTR given. If the size is 0, the function does no checks and returns PTR.*/ static void * -validate_user_pointer (const void *ptr, size_t size) +validate_user_pointer (const void *ptr, size_t size, bool check_write) { - if (size > 0 && (ptr == NULL || - !is_user_vaddr (ptr) || - !is_user_vaddr (ptr + size - 1) || - pagedir_get_page (thread_current()->pagedir, ptr) == NULL)) + if (size == 0) + return ptr; + /* ptr < ptr + size - 1, so sufficient to check that (ptr + size -1) is a + valid user virtual memory address. */ + if (!is_user_vaddr (ptr + size - 1)) thread_exit (); - - return (void *) ptr; + /* Check read access to pointer. */ + int result; + if ((result = get_user (ptr)) == -1) + thread_exit (); + /* Check write access to pointer (if required). */ + if (check_write && !put_user (ptr, result)) + thread_exit (); + return ptr; } /* PROVIDED BY SPEC. -- 2.49.1 From cf4bf90cbb06eaa90961bdeab41f83a99666a90a Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Tue, 12 Nov 2024 15:34:45 +0000 Subject: [PATCH 04/14] Implement user pointer checking for C strings --- src/userprog/syscall.c | 74 ++++++++++++++++++++++++++++-------------- 1 file changed, 50 insertions(+), 24 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index ca6f116..2b0a551 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -14,6 +14,8 @@ #include #include +#define MAX_SYSCALL_ARGS 3 + static struct lock filesys_lock; static unsigned fd_counter = MIN_USER_FD; @@ -47,8 +49,9 @@ static unsigned syscall_tell (int fd); static void syscall_close (int fd); static struct open_file *fd_get_file (int fd); -static void *validate_user_pointer (const void *ptr, size_t size, - bool check_write); +static void validate_user_pointer (const void *ptr, size_t size, + bool check_write); +static void validate_user_string (const char *str, bool check_write); static int get_user (const uint8_t *); static bool put_user (uint8_t *, uint8_t); @@ -100,8 +103,8 @@ static void syscall_handler (struct intr_frame *f) { /* First, read the system call number from the stack. */ - validate_user_pointer (f->esp, 1, false); - unsigned syscall_number = *(int *)f->esp; + validate_user_pointer (f->esp, sizeof (uintptr_t), false); + uintptr_t syscall_number = *(int *)f->esp; /* Ensures the number corresponds to a system call that can be handled. */ if (syscall_number >= LOOKUP_SIZE) @@ -112,8 +115,8 @@ syscall_handler (struct intr_frame *f) /* Next, read and copy the arguments from the stack pointer. */ validate_user_pointer (f->esp + sizeof (uintptr_t), syscall.arity * sizeof (uintptr_t), false); - uintptr_t args[3] = { 0 }; - for (int i = 0; i < syscall.arity; i++) + uintptr_t args[MAX_SYSCALL_ARGS] = { 0 }; + for (int i = 0; i < syscall.arity && i < MAX_SYSCALL_ARGS; i++) args[i] = *(uintptr_t *)(f->esp + sizeof (uintptr_t) * (i + 1)); /* Call the function that handles this system call with the arguments. When @@ -145,7 +148,7 @@ syscall_exit (int status) static pid_t syscall_exec (const char *cmd_line) { - validate_user_pointer (cmd_line, 1, false); + validate_user_string (cmd_line, false); lock_acquire (&filesys_lock); pid_t pid = process_execute(cmd_line); @@ -168,7 +171,7 @@ syscall_wait (pid_t pid) static bool syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) { - validate_user_pointer (file, 1, false); + validate_user_string (file, false); lock_acquire (&filesys_lock); bool status = filesys_create (file, initial_size); @@ -183,7 +186,7 @@ syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) static bool syscall_remove (const char *file) { - validate_user_pointer (file, 1, false); + validate_user_string (file, false); lock_acquire (&filesys_lock); bool status = filesys_remove (file); @@ -199,7 +202,7 @@ syscall_remove (const char *file) static int syscall_open (const char *file) { - validate_user_pointer (file, 1, false); + validate_user_string (file, false); lock_acquire (&filesys_lock); struct file *ptr = filesys_open (file); @@ -405,26 +408,49 @@ fd_get_file (int fd) } /* Validates if a block of memory starting at PTR and of size SIZE bytes is - fully contained within user virtual memory. Returns NULL if the memory - is invalid. Otherwise, returns the PTR given. - If the size is 0, the function does no checks and returns PTR.*/ -static void * + fully contained within valid user virtual memory. thread_exit () if the + memory is invalid. + If the size is 0, the function does no checks and returns PTR. */ +static void validate_user_pointer (const void *ptr, size_t size, bool check_write) { if (size == 0) - return ptr; + return; /* ptr < ptr + size - 1, so sufficient to check that (ptr + size -1) is a valid user virtual memory address. */ - if (!is_user_vaddr (ptr + size - 1)) + void *last = ptr + size - 1; + if (!is_user_vaddr (last)) thread_exit (); - /* Check read access to pointer. */ - int result; - if ((result = get_user (ptr)) == -1) - thread_exit (); - /* Check write access to pointer (if required). */ - if (check_write && !put_user (ptr, result)) - thread_exit (); - return ptr; + for (; ptr <= last; ptr++) + { + int result; + /* Check read access to pointer. */ + if ((result = get_user (ptr)) == -1) + thread_exit (); + /* Check write access to pointer (if required). */ + if (check_write && !put_user (ptr, result)) + thread_exit (); + } +} + +/* Validates of a C-string starting at ptr is fully contained within valid + user virtual memory. thread_exit () if the memory is invalid. */ +static void +validate_user_string (const char *ptr, bool check_write) +{ + while (true) + { + if (!is_user_vaddr (ptr)) + thread_exit (); + int result; + if ((result = get_user ((const uint8_t *)ptr)) == -1) + thread_exit (); + if (check_write && !put_user ((uint8_t *)ptr, result)) + thread_exit (); + if (*ptr == '\0') + return; + ptr++; + } } /* PROVIDED BY SPEC. -- 2.49.1 From 59e7a64f8e1e538c010cf6c31974c4bf9d5dcbe1 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Tue, 12 Nov 2024 15:48:22 +0000 Subject: [PATCH 05/14] Only check user pages rather than all bytes in-between, for known-size pointers --- src/userprog/syscall.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 2b0a551..1be6c77 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -421,7 +421,8 @@ validate_user_pointer (const void *ptr, size_t size, bool check_write) void *last = ptr + size - 1; if (!is_user_vaddr (last)) thread_exit (); - for (; ptr <= last; ptr++) + ptr = pg_round_down (ptr); + while (ptr <= last) { int result; /* Check read access to pointer. */ @@ -430,6 +431,7 @@ validate_user_pointer (const void *ptr, size_t size, bool check_write) /* Check write access to pointer (if required). */ if (check_write && !put_user (ptr, result)) thread_exit (); + ptr += PGSIZE; } } -- 2.49.1 From 3ef5264b6e3858b6e89311795a7f5e05ec02cb6a Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Tue, 26 Nov 2024 04:43:25 +0000 Subject: [PATCH 06/14] feat: allow stack to grow for process up to 8MB in size --- src/Makefile.build | 1 + src/userprog/exception.c | 7 +++++++ src/vm/stackgrowth.c | 38 ++++++++++++++++++++++++++++++++++++++ src/vm/stackgrowth.h | 11 +++++++++++ 4 files changed, 57 insertions(+) create mode 100644 src/vm/stackgrowth.c create mode 100644 src/vm/stackgrowth.h diff --git a/src/Makefile.build b/src/Makefile.build index c0d535e..4167f37 100644 --- a/src/Makefile.build +++ b/src/Makefile.build @@ -63,6 +63,7 @@ userprog_SRC += userprog/tss.c # TSS management. # Virtual memory code. vm_SRC += devices/swap.c # Swap block manager. +vm_SRC += vm/stackgrowth.c #vm_SRC = vm/file.c # Some other file. # Filesystem code. diff --git a/src/userprog/exception.c b/src/userprog/exception.c index 0a20b53..500cb89 100644 --- a/src/userprog/exception.c +++ b/src/userprog/exception.c @@ -4,6 +4,7 @@ #include "userprog/gdt.h" #include "threads/interrupt.h" #include "threads/thread.h" +#include "vm/stackgrowth.h" /* Number of page faults processed. */ static long long page_fault_cnt; @@ -145,6 +146,12 @@ page_fault (struct intr_frame *f) write = (f->error_code & PF_W) != 0; user = (f->error_code & PF_U) != 0; + if (user && needs_new_page (fault_addr, f->esp)) + { + if (grow_stack (fault_addr)) + return; + } + /* To implement virtual memory, delete the rest of the function body, and replace it with code that brings in the page to which fault_addr refers. */ diff --git a/src/vm/stackgrowth.c b/src/vm/stackgrowth.c new file mode 100644 index 0000000..164eb9d --- /dev/null +++ b/src/vm/stackgrowth.c @@ -0,0 +1,38 @@ +#include +#include "stackgrowth.h" +#include "threads/palloc.h" +#include "threads/thread.h" +#include "threads/vaddr.h" +#include "userprog/pagedir.h" + +/* Validates a given address for being <=32 bytes away from the stack pointer or + above the stack */ +bool needs_new_page (void *addr, void *esp) +{ + return (is_user_vaddr (addr) && + (uint32_t*)addr >= ((uint32_t*)esp - 32) && + ((PHYS_BASE - pg_round_down (addr)) + <= MAX_STACK_SIZE)); +} + +/* Extends the stack by the necessary number of pages */ +bool grow_stack (void *addr) +{ + struct thread *t = thread_current (); + void *last_page = pg_round_down (addr); + + uint8_t *new_page = palloc_get_page (PAL_USER | PAL_ZERO); + if ( new_page == NULL) + return false; + + bool added_page = pagedir_get_page (t->pagedir, last_page) == NULL + && pagedir_set_page (t->pagedir, last_page, new_page, true); + + if (!added_page) { + palloc_free_page (new_page); + return false; + } + return true; + + +} \ No newline at end of file diff --git a/src/vm/stackgrowth.h b/src/vm/stackgrowth.h new file mode 100644 index 0000000..0502210 --- /dev/null +++ b/src/vm/stackgrowth.h @@ -0,0 +1,11 @@ +#ifndef GROWSTACK_H +#define GROWSTACK_H + +#include + +#define MAX_STACK_SIZE 8388608 // (8MB) + +bool needs_new_page (void *addr, void *esp); +bool grow_stack (void *addr); + +#endif //GROWSTACK_H -- 2.49.1 From af7f2ba873e482ea210ca737e5b79da204f1d4ab Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Tue, 26 Nov 2024 04:54:00 +0000 Subject: [PATCH 07/14] Fix: Magic number in stackgrowth.c --- src/vm/stackgrowth.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/vm/stackgrowth.c b/src/vm/stackgrowth.c index 164eb9d..7d5470d 100644 --- a/src/vm/stackgrowth.c +++ b/src/vm/stackgrowth.c @@ -5,14 +5,16 @@ #include "threads/vaddr.h" #include "userprog/pagedir.h" +#define MAX_STACK_ACCESS_DIST 32 + /* Validates a given address for being <=32 bytes away from the stack pointer or above the stack */ bool needs_new_page (void *addr, void *esp) { return (is_user_vaddr (addr) && - (uint32_t*)addr >= ((uint32_t*)esp - 32) && - ((PHYS_BASE - pg_round_down (addr)) - <= MAX_STACK_SIZE)); + (uint32_t*)addr >= ((uint32_t*)esp - MAX_STACK_ACCESS_DIST) && + ((PHYS_BASE - pg_round_down (addr)) + <= MAX_STACK_SIZE)); } /* Extends the stack by the necessary number of pages */ -- 2.49.1 From c670c29e47c009c4ccb0c9bb02bba06bb300f878 Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Wed, 27 Nov 2024 18:57:20 +0000 Subject: [PATCH 08/14] update stack growth header to fit virtual memory naming format --- src/vm/stackgrowth.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vm/stackgrowth.h b/src/vm/stackgrowth.h index 0502210..a23e481 100644 --- a/src/vm/stackgrowth.h +++ b/src/vm/stackgrowth.h @@ -1,5 +1,5 @@ -#ifndef GROWSTACK_H -#define GROWSTACK_H +#ifndef VM_GROWSTACK_H +#define VM_GROWSTACK_H #include @@ -8,4 +8,4 @@ bool needs_new_page (void *addr, void *esp); bool grow_stack (void *addr); -#endif //GROWSTACK_H +#endif /* vm/frame.h */ -- 2.49.1 From c74a8c55aae4d4607670b9d0f192a3cc891acd95 Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Wed, 27 Nov 2024 19:21:43 +0000 Subject: [PATCH 09/14] Implement stack growth for system calls and add stack pointer tracking to thread --- src/threads/thread.h | 4 ++++ src/userprog/syscall.c | 30 +++++++++++++++++++++++++++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/threads/thread.h b/src/threads/thread.h index 4a88577..60f91ce 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -143,6 +143,10 @@ struct thread struct hash open_files; /* Hash Table of FD -> Struct File. */ #endif +#ifdef VM + void *curr_esp; +#endif + /* Owned by thread.c. */ unsigned magic; /* Detects stack overflow. */ }; diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 3efe7b5..7fbc939 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -10,6 +10,9 @@ #include "threads/synch.h" #include "userprog/process.h" #include "userprog/pagedir.h" +#ifdef VM +#include "vm/stackgrowth.h" +#endif #include #include @@ -98,6 +101,7 @@ syscall_handler (struct intr_frame *f) /* First, read the system call number from the stack. */ validate_user_pointer (f->esp, sizeof (uintptr_t)); uintptr_t syscall_number = *(int *) f->esp; + thread_current ()->curr_esp = f->esp; /* Ensures the number corresponds to a system call that can be handled. */ if (syscall_number >= LOOKUP_SIZE) @@ -451,6 +455,20 @@ fd_get_file (int fd) return hash_entry (e, struct open_file, elem); } +static bool +try_alloc_new_page (const void *ptr) +{ + if (needs_new_page (ptr, thread_current()->curr_esp)) + { + if (!grow_stack (ptr)) + return 0; + else + return 1; + } + else + return 0; +} + /* Validates if a block of memory starting at START and of size SIZE bytes is fully contained within user virtual memory. Kills the thread (by exiting with failure) if the memory is invalid. Otherwise, returns (nothing) normally. @@ -472,7 +490,10 @@ validate_user_pointer (const void *start, size_t size) memory by the page table. */ for (const void *ptr = pg_round_down (start); ptr <= end; ptr += PGSIZE) if (pagedir_get_page (thread_current ()->pagedir, ptr) == NULL) - syscall_exit (EXIT_FAILURE); + { + if (!try_alloc_new_page (ptr)) + syscall_exit (EXIT_FAILURE); + } } /* Validates if a string is fully contained within user virtual memory. Kills @@ -495,10 +516,13 @@ validate_user_string (const char *str) /* If we reach addresses that are not mapped to physical memory before the end of the string, the thread is terminated. */ - if (!is_user_vaddr(page) || - pagedir_get_page (thread_current ()->pagedir, page) == NULL) + if (!is_user_vaddr(page)) syscall_exit (EXIT_FAILURE); + if (pagedir_get_page (thread_current ()->pagedir, page) == NULL) + if (!try_alloc_new_page (str)) + syscall_exit (EXIT_FAILURE); + while (offset < PGSIZE) { if (*str == '\0') -- 2.49.1 From 4f84a83611aad4bde85641d83c5a7d00f2ec051e Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Wed, 27 Nov 2024 19:41:22 +0000 Subject: [PATCH 10/14] Refactor: abstract new page allocation to one general function and make helper functions static --- src/userprog/exception.c | 4 ++-- src/userprog/syscall.c | 25 +++++++------------------ src/vm/stackgrowth.c | 30 ++++++++++++++++++++++++------ src/vm/stackgrowth.h | 3 +-- 4 files changed, 34 insertions(+), 28 deletions(-) diff --git a/src/userprog/exception.c b/src/userprog/exception.c index 500cb89..272325e 100644 --- a/src/userprog/exception.c +++ b/src/userprog/exception.c @@ -146,9 +146,9 @@ page_fault (struct intr_frame *f) write = (f->error_code & PF_W) != 0; user = (f->error_code & PF_U) != 0; - if (user && needs_new_page (fault_addr, f->esp)) + if (user) { - if (grow_stack (fault_addr)) + if (try_alloc_new_page (fault_addr, f->esp)) return; } diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 7fbc939..26a37e7 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -455,20 +455,6 @@ fd_get_file (int fd) return hash_entry (e, struct open_file, elem); } -static bool -try_alloc_new_page (const void *ptr) -{ - if (needs_new_page (ptr, thread_current()->curr_esp)) - { - if (!grow_stack (ptr)) - return 0; - else - return 1; - } - else - return 0; -} - /* Validates if a block of memory starting at START and of size SIZE bytes is fully contained within user virtual memory. Kills the thread (by exiting with failure) if the memory is invalid. Otherwise, returns (nothing) normally. @@ -480,6 +466,8 @@ validate_user_pointer (const void *start, size_t size) if (size == 0) return; + struct thread *t = thread_current (); + const void *end = start + size - 1; /* Check if the start and end pointers are valid user virtual addresses. */ @@ -489,9 +477,9 @@ validate_user_pointer (const void *start, size_t size) /* We now need to check if the entire memory block is mapped to physical memory by the page table. */ for (const void *ptr = pg_round_down (start); ptr <= end; ptr += PGSIZE) - if (pagedir_get_page (thread_current ()->pagedir, ptr) == NULL) + if (pagedir_get_page (t->pagedir, ptr) == NULL) { - if (!try_alloc_new_page (ptr)) + if (!try_alloc_new_page (ptr, t->curr_esp)) syscall_exit (EXIT_FAILURE); } } @@ -508,6 +496,7 @@ validate_user_string (const char *str) /* Calculate the offset of the string within the (first) page. */ size_t offset = (uintptr_t) str % PGSIZE; + struct thread *t = thread_current (); /* We move page by page, checking if the page is mapped to physical memory. */ for (;;) @@ -519,8 +508,8 @@ validate_user_string (const char *str) if (!is_user_vaddr(page)) syscall_exit (EXIT_FAILURE); - if (pagedir_get_page (thread_current ()->pagedir, page) == NULL) - if (!try_alloc_new_page (str)) + if (pagedir_get_page (t->pagedir, page) == NULL) + if (!try_alloc_new_page (str, t->curr_esp)) syscall_exit (EXIT_FAILURE); while (offset < PGSIZE) diff --git a/src/vm/stackgrowth.c b/src/vm/stackgrowth.c index 7d5470d..50bdc39 100644 --- a/src/vm/stackgrowth.c +++ b/src/vm/stackgrowth.c @@ -7,9 +7,28 @@ #define MAX_STACK_ACCESS_DIST 32 -/* Validates a given address for being <=32 bytes away from the stack pointer or - above the stack */ -bool needs_new_page (void *addr, void *esp) +static bool needs_new_page (const void *addr, const void *esp); +static bool grow_stack (const void *addr); + +bool +try_alloc_new_page (const void *ptr, const void *esp) +{ + if (needs_new_page (ptr, esp)) + { + if (!grow_stack (ptr)) + return 0; + else + return 1; + } + else + return 0; +} + +/* Validates a given address for being a stack query and not a generic erroneous + address + */ +static bool +needs_new_page (const void *addr, const void *esp) { return (is_user_vaddr (addr) && (uint32_t*)addr >= ((uint32_t*)esp - MAX_STACK_ACCESS_DIST) && @@ -18,7 +37,8 @@ bool needs_new_page (void *addr, void *esp) } /* Extends the stack by the necessary number of pages */ -bool grow_stack (void *addr) +static bool +grow_stack (const void *addr) { struct thread *t = thread_current (); void *last_page = pg_round_down (addr); @@ -35,6 +55,4 @@ bool grow_stack (void *addr) return false; } return true; - - } \ No newline at end of file diff --git a/src/vm/stackgrowth.h b/src/vm/stackgrowth.h index a23e481..acd123e 100644 --- a/src/vm/stackgrowth.h +++ b/src/vm/stackgrowth.h @@ -5,7 +5,6 @@ #define MAX_STACK_SIZE 8388608 // (8MB) -bool needs_new_page (void *addr, void *esp); -bool grow_stack (void *addr); +bool try_alloc_new_page (const void *ptr, const void *esp); #endif /* vm/frame.h */ -- 2.49.1 From 5c661c2e24bc83c1229516ea193b3c87237c2aa5 Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Fri, 29 Nov 2024 23:49:49 +0000 Subject: [PATCH 11/14] Feat: pointer validation checks string across multiple pages and handle kernel page faults --- src/threads/thread.h | 2 -- src/userprog/Make.vars | 4 ++-- src/userprog/exception.c | 20 ++++++++++---------- src/userprog/syscall.c | 38 +++++++++++++++++++++++++------------- 4 files changed, 37 insertions(+), 27 deletions(-) diff --git a/src/threads/thread.h b/src/threads/thread.h index 60f91ce..f031981 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -143,9 +143,7 @@ struct thread struct hash open_files; /* Hash Table of FD -> Struct File. */ #endif -#ifdef VM void *curr_esp; -#endif /* Owned by thread.c. */ unsigned magic; /* Detects stack overflow. */ diff --git a/src/userprog/Make.vars b/src/userprog/Make.vars index e4dbb08..23bae3d 100644 --- a/src/userprog/Make.vars +++ b/src/userprog/Make.vars @@ -1,7 +1,7 @@ # -*- makefile -*- -kernel.bin: DEFINES = -DUSERPROG -DFILESYS -KERNEL_SUBDIRS = threads devices lib lib/kernel userprog filesys +kernel.bin: DEFINES = -DUSERPROG -DFILESYS -DVM +KERNEL_SUBDIRS = threads devices lib lib/kernel userprog filesys vm TEST_SUBDIRS = tests/userprog tests/userprog/no-vm tests/filesys/base GRADING_FILE = $(SRCDIR)/tests/userprog/Grading SIMULATOR = --qemu diff --git a/src/userprog/exception.c b/src/userprog/exception.c index 3e3b133..1fcbe61 100644 --- a/src/userprog/exception.c +++ b/src/userprog/exception.c @@ -146,19 +146,19 @@ page_fault (struct intr_frame *f) write = (f->error_code & PF_W) != 0; user = (f->error_code & PF_U) != 0; - /* Kernel page fault is further handled by the kernel itself. */ - if (kernel) + if (user && not_present) { - f->eip = (void *)f->eax; + if (try_alloc_new_page (fault_addr, f->esp)) + return; + } + else + { + if (try_alloc_new_page (fault_addr, thread_current ()->curr_esp)) + return; + f->eip = (void *)f->eax; f->eax = 0xffffffff; return; - } - - if (user) - { - if (try_alloc_new_page (fault_addr, f->esp)) - return; - } + } /* To implement virtual memory, delete the rest of the function body, and replace it with code that brings in the page to diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index da6d77d..4bc34ca 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -10,9 +10,6 @@ #include "threads/synch.h" #include "userprog/process.h" #include "userprog/pagedir.h" -#ifdef VM -#include "vm/stackgrowth.h" -#endif #include #include #include @@ -465,17 +462,17 @@ validate_user_pointer (const void *ptr, size_t size, bool check_write) valid user virtual memory address. */ void *last = ptr + size - 1; if (!is_user_vaddr (last)) - thread_exit (); + syscall_exit (EXIT_FAILURE); ptr = pg_round_down (ptr); while (ptr <= last) { int result; /* Check read access to pointer. */ if ((result = get_user (ptr)) == -1) - thread_exit (); + syscall_exit (EXIT_FAILURE); /* Check write access to pointer (if required). */ if (check_write && !put_user (ptr, result)) - thread_exit (); + syscall_exit (EXIT_FAILURE); ptr += PGSIZE; } } @@ -485,18 +482,33 @@ validate_user_pointer (const void *ptr, size_t size, bool check_write) static void validate_user_string (const char *ptr, bool check_write) { - while (true) + size_t offset = (uintptr_t) ptr % PGSIZE; + + for (;;) { + void *page = pg_round_down (ptr); + + if (!is_user_vaddr (page)) + syscall_exit (EXIT_FAILURE); if (!is_user_vaddr (ptr)) - thread_exit (); + syscall_exit (EXIT_FAILURE); int result; if ((result = get_user ((const uint8_t *)ptr)) == -1) - thread_exit (); + syscall_exit (EXIT_FAILURE); if (check_write && !put_user ((uint8_t *)ptr, result)) - thread_exit (); - if (*ptr == '\0') - return; - ptr++; + syscall_exit (EXIT_FAILURE); + + while (offset < PGSIZE) + { + if (*ptr == '\0') + return; /* We reached the end of the string without issues. */ + + ptr++; + offset++; + } + + offset = 0; + } } -- 2.49.1 From 13de832586e55a4899e0aa1b09abb97ddd98a571 Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Fri, 29 Nov 2024 23:52:05 +0000 Subject: [PATCH 12/14] Refactor stack growth code to remove messy conditions --- src/vm/stackgrowth.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/vm/stackgrowth.c b/src/vm/stackgrowth.c index 50bdc39..bc9717c 100644 --- a/src/vm/stackgrowth.c +++ b/src/vm/stackgrowth.c @@ -13,15 +13,7 @@ static bool grow_stack (const void *addr); bool try_alloc_new_page (const void *ptr, const void *esp) { - if (needs_new_page (ptr, esp)) - { - if (!grow_stack (ptr)) - return 0; - else - return 1; - } - else - return 0; + return needs_new_page (ptr, esp) && grow_stack (ptr); } /* Validates a given address for being a stack query and not a generic erroneous -- 2.49.1 From 6f85d7642d8d023de8b03dc4efed32c5df7d4340 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Sat, 30 Nov 2024 22:40:13 +0000 Subject: [PATCH 13/14] feat: implement clock (second-chance) page eviction algorithm --- src/Makefile.build | 2 +- src/userprog/process.c | 35 +++++--- src/vm/frame.c | 191 +++++++++++++++++++++++++++++++++-------- src/vm/frame.h | 3 +- src/vm/page.c | 20 +++++ src/vm/page.h | 9 ++ 6 files changed, 209 insertions(+), 51 deletions(-) create mode 100644 src/vm/page.c create mode 100644 src/vm/page.h diff --git a/src/Makefile.build b/src/Makefile.build index 4e57a13..7778f57 100644 --- a/src/Makefile.build +++ b/src/Makefile.build @@ -63,8 +63,8 @@ userprog_SRC += userprog/tss.c # TSS management. # Virtual memory code. vm_SRC += vm/frame.c # Frame table manager. +vm_SRC += vm/page.c # Page table manager. vm_SRC += devices/swap.c # Swap block manager. -#vm_SRC = vm/file.c # Some other file. # Filesystem code. filesys_SRC = filesys/filesys.c # Filesystem core. diff --git a/src/userprog/process.c b/src/userprog/process.c index a8f1d10..aa5091c 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -116,7 +116,7 @@ process_execute (const char *cmd) return tid; } -static void *get_usr_kpage (enum palloc_flags flags); +static void *get_usr_kpage (enum palloc_flags flags, void *upage); static void free_usr_kpage (void *kpage); static bool install_page (void *upage, void *kpage, bool writable); @@ -257,12 +257,13 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name) int pages_needed = DIV_CEIL (overflow_bytes, PGSIZE); /* Allocate the pages and map them to the user process. */ + void *upage; + uint8_t *kpage; for (int i = 1; i < pages_needed + 1; i++) { - uint8_t *kpage = get_usr_kpage (PAL_ZERO); - if (!install_page (((uint8_t *) PHYS_BASE) - PGSIZE * (i + 1), - kpage, true)) - return false; + upage = ((uint8_t *) PHYS_BASE) - PGSIZE * (i + 1); + kpage = get_usr_kpage (PAL_ZERO, upage); + if (!install_page (upage, kpage, true)) return false; } } @@ -710,7 +711,7 @@ load_segment (struct file *file, off_t ofs, uint8_t *upage, if (kpage == NULL){ /* Get a new page of memory. */ - kpage = get_usr_kpage (0); + kpage = get_usr_kpage (0, upage); if (kpage == NULL){ return false; } @@ -752,11 +753,13 @@ setup_stack (void **esp) { uint8_t *kpage; bool success = false; - - kpage = get_usr_kpage (PAL_ZERO); + + void *upage = ((uint8_t *) PHYS_BASE) - PGSIZE; + + kpage = get_usr_kpage (PAL_ZERO, upage); if (kpage != NULL) { - success = install_page (((uint8_t *) PHYS_BASE) - PGSIZE, kpage, true); + success = install_page (upage, kpage, true); if (success) *esp = PHYS_BASE; else @@ -765,14 +768,20 @@ setup_stack (void **esp) return success; } -/* Claims a page from the user pool and returns its kernel address, - updating the frame table if VM is enabled. */ +/* Claims a page from the user pool for ownership by the current thread + and returns its kernel address, updating the frame table if VM + is enabled. Requires the intended virtual address for where the page + will be installed. */ static void * -get_usr_kpage (enum palloc_flags flags) +get_usr_kpage (enum palloc_flags flags, void *upage) { void *page; #ifdef VM - page = frame_alloc (flags); + struct thread *t = thread_current (); + if (pagedir_get_page (t->pagedir, upage) != NULL) + return NULL; + else + page = frame_alloc (flags, upage, t); #else page = palloc_get_page (flags | PAL_USER); #endif diff --git a/src/vm/frame.c b/src/vm/frame.c index b030c59..cdb141c 100644 --- a/src/vm/frame.c +++ b/src/vm/frame.c @@ -1,34 +1,42 @@ #include #include #include +#include #include "frame.h" +#include "page.h" #include "threads/malloc.h" +#include "threads/vaddr.h" +#include "userprog/pagedir.h" #include "threads/synch.h" +#include "devices/swap.h" /* Hash table that maps every active frame's kernel virtual address to its corresponding 'frame_metadata'.*/ struct hash frame_table; -/* Linked list of frame_metadata whose pages are predicted to currently - be in the working set of a process. They are not considered for - eviction, but are considered for demotion to the 'inactive' list. */ -struct list active_list; +/* Linked list used to represent the circular queue in the 'clock' + algorithm for page eviction. Iterating from the element that is + currently pointed at by 'next_victim' yields an ordering of the entries + from oldest to newest (in terms of when they were added or checked + for having been referenced by a process). */ +struct list lru_list; -/* Linked list of frame_metadata whose pages are predicted to leave the - working set of their processes soon, so are considered for eviction. - Pages are considered for eviction from the tail end, and are initially - demoted to 'inactive' at the head. */ -struct list inactive_list; +/* The next element in lru_list to be considered for eviction (oldest added + or referenced page in the circular queue). If this page has has an + 'accessed' bit of 0 when considering eviction, then it will be the next + victim. Otherwise, the next element in the queue is similarly considered. */ +struct list_elem *next_victim = NULL; /* Synchronisation variables. */ -/* Ensures mutual exclusion to accessing the 'head' and first element of - 'inactive_list', which is accessed every time a frame is allocated. */ -struct lock inactive_head_lock; +/* Protects access to 'lru_list'. */ +struct lock lru_lock; struct frame_metadata { void *frame; /* The kernel virtual address holding the frame. */ + void *upage; /* The user virtual address pointing to the frame. */ + struct thread *owner; /* Pointer to the thread that owns the frame. */ struct hash_elem hash_elem; /* Tracks the position of the frame metadata within 'frame_table', whose key is the kernel virtual address of the frame. */ @@ -40,56 +48,102 @@ struct frame_metadata hash_hash_func frame_metadata_hash; hash_less_func frame_metadata_less; +static struct list_elem *lru_next (struct list_elem *e); +static struct list_elem *lru_prev (struct list_elem *e); +static struct frame_metadata *get_victim (void); + /* Initialize the frame system by initializing the frame (hash) table with the frame_metadata hashing and comparison functions, as well as initializing - the active & inactive lists. Also initializes the system's synchronisation - primitives. */ + 'lru_list' and its associated synchronisation primitives. */ void frame_init (void) { hash_init (&frame_table, frame_metadata_hash, frame_metadata_less, NULL); - list_init (&active_list); - list_init (&inactive_list); - lock_init (&inactive_head_lock); + list_init (&lru_list); + lock_init (&lru_lock); } +/* TODO: Consider synchronisation more closely (i.e. just for hash + table). */ /* Attempt to allocate a frame for a user process, either by direct allocation of a user page if there is sufficient RAM, or by evicting a currently active page if memory allocated for user processes is fulled and storing it in swap. If swap is full in the former case, panic the kernel. */ void * -frame_alloc (enum palloc_flags flags) +frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner) { + struct frame_metadata *frame_metadata; flags |= PAL_USER; - + + lock_acquire (&lru_lock); void *frame = palloc_get_page (flags); + + /* If a frame couldn't be allocated we must be out of main memory. Thus, + obtain a victim page to replace with our page, and swap the victim + into disk. */ if (frame == NULL) { - /* TODO: Find victim page to replace, and swap it with this new page. */ - return NULL; + /* 1. Obtain victim. */ + if (next_victim == NULL) + PANIC ("Couldn't allocate a single page to main memory!\n"); + + struct frame_metadata *victim = get_victim (); + ASSERT (victim != NULL); /* get_victim () should never return null. */ + + /* 2. Swap out victim into disk. */ + size_t swap_slot = swap_out (victim->frame); + page_set_swap (victim->owner, victim->upage, swap_slot); + + /* If zero flag is set, zero out the victim page. */ + if (flags & PAL_ZERO) + memset (victim->frame, 0, PGSIZE); + + /* 3. Indicate that the new frame's metadata will be stored + inside the same structure that stored the victim's metadata. + As both the new frame and the victim frame share the same kernel + virtual address, the hash map need not be updated, and neither + the list_elem value as both share the same lru_list position. */ + frame_metadata = victim; } - struct frame_metadata *frame_metadata = - malloc (sizeof (struct frame_metadata)); - frame_metadata->frame = frame; + /* If sufficient main memory allows the frame to be directly allocated, + we must update the frame table with a new entry, and grow lru_list. */ + else + { + /* Must own lru_lock here, as otherwise there is a race condition + with next_victim either being NULL or uninitialized. */ + frame_metadata = malloc (sizeof (struct frame_metadata)); + frame_metadata->frame = frame; - /* Newly faulted pages begin at the head of the inactive list. */ - lock_acquire (&inactive_head_lock); - list_push_front (&inactive_list, &frame_metadata->list_elem); - lock_release (&inactive_head_lock); + /* Newly allocated frames are pushed to the back of the circular queue + represented by lru_list. Must explicitly handle the case where the + circular queue is empty (when next_victim == NULL). */ + if (next_victim == NULL) + { + list_push_back (&lru_list, &frame_metadata->list_elem); + next_victim = &frame_metadata->list_elem; + } + else + { + struct list_elem *lru_tail = lru_prev (next_victim); + list_insert (lru_tail, &frame_metadata->list_elem); + } - /* Finally, insert frame metadata within the frame table, with the key as its - allocated kernel address. */ - hash_replace (&frame_table, &frame_metadata->hash_elem); + hash_insert (&frame_table, &frame_metadata->hash_elem); + } - return frame; + frame_metadata->upage = upage; + frame_metadata->owner = owner; + lock_release (&lru_lock); + + return frame_metadata->frame; } /* Attempt to deallocate a frame for a user process by removing it from the - frame table as well as active/inactive list, and freeing the underlying - page memory. Panics if the frame isn't active in memory. */ + frame table as well as lru_list, and freeing the underlying page + memory & metadata struct. Panics if the frame isn't active in memory. */ void frame_free (void *frame) { @@ -98,17 +152,58 @@ frame_free (void *frame) struct hash_elem *e = hash_delete (&frame_table, &key_metadata.hash_elem); - if (e == NULL) PANIC ("Attempted to free a frame without a corresponding " - "kernel address!\n"); + if (e == NULL) PANIC ("Attempted to free a frame at kernel address %p, " + "but this address is not allocated!\n", frame); struct frame_metadata *frame_metadata = hash_entry (e, struct frame_metadata, hash_elem); + lock_acquire (&lru_lock); list_remove (&frame_metadata->list_elem); + + /* If we're freeing the frame marked as the next victim, update + next_victim to either be the next least recently used page, or NULL + if no pages are loaded in main memory. */ + if (&frame_metadata->list_elem == next_victim) + { + if (list_empty (&lru_list)) + next_victim = NULL; + else + next_victim = lru_next (next_victim); + } + lock_release (&lru_lock); + free (frame_metadata); palloc_free_page (frame); } +/* TODO: Account for page aliases when checking accessed bit. */ +/* A pre-condition for calling this function is that the calling thread + owns lru_lock and that lru_list is non-empty. */ +static struct frame_metadata * +get_victim (void) + { + struct list_elem *e = next_victim; + struct frame_metadata *frame_metadata; + uint32_t *pd; + void *upage; + for (;;) + { + frame_metadata = list_entry (e, struct frame_metadata, list_elem); + pd = frame_metadata->owner->pagedir; + upage = frame_metadata->upage; + e = lru_next (e); + + if (!pagedir_is_accessed (pd, upage)) + break; + + pagedir_set_accessed (pd, upage, false); + } + + next_victim = e; + return frame_metadata; + } + /* Hash function for frame metadata, used for storing entries in the frame table. */ unsigned @@ -135,3 +230,27 @@ frame_metadata_less (const struct hash_elem *a_, const struct hash_elem *b_, return a->frame < b->frame; } +/* Returns the next recently used element after the one provided, which + is achieved by iterating through lru_list like a circular queue + (wrapping around the list at the tail). */ +static struct list_elem * +lru_next (struct list_elem *e) +{ + if (!list_empty (&lru_list) && e == list_back (&lru_list)) + return list_front (&lru_list); + + return list_next (e); +} + +/* Returns the previous recently used element after the one provided, which + is achieved by iterating through lru_list like a circular queue + (wrapping around the list at the head). */ +static struct list_elem * +lru_prev (struct list_elem *e) +{ + if (!list_empty (&lru_list) && e == list_front (&lru_list)) + return list_back (&lru_list); + + return list_prev (e); +} + diff --git a/src/vm/frame.h b/src/vm/frame.h index 8e52ec2..93081d3 100644 --- a/src/vm/frame.h +++ b/src/vm/frame.h @@ -1,10 +1,11 @@ #ifndef VM_FRAME_H #define VM_FRAME_H +#include "threads/thread.h" #include "threads/palloc.h" void frame_init (void); -void *frame_alloc (enum palloc_flags); +void *frame_alloc (enum palloc_flags, void *, struct thread *); void frame_free (void *frame); #endif /* vm/frame.h */ diff --git a/src/vm/page.c b/src/vm/page.c new file mode 100644 index 0000000..8ebc71c --- /dev/null +++ b/src/vm/page.c @@ -0,0 +1,20 @@ +#include "page.h" + +/* Updates the 'owner' thread's page table entry for virtual address 'upage' + to have a present bit of 0 and stores the specified swap slot value in the + entry for later retrieval from disk. */ +void +page_set_swap (struct thread *owner, void *upage, size_t swap_slot) +{ + +} + +/* Given that the page with user address 'upage' owned by 'owner' is flagged + to be in the swap disk via the owner's page table, returns its stored + swap slot. Otherwise panics the kernel. */ +size_t +page_get_swap (struct thread *owner, void *upage) +{ + +} + diff --git a/src/vm/page.h b/src/vm/page.h new file mode 100644 index 0000000..7259ca9 --- /dev/null +++ b/src/vm/page.h @@ -0,0 +1,9 @@ +#ifndef VM_PAGE_H +#define VM_PAGE_H + +#include "threads/thread.h" + +void page_set_swap (struct thread *, void *, size_t); +size_t page_get_swap (struct thread *, void *); + +#endif /* vm/frame.h */ -- 2.49.1 From 5265fed28855f4e7e56789ac8c196511eaeaa38b Mon Sep 17 00:00:00 2001 From: sBubshait Date: Thu, 5 Dec 2024 00:27:40 +0000 Subject: [PATCH 14/14] Refactor stack growth to be helper functions in exception for easier merging --- src/Makefile.build | 1 - src/userprog/exception.c | 87 ++++++++++++++++++++++++++++++++++------ src/userprog/process.c | 4 +- src/userprog/process.h | 2 + src/vm/stackgrowth.c | 50 ----------------------- src/vm/stackgrowth.h | 10 ----- 6 files changed, 79 insertions(+), 75 deletions(-) delete mode 100644 src/vm/stackgrowth.c delete mode 100644 src/vm/stackgrowth.h diff --git a/src/Makefile.build b/src/Makefile.build index 4398839..7240189 100644 --- a/src/Makefile.build +++ b/src/Makefile.build @@ -65,7 +65,6 @@ userprog_SRC += userprog/tss.c # TSS management. vm_SRC += vm/frame.c # Frame table manager. vm_SRC += vm/page.c # Page table manager. vm_SRC += devices/swap.c # Swap block manager. -vm_SRC += vm/stackgrowth.c #vm_SRC = vm/file.c # Some other file. # Filesystem code. diff --git a/src/userprog/exception.c b/src/userprog/exception.c index 1fcbe61..7acfb75 100644 --- a/src/userprog/exception.c +++ b/src/userprog/exception.c @@ -2,9 +2,15 @@ #include #include #include "userprog/gdt.h" +#include "userprog/pagedir.h" +#include "userprog/process.h" #include "threads/interrupt.h" +#include "threads/palloc.h" #include "threads/thread.h" -#include "vm/stackgrowth.h" +#include "threads/vaddr.h" + +#define MAX_STACK_SIZE (8 * 1024 * 1024) // 8MB +#define MAX_STACK_OFFSET 32 // 32 bytes offset below stack pointer (ESP) /* Number of page faults processed. */ static long long page_fault_cnt; @@ -12,6 +18,9 @@ static long long page_fault_cnt; static void kill (struct intr_frame *); static void page_fault (struct intr_frame *); +static bool is_valid_stack_access (const void *fault_addr, const void *esp); +static bool grow_stack (void *upage); + /* Registers handlers for interrupts that can be caused by user programs. @@ -146,19 +155,26 @@ page_fault (struct intr_frame *f) write = (f->error_code & PF_W) != 0; user = (f->error_code & PF_U) != 0; - if (user && not_present) + if (!user || !not_present) + { + f->eip = (void *)f->eax; + f->eax = 0xffffffff; + return; + } + + /* If the fault address is in a user page that is not present, then it might + be just that the stack needs to grow. So we attempt to grow the stack. */ + void *upage = pg_round_down (fault_addr); + if (not_present && is_user_vaddr (upage) && upage != NULL) { - if (try_alloc_new_page (fault_addr, f->esp)) - return; + if (is_valid_stack_access (fault_addr, f->esp)) + { + if (grow_stack (upage)) + return; + } + + /* TODO: Check SPT for the page. */ } - else - { - if (try_alloc_new_page (fault_addr, thread_current ()->curr_esp)) - return; - f->eip = (void *)f->eax; - f->eax = 0xffffffff; - return; - } /* To implement virtual memory, delete the rest of the function body, and replace it with code that brings in the page to @@ -171,3 +187,50 @@ page_fault (struct intr_frame *f) kill (f); } +/* Validates whether the fault address is a valid stack access. Access is a + valid stack access under the following two conditions: + 1. The fault address must be within MAX_STACK_OFFSET (32) bytes below + the current stack pointer. (Accounts for both PUSH and PUSHA instructions) + 2. Growing this stack to this address does not cause it to exceed the + MAX_STACK_SIZE (8MB) limit. + + Returns true if both conditions are met, false otherwise. + + Pre: fault_addr is a valid user virtual address (so also not NULL). */ +static bool +is_valid_stack_access (const void *fault_addr, const void *esp) +{ + uint32_t new_stack_size = PHYS_BASE - pg_round_down (fault_addr); + + uint32_t *lowest_valid_push_addr = (uint32_t *)esp - MAX_STACK_OFFSET; + bool is_within_push_range = (uint32_t *)fault_addr >= lowest_valid_push_addr; + + return is_within_push_range && new_stack_size <= MAX_STACK_SIZE; +} + +/* Attempts to grow the stack by allocating and mapping a new page. + This involves: + 1. Allocating a zeroed page from the user pool + 2. Installing it into the page table with write permissions + + Returns true if the stack was successfully grown, false if either + allocation or installation fails. + + Pre: upage is a valid page-aligned address (so also not NULL). */ +static bool +grow_stack (void *upage) +{ + /* Allocate new page for stack */ + void *kpage = palloc_get_page (PAL_USER | PAL_ZERO); + if (kpage == NULL) + return false; + + /* Install the page into user page table */ + if (!install_page (upage, kpage, true)) + { + palloc_free_page (kpage); + return false; + } + + return true; +} \ No newline at end of file diff --git a/src/userprog/process.c b/src/userprog/process.c index aa5091c..b981743 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -118,7 +118,7 @@ process_execute (const char *cmd) static void *get_usr_kpage (enum palloc_flags flags, void *upage); static void free_usr_kpage (void *kpage); -static bool install_page (void *upage, void *kpage, bool writable); +bool install_page (void *upage, void *kpage, bool writable); static bool process_init_stack (char *cmd_saveptr, void **esp, char *file_name); static void *push_to_stack (void **esp, void *data, size_t data_size); @@ -809,7 +809,7 @@ free_usr_kpage (void *kpage) with palloc_get_page(). Returns true on success, false if UPAGE is already mapped or if memory allocation fails. */ -static bool +bool install_page (void *upage, void *kpage, bool writable) { struct thread *t = thread_current (); diff --git a/src/userprog/process.h b/src/userprog/process.h index 688cd2a..7cf3df4 100644 --- a/src/userprog/process.h +++ b/src/userprog/process.h @@ -8,4 +8,6 @@ int process_wait (tid_t); void process_exit (void); void process_activate (void); +bool install_page (void *upage, void *kpage, bool writable); + #endif /* userprog/process.h */ diff --git a/src/vm/stackgrowth.c b/src/vm/stackgrowth.c deleted file mode 100644 index bc9717c..0000000 --- a/src/vm/stackgrowth.c +++ /dev/null @@ -1,50 +0,0 @@ -#include -#include "stackgrowth.h" -#include "threads/palloc.h" -#include "threads/thread.h" -#include "threads/vaddr.h" -#include "userprog/pagedir.h" - -#define MAX_STACK_ACCESS_DIST 32 - -static bool needs_new_page (const void *addr, const void *esp); -static bool grow_stack (const void *addr); - -bool -try_alloc_new_page (const void *ptr, const void *esp) -{ - return needs_new_page (ptr, esp) && grow_stack (ptr); -} - -/* Validates a given address for being a stack query and not a generic erroneous - address - */ -static bool -needs_new_page (const void *addr, const void *esp) -{ - return (is_user_vaddr (addr) && - (uint32_t*)addr >= ((uint32_t*)esp - MAX_STACK_ACCESS_DIST) && - ((PHYS_BASE - pg_round_down (addr)) - <= MAX_STACK_SIZE)); -} - -/* Extends the stack by the necessary number of pages */ -static bool -grow_stack (const void *addr) -{ - struct thread *t = thread_current (); - void *last_page = pg_round_down (addr); - - uint8_t *new_page = palloc_get_page (PAL_USER | PAL_ZERO); - if ( new_page == NULL) - return false; - - bool added_page = pagedir_get_page (t->pagedir, last_page) == NULL - && pagedir_set_page (t->pagedir, last_page, new_page, true); - - if (!added_page) { - palloc_free_page (new_page); - return false; - } - return true; -} \ No newline at end of file diff --git a/src/vm/stackgrowth.h b/src/vm/stackgrowth.h deleted file mode 100644 index acd123e..0000000 --- a/src/vm/stackgrowth.h +++ /dev/null @@ -1,10 +0,0 @@ -#ifndef VM_GROWSTACK_H -#define VM_GROWSTACK_H - -#include - -#define MAX_STACK_SIZE 8388608 // (8MB) - -bool try_alloc_new_page (const void *ptr, const void *esp); - -#endif /* vm/frame.h */ -- 2.49.1