From b0321bf29097bb2fb5c5b8ae8c97244ae5953426 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 1 Nov 2019 17:44:40 +0100 Subject: [PATCH] Kernel: Zero-fill faults should not temporarily enable interrupts We were doing a temporary STI/CLI in MemoryManager::zero_page() to be able to acquire the VMObject's lock before zeroing out a page. This logic was inherited from the inode fault handler, where we need to enable interrupts anyway, since we might need to interact with the underlying storage device. Zero-fill faults don't actually need to lock the VMObject, since they are already guaranteed exclusivity by interrupts being disabled when entering the fault handler. This is different from inode faults, where a second thread can often get an inode fault for the same exact page in the same VMObject before the first fault handler has received a response from the disk. This is why the lock exists in the first place, to prevent this race. This fixes an intermittent crash in sys$execve() that was made much more visible after I made userspace stacks lazily allocated. --- Kernel/VM/MemoryManager.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Kernel/VM/MemoryManager.cpp b/Kernel/VM/MemoryManager.cpp index 74ba159f2f..0f7f2cbfa2 100644 --- a/Kernel/VM/MemoryManager.cpp +++ b/Kernel/VM/MemoryManager.cpp @@ -293,9 +293,10 @@ bool MemoryManager::zero_page(Region& region, unsigned page_index_in_region) ASSERT_INTERRUPTS_DISABLED(); auto& vmo = region.vmobject(); auto& vmo_page = vmo.physical_pages()[region.first_page_index() + page_index_in_region]; - sti(); - LOCKER(vmo.m_paging_lock); - cli(); + ASSERT(vmo.is_anonymous()); + + // NOTE: We don't need to acquire the VMObject's lock. + // This function is already exclusive due to interrupts being blocked. if (!vmo_page.is_null()) { #ifdef PAGE_FAULT_DEBUG