From 6654021655b13c74b521764dd56dacea0e6ec379 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Holz?= Date: Sun, 3 Mar 2024 14:54:10 +0100 Subject: [PATCH] Kernel/riscv64: Don't hard-code the page fault reason on RISC-V Instead, rewrite the region page fault handling code to not use PageFault::type() on RISC-V. I split Region::handle_fault into having a RISC-V-specific implementation, as I am not sure if I cover all page fault handling edge cases by solely relying on MM's own region metadata. We should probably also take the processor-provided page fault reason into account, if we decide to merge these two implementations in the future. --- Kernel/Arch/PageFault.cpp | 3 +- Kernel/Arch/PageFault.h | 3 +- Kernel/Arch/riscv64/Interrupts.cpp | 5 +- Kernel/Memory/Region.cpp | 52 ++++++++++++++++++++ Userland/Applications/CrashReporter/main.cpp | 4 +- 5 files changed, 60 insertions(+), 7 deletions(-) diff --git a/Kernel/Arch/PageFault.cpp b/Kernel/Arch/PageFault.cpp index 033bb41f3a..50ea1a5650 100644 --- a/Kernel/Arch/PageFault.cpp +++ b/Kernel/Arch/PageFault.cpp @@ -120,7 +120,8 @@ void PageFault::handle(RegisterState& regs) auto fault_address_string = KString::formatted("{:p}", fault_address); auto fault_address_view = fault_address_string.is_error() ? ""sv : fault_address_string.value()->view(); (void)current_process.try_set_coredump_property("fault_address"sv, fault_address_view); - (void)current_process.try_set_coredump_property("fault_type"sv, type() == PageFault::Type::PageNotPresent ? "NotPresent"sv : "ProtectionViolation"sv); + if (type() != PageFault::Type::Unknown) + (void)current_process.try_set_coredump_property("fault_type"sv, type() == PageFault::Type::PageNotPresent ? "NotPresent"sv : "ProtectionViolation"sv); StringView fault_access; if (is_instruction_fetch()) fault_access = "Execute"sv; diff --git a/Kernel/Arch/PageFault.h b/Kernel/Arch/PageFault.h index 04f3dd0a1c..c0e14f88ee 100644 --- a/Kernel/Arch/PageFault.h +++ b/Kernel/Arch/PageFault.h @@ -50,6 +50,7 @@ public: enum class Type { PageNotPresent = PageFaultFlags::NotPresent, ProtectionViolation = PageFaultFlags::ProtectionViolation, + Unknown, }; enum class Access { @@ -90,7 +91,7 @@ public: bool is_instruction_fetch() const { return m_is_instruction_fetch; } private: - Type m_type; + Type m_type = Type::Unknown; Access m_access; ExecutionMode m_execution_mode; bool m_is_reserved_bit_violation { false }; diff --git a/Kernel/Arch/riscv64/Interrupts.cpp b/Kernel/Arch/riscv64/Interrupts.cpp index f8d0f4255b..babc7b091a 100644 --- a/Kernel/Arch/riscv64/Interrupts.cpp +++ b/Kernel/Arch/riscv64/Interrupts.cpp @@ -96,9 +96,8 @@ extern "C" void trap_handler(TrapFrame& trap_frame) else if (scause == StoreOrAMOPageFault) fault.set_access(PageFault::Access::Write); - // FIXME: RISC-V doesn't tell you *why* a memory access failed, only the original access type (r/w/x). - // So either detect the correct type by walking the page table or rewrite MM to not depend on the processor-provided reason. - fault.set_type(PageFault::Type::ProtectionViolation); + // RISC-V doesn't tell you the reason why a page fault occurred, so we don't use PageFault::set_type() here. + // The RISC-V implementation of Region::handle_fault() works without a correct PageFault::type(). fault.handle(*trap_frame.regs); break; diff --git a/Kernel/Memory/Region.cpp b/Kernel/Memory/Region.cpp index c68a55b5f7..9a081b901d 100644 --- a/Kernel/Memory/Region.cpp +++ b/Kernel/Memory/Region.cpp @@ -358,6 +358,7 @@ void Region::clear_to_zero() PageFaultResponse Region::handle_fault(PageFault const& fault) { +#if !ARCH(RISCV64) auto page_index_in_region = page_index_from_address(fault.vaddr()); if (fault.type() == PageFault::Type::PageNotPresent) { if (fault.is_read() && !is_readable()) { @@ -404,6 +405,57 @@ PageFaultResponse Region::handle_fault(PageFault const& fault) } dbgln("PV(error) fault in Region({})[{}] at {}", this, page_index_in_region, fault.vaddr()); return PageFaultResponse::ShouldCrash; +#else + // FIXME: Consider to merge the RISC-V page fault handling code with the x86_64/aarch64 implementation. + // RISC-V doesn't tell you *why* a memory access failed, only the original access type (r/w/x). + // We probably should take the page fault reason into account, if the processor provides it. + + auto page_index_in_region = page_index_from_address(fault.vaddr()); + + if (fault.is_read() && !is_readable()) { + dbgln("Read page fault in non-readable Region({})[{}]", this, page_index_in_region); + return PageFaultResponse::ShouldCrash; + } + + if (fault.is_write() && !is_writable()) { + dbgln("Write page fault in non-writable Region({})[{}] at {}", this, page_index_in_region, fault.vaddr()); + return PageFaultResponse::ShouldCrash; + } + + if (fault.is_instruction_fetch() && !is_executable()) { + dbgln("Instruction fetch page fault in non-executable Region({})[{}] at {}", this, page_index_in_region, fault.vaddr()); + return PageFaultResponse::ShouldCrash; + } + + if (fault.is_write() && is_writable() && should_cow(page_index_in_region)) { + dbgln_if(PAGE_FAULT_DEBUG, "CoW page fault in Region({})[{}] at {}", this, page_index_in_region, fault.vaddr()); + auto phys_page = physical_page(page_index_in_region); + if (phys_page->is_shared_zero_page() || phys_page->is_lazy_committed_page()) { + dbgln_if(PAGE_FAULT_DEBUG, "Zero page fault in Region({})[{}] at {}", this, page_index_in_region, fault.vaddr()); + return handle_zero_fault(page_index_in_region, *phys_page); + } + return handle_cow_fault(page_index_in_region); + } + + if (vmobject().is_inode()) { + dbgln_if(PAGE_FAULT_DEBUG, "Inode page fault in Region({})[{}]", this, page_index_in_region); + return handle_inode_fault(page_index_in_region); + } + + SpinlockLocker vmobject_locker(vmobject().m_lock); + auto& page_slot = physical_page_slot(page_index_in_region); + if (page_slot->is_lazy_committed_page()) { + auto page_index_in_vmobject = translate_to_vmobject_page(page_index_in_region); + VERIFY(m_vmobject->is_anonymous()); + page_slot = static_cast(*m_vmobject).allocate_committed_page({}); + if (!remap_vmobject_page(page_index_in_vmobject, *page_slot)) + return PageFaultResponse::OutOfMemory; + return PageFaultResponse::Continue; + } + + dbgln("Unexpected page fault in Region({})[{}] at {}", this, page_index_in_region, fault.vaddr()); + return PageFaultResponse::ShouldCrash; +#endif } PageFaultResponse Region::handle_zero_fault(size_t page_index_in_region, PhysicalPage& page_in_slot_at_time_of_fault) diff --git a/Userland/Applications/CrashReporter/main.cpp b/Userland/Applications/CrashReporter/main.cpp index c2ec437d24..d007e71026 100644 --- a/Userland/Applications/CrashReporter/main.cpp +++ b/Userland/Applications/CrashReporter/main.cpp @@ -81,8 +81,8 @@ static TitleAndText build_backtrace(Coredump::Reader const& coredump, ELF::Core: auto fault_address = metadata.get("fault_address"); auto fault_type = metadata.get("fault_type"); auto fault_access = metadata.get("fault_access"); - if (fault_address.has_value() && fault_type.has_value() && fault_access.has_value()) { - builder.appendff("{} fault on {} at address {}", fault_type.value(), fault_access.value(), fault_address.value()); + if (fault_address.has_value() && fault_access.has_value()) { + builder.appendff("{} fault on {} at address {}", fault_type.value_or("Page"), fault_access.value(), fault_address.value()); constexpr FlatPtr malloc_scrub_pattern = explode_byte(MALLOC_SCRUB_BYTE); constexpr FlatPtr free_scrub_pattern = explode_byte(FREE_SCRUB_BYTE); auto raw_fault_address = AK::StringUtils::convert_to_uint_from_hex(fault_address.value().substring_view(2));