From 794567327f5fc00feb208bce5ad6caeddf11188d Mon Sep 17 00:00:00 2001 From: apio Date: Sat, 25 Feb 2023 17:09:03 +0100 Subject: [PATCH] kernel, luna: Port non-VFS changes over from pull request #22 OwnedPtr, SharedPtr: Add operator bool Option, Result: Make try_move_value() non-const since it modifies the Option kernel: Switch to a stack we control for the main task as soon as we leave early boot Heap: Fix GPF caused when making many small allocations Heap: Avoid accessing a block after it's potentially deleted luna: Skip UBSAN.cpp in CMakeLists as that's not implemented yet luna: Use spinlocks in the heap implementation kernel, luna: Move Spinlock.h to luna Option: Use __builtin_launder to ensure that the compiler doesn't label this as UB SharedPtr: Implement make_shared using adopt_shared SharedPtr: Delete ptr on failure in all adopt_shared* functions --- kernel/CMakeLists.txt | 1 - kernel/src/Log.cpp | 2 +- kernel/src/arch/CPU.h | 2 + kernel/src/arch/x86_64/CPU.cpp | 9 ++ kernel/src/main.cpp | 14 ++- kernel/src/memory/KernelVM.cpp | 2 +- kernel/src/memory/MemoryManager.cpp | 2 +- luna/CMakeLists.txt | 1 + luna/include/luna/Option.h | 18 +--- luna/include/luna/OwnedPtr.h | 5 ++ luna/include/luna/Result.h | 2 +- luna/include/luna/SharedPtr.h | 29 +++---- luna/include/luna/Spinlock.h | 130 ++++++++++++++++++++++++++++ luna/src/Heap.cpp | 24 +++-- luna/src/Spinlock.cpp | 58 +++++++++++++ 15 files changed, 254 insertions(+), 45 deletions(-) create mode 100644 luna/include/luna/Spinlock.h create mode 100644 luna/src/Spinlock.cpp diff --git a/kernel/CMakeLists.txt b/kernel/CMakeLists.txt index 6a341657..96029772 100644 --- a/kernel/CMakeLists.txt +++ b/kernel/CMakeLists.txt @@ -18,7 +18,6 @@ set(SOURCES src/arch/Serial.cpp src/arch/Timer.cpp src/arch/PCI.cpp - src/thread/Spinlock.cpp src/thread/Thread.cpp src/thread/Scheduler.cpp src/sys/Syscall.cpp diff --git a/kernel/src/Log.cpp b/kernel/src/Log.cpp index d28a5e2d..20ecc26a 100644 --- a/kernel/src/Log.cpp +++ b/kernel/src/Log.cpp @@ -2,10 +2,10 @@ #include "arch/CPU.h" #include "arch/Serial.h" #include "arch/Timer.h" -#include "thread/Spinlock.h" #include "video/TextConsole.h" #include #include +#include static bool g_debug_enabled = true; static bool g_serial_enabled = true; diff --git a/kernel/src/arch/CPU.h b/kernel/src/arch/CPU.h index 54422463..3e56d105 100644 --- a/kernel/src/arch/CPU.h +++ b/kernel/src/arch/CPU.h @@ -26,5 +26,7 @@ namespace CPU void get_stack_trace_at(Registers* regs, void (*callback)(u64, void*), void* arg); void print_stack_trace_at(Registers* regs); + [[noreturn]] void bootstrap_switch_stack(u64 stack, void* function); + void pause(); } diff --git a/kernel/src/arch/x86_64/CPU.cpp b/kernel/src/arch/x86_64/CPU.cpp index 5a56a7c8..b703071c 100644 --- a/kernel/src/arch/x86_64/CPU.cpp +++ b/kernel/src/arch/x86_64/CPU.cpp @@ -295,6 +295,15 @@ namespace CPU &frame_index); } + [[noreturn]] void bootstrap_switch_stack(u64 stack, void* function) + { + asm volatile("mov %0, %%rsp\n" + "jmp *%1" + : + : "r"(stack), "r"(function)); + __builtin_unreachable(); + } + void pause() { asm volatile("pause"); diff --git a/kernel/src/main.cpp b/kernel/src/main.cpp index 4a8d08c1..611ad23f 100644 --- a/kernel/src/main.cpp +++ b/kernel/src/main.cpp @@ -87,11 +87,19 @@ Result init() return {}; } -extern "C" [[noreturn]] void _start() +[[noreturn]] void init_wrapper() { - Init::check_magic(); - Init::early_init(); auto rc = init(); if (rc.has_error()) kerrorln("Runtime error: %s", rc.error_string()); CPU::idle_loop(); } + +// FIXME: Add a guard page to make sure the stack doesn't end up in random kernel memory. Also reclaim this memory after +// leaving the init task. +extern "C" [[noreturn]] void _start() +{ + Init::check_magic(); + Init::early_init(); + Stack stack { MemoryManager::alloc_for_kernel(8, MMU::ReadWrite | MMU::NoExecute).value(), 8 * ARCH_PAGE_SIZE }; + CPU::bootstrap_switch_stack(stack.top(), (void*)init_wrapper); +} diff --git a/kernel/src/memory/KernelVM.cpp b/kernel/src/memory/KernelVM.cpp index 48c4b80a..1773ba14 100644 --- a/kernel/src/memory/KernelVM.cpp +++ b/kernel/src/memory/KernelVM.cpp @@ -1,7 +1,7 @@ #include "memory/KernelVM.h" #include "arch/MMU.h" -#include "thread/Spinlock.h" #include +#include static const u64 KERNEL_VM_RANGE_START = 0xffffffffc0000000; diff --git a/kernel/src/memory/MemoryManager.cpp b/kernel/src/memory/MemoryManager.cpp index 3c010cda..96818eb7 100644 --- a/kernel/src/memory/MemoryManager.cpp +++ b/kernel/src/memory/MemoryManager.cpp @@ -2,10 +2,10 @@ #include "arch/MMU.h" #include "memory/KernelVM.h" #include "memory/MemoryMap.h" -#include "thread/Spinlock.h" #include #include #include +#include #include #include diff --git a/luna/CMakeLists.txt b/luna/CMakeLists.txt index 1b01daf5..652c341c 100644 --- a/luna/CMakeLists.txt +++ b/luna/CMakeLists.txt @@ -16,6 +16,7 @@ set(FREESTANDING_SOURCES src/TarStream.cpp src/DebugLog.cpp src/Heap.cpp + src/Spinlock.cpp ) set(SOURCES diff --git a/luna/include/luna/Option.h b/luna/include/luna/Option.h index bbe09698..70266c0f 100644 --- a/luna/include/luna/Option.h +++ b/luna/include/luna/Option.h @@ -101,7 +101,7 @@ template class Option return true; } - bool try_move_value(T& ref) const + bool try_move_value(T& ref) { if (!has_value()) return false; m_has_value = false; @@ -138,26 +138,16 @@ template class Option private: struct Storage { - u8 buffer[sizeof(T)]; - - T* fetch_ptr() - { - return (T*)buffer; - } + alignas(T) u8 buffer[sizeof(T)]; T& fetch_reference() { - return *fetch_ptr(); - } - - const T* fetch_ptr() const - { - return (const T*)buffer; + return *__builtin_launder(reinterpret_cast(&buffer)); } const T& fetch_reference() const { - return *fetch_ptr(); + return *__builtin_launder(reinterpret_cast(&buffer)); } void store_reference(const T& ref) diff --git a/luna/include/luna/OwnedPtr.h b/luna/include/luna/OwnedPtr.h index 319b8160..b831f65f 100644 --- a/luna/include/luna/OwnedPtr.h +++ b/luna/include/luna/OwnedPtr.h @@ -59,6 +59,11 @@ template class OwnedPtr return *m_ptr; } + operator bool() const + { + return m_ptr != nullptr; + } + template friend Result> adopt_shared_from_owned(OwnedPtr&&); private: diff --git a/luna/include/luna/Result.h b/luna/include/luna/Result.h index 8fd56785..9f833b8e 100644 --- a/luna/include/luna/Result.h +++ b/luna/include/luna/Result.h @@ -110,7 +110,7 @@ template class Result return m_value.try_set_value(ref); } - bool try_move_value(T& ref) const + bool try_move_value(T& ref) { return m_value.try_move_value(ref); } diff --git a/luna/include/luna/SharedPtr.h b/luna/include/luna/SharedPtr.h index cdf02fb5..020c37a5 100644 --- a/luna/include/luna/SharedPtr.h +++ b/luna/include/luna/SharedPtr.h @@ -99,31 +99,34 @@ template class SharedPtr return *m_ptr; } + operator bool() const + { + return m_ptr != nullptr; + } + private: T* m_ptr; RefCount* m_ref_count; }; -template Result> make_shared(Args... args) +// NOTE: ptr is deleted if any of the adopt_shared* functions fail to construct a SharedPtr. +template Result> adopt_shared(T* ptr) { using RefCount = __detail::RefCount; - RefCount* const ref_count = TRY(make()); - auto guard = make_scope_guard([&] { delete ref_count; }); + auto guard = make_scope_guard([ptr] { delete ptr; }); + + RefCount* const ref_count = TRY(make()); - T* const ptr = TRY(make(args...)); guard.deactivate(); return SharedPtr { ptr, ref_count }; } -template Result> adopt_shared(T* ptr) +template Result> make_shared(Args... args) { - using RefCount = __detail::RefCount; - - RefCount* const ref_count = TRY(make()); - - return SharedPtr { ptr, ref_count }; + T* raw_ptr = TRY(make(args...)); + return adopt_shared(raw_ptr); } template Result> adopt_shared_if_nonnull(T* ptr) @@ -138,13 +141,7 @@ template Result> adopt_shared_from_owned(OwnedPtr&& T* ptr = other.m_ptr; other.m_ptr = nullptr; - // FIXME: Should the pointee magically vanish on failure? Or go back into the OwnedPtr, even though it's been - // moved... - auto guard = make_scope_guard([&] { delete ptr; }); - const SharedPtr shared_ptr = TRY(adopt_shared(ptr)); - guard.deactivate(); - return shared_ptr; } diff --git a/luna/include/luna/Spinlock.h b/luna/include/luna/Spinlock.h new file mode 100644 index 00000000..0b284c17 --- /dev/null +++ b/luna/include/luna/Spinlock.h @@ -0,0 +1,130 @@ +#pragma once +#include +#include + +class Spinlock +{ + public: + void lock(); + void unlock(); + + bool try_lock(); + + bool is_locked() const + { + return m_lock.load() != 0; + } + + private: + Atomic m_lock { 0 }; +}; + +class ScopeLock +{ + public: + ScopeLock(Spinlock& lock); + ~ScopeLock(); + + ScopeLock(const ScopeLock&) = delete; + ScopeLock(ScopeLock&&) = delete; + + Spinlock& take_over() + { + m_taken_over = true; + return m_lock; + } + + private: + Spinlock& m_lock; + bool m_taken_over { false }; +}; + +class SafeScopeLock +{ + public: + SafeScopeLock(Spinlock& lock); + ~SafeScopeLock(); + + SafeScopeLock(const SafeScopeLock&) = delete; + SafeScopeLock(SafeScopeLock&&) = delete; + + bool did_succeed() const + { + return m_success; + } + + private: + Spinlock& m_lock; + bool m_success { false }; +}; + +template class LockedValue +{ + struct LockedValueGuard + { + LockedValueGuard(LockedValue& value_ref) : m_value_ref(&value_ref) + { + } + + LockedValueGuard(const LockedValueGuard& other) = delete; + LockedValueGuard(LockedValueGuard&& other) + { + m_value_ref = other.m_value_ref; + other.m_value_ref = nullptr; + } + + ~LockedValueGuard() + { + if (m_value_ref) m_value_ref->m_lock.unlock(); + } + + T& ref() + { + expect(m_value_ref, "LockedValueGuard::ref() called on a moved LockedValueGuard"); + return m_value_ref->m_value; + } + + void set(const T& other) + { + ref() = other; + } + + T* operator->() + { + return &ref(); + } + + T& operator*() + { + return ref(); + } + + private: + LockedValue* m_value_ref; + }; + + public: + LockedValue() : m_value() + { + } + + LockedValue(T value) : m_value(value) + { + } + + LockedValueGuard lock() + { + m_lock.lock(); + return { *this }; + } + + Option try_lock() + { + if (m_lock.try_lock()) { return { *this }; } + return {}; + } + + private: + T m_value; + Spinlock m_lock; +}; diff --git a/luna/src/Heap.cpp b/luna/src/Heap.cpp index 2e9b686f..81c6c45f 100644 --- a/luna/src/Heap.cpp +++ b/luna/src/Heap.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #ifdef USE_FREESTANDING @@ -45,9 +46,10 @@ static_assert(sizeof(HeapBlock) == 48UL); static const isize HEAP_BLOCK_SIZE = 48; static LinkedList heap; +static Spinlock g_heap_lock; -// If we're allocating a large amount of memory, map enough pages for it, but otherwise just use the default amount of -// pages. +// If we're allocating a large amount of memory, map enough pages for it, but otherwise just use the default amount +// of pages. static usize get_pages_for_allocation(usize bytes) { usize pages = get_blocks_from_size(bytes, PAGE_SIZE); @@ -97,7 +99,7 @@ static Option split(HeapBlock* block, usize size) const usize old_size = block->full_size; // Save the old value of this variable since we are going to use it after modifying it - if (available < (size + sizeof(HeapBlock))) + if (available <= (size + sizeof(HeapBlock))) return {}; // This block hasn't got enough free space to hold the requested size. const usize offset = get_fair_offset_to_split_at(block, size + sizeof(HeapBlock)); @@ -128,6 +130,8 @@ static Result combine_forward(HeapBlock* block) heap.remove(next); next->magic = BLOCK_DEAD; + block->full_size += next->full_size + sizeof(HeapBlock); + if (next->status & BLOCK_END_MEM) { if (next->status & BLOCK_START_MEM) @@ -140,8 +144,6 @@ static Result combine_forward(HeapBlock* block) block->status |= BLOCK_END_MEM; } - block->full_size += next->full_size + sizeof(HeapBlock); - return {}; } @@ -157,6 +159,8 @@ static Result combine_backward(HeapBlock* block) heap.remove(block); block->magic = BLOCK_DEAD; + last->full_size += block->full_size + sizeof(HeapBlock); + if (block->status & BLOCK_END_MEM) { if (block->status & BLOCK_START_MEM) @@ -169,8 +173,6 @@ static Result combine_backward(HeapBlock* block) last->status |= BLOCK_END_MEM; } - last->full_size += block->full_size + sizeof(HeapBlock); - return last; } @@ -178,6 +180,8 @@ Result malloc_impl(usize size, bool should_scrub) { if (!size) return (void*)BLOCK_MAGIC; + ScopeLock lock(g_heap_lock); + size = align_up<16>(size); Option block = heap.first(); @@ -231,6 +235,8 @@ Result free_impl(void* ptr) if (ptr == (void*)BLOCK_MAGIC) return {}; // This pointer was returned from a call to malloc(0) if (!ptr) return {}; + ScopeLock lock(g_heap_lock); + HeapBlock* block = get_heap_block_for_pointer(ptr); if (block->magic != BLOCK_MAGIC) @@ -286,6 +292,8 @@ Result realloc_impl(void* ptr, usize size) return (void*)BLOCK_MAGIC; } + ScopeLock lock(g_heap_lock); + HeapBlock* const block = get_heap_block_for_pointer(ptr); if (block->magic != BLOCK_MAGIC) @@ -327,6 +335,8 @@ Result realloc_impl(void* ptr, usize size) usize old_size = block->req_size; + lock.take_over().unlock(); + void* const new_ptr = TRY(malloc_impl(size, false)); memcpy(new_ptr, ptr, old_size > size ? size : old_size); TRY(free_impl(ptr)); diff --git a/luna/src/Spinlock.cpp b/luna/src/Spinlock.cpp new file mode 100644 index 00000000..2896aebe --- /dev/null +++ b/luna/src/Spinlock.cpp @@ -0,0 +1,58 @@ +#include +#include + +#ifdef ARCH_X86_64 +#define pause() asm volatile("pause") +#else +#error "Unsupported architecture" +#endif + +void Spinlock::lock() +{ + int expected = 0; + while (!m_lock.compare_exchange_strong(expected, 1)) + { + expected = 0; + pause(); + } +} + +bool Spinlock::try_lock() +{ + int expected = 0; + return m_lock.compare_exchange_strong(expected, 1); +} + +void Spinlock::unlock() +{ + int expected = 1; + if (!m_lock.compare_exchange_strong(expected, 0)) + { + dbgln("Spinlock::unlock() called on an unlocked lock with value %d", expected); + } +} + +ScopeLock::ScopeLock(Spinlock& lock) : m_lock(lock) +{ + m_lock.lock(); +} + +ScopeLock::~ScopeLock() +{ + if (!m_taken_over) m_lock.unlock(); +} + +const u32 RETRIES = 5000000; + +SafeScopeLock::SafeScopeLock(Spinlock& lock) : m_lock(lock) +{ + u32 tries_left = RETRIES; + while (!lock.try_lock() && --tries_left) { pause(); } + + if (tries_left) m_success = true; +} + +SafeScopeLock::~SafeScopeLock() +{ + if (m_success) m_lock.unlock(); +}