From 497c759ab773b73a2f66a82ad13901b9178ee49e Mon Sep 17 00:00:00 2001 From: Idan Horowitz Date: Wed, 7 Apr 2021 02:17:05 +0300 Subject: [PATCH] Kernel: Remove old region from process' regions vector before splitting This does not affect functionality right now, but it means that the regions vector will now never have any overlapping regions, which will allow the use of balance binary search trees instead of a vector in the future. (since they require keys to be exclusive) --- Kernel/Syscalls/mmap.cpp | 44 +++++++++++++++++++++++++--------------- Kernel/VM/Space.cpp | 11 ++++++---- Kernel/VM/Space.h | 1 + 3 files changed, 36 insertions(+), 20 deletions(-) diff --git a/Kernel/Syscalls/mmap.cpp b/Kernel/Syscalls/mmap.cpp index d818701629..db401a59c5 100644 --- a/Kernel/Syscalls/mmap.cpp +++ b/Kernel/Syscalls/mmap.cpp @@ -341,20 +341,24 @@ KResultOr Process::sys$mprotect(Userspace addr, size_t size, int pro return EACCES; } + // Remove the old region from our regions tree, since were going to add another region + // with the exact same start address, but dont deallocate it yet + auto region = space().take_region(*old_region); + VERIFY(region); + + // Unmap the old region here, specifying that we *don't* want the VM deallocated. + region->unmap(Region::ShouldDeallocateVirtualMemoryRange::No); + // This vector is the region(s) adjacent to our range. // We need to allocate a new region for the range we wanted to change permission bits on. - auto adjacent_regions = space().split_region_around_range(*old_region, range_to_mprotect); + auto adjacent_regions = space().split_region_around_range(*region, range_to_mprotect); - size_t new_range_offset_in_vmobject = old_region->offset_in_vmobject() + (range_to_mprotect.base().get() - old_region->range().base().get()); - auto& new_region = space().allocate_split_region(*old_region, range_to_mprotect, new_range_offset_in_vmobject); + size_t new_range_offset_in_vmobject = region->offset_in_vmobject() + (range_to_mprotect.base().get() - region->range().base().get()); + auto& new_region = space().allocate_split_region(*region, range_to_mprotect, new_range_offset_in_vmobject); new_region.set_readable(prot & PROT_READ); new_region.set_writable(prot & PROT_WRITE); new_region.set_executable(prot & PROT_EXEC); - // Unmap the old region here, specifying that we *don't* want the VM deallocated. - old_region->unmap(Region::ShouldDeallocateVirtualMemoryRange::No); - space().deallocate_region(*old_region); - // Map the new regions using our page directory (they were just allocated and don't have one). for (auto* adjacent_region : adjacent_regions) { adjacent_region->map(space().page_directory()); @@ -475,11 +479,15 @@ KResultOr Process::sys$munmap(Userspace addr, size_t size) if (!old_region->is_mmap()) return EPERM; - auto new_regions = space().split_region_around_range(*old_region, range_to_unmap); + // Remove the old region from our regions tree, since were going to add another region + // with the exact same start address, but dont deallocate it yet + auto region = space().take_region(*old_region); + VERIFY(region); // We manually unmap the old region here, specifying that we *don't* want the VM deallocated. - old_region->unmap(Region::ShouldDeallocateVirtualMemoryRange::No); - space().deallocate_region(*old_region); + region->unmap(Region::ShouldDeallocateVirtualMemoryRange::No); + + auto new_regions = space().split_region_around_range(*region, range_to_unmap); // Instead we give back the unwanted VM manually. space().page_directory().range_allocator().deallocate(range_to_unmap); @@ -512,13 +520,16 @@ KResultOr Process::sys$munmap(Userspace addr, size_t size) continue; } - // otherwise just split the regions and collect them for future mapping - new_regions.append(space().split_region_around_range(*old_region, range_to_unmap)); + // Remove the old region from our regions tree, since were going to add another region + // with the exact same start address, but dont deallocate it yet + auto region = space().take_region(*old_region); + VERIFY(region); // We manually unmap the old region here, specifying that we *don't* want the VM deallocated. - old_region->unmap(Region::ShouldDeallocateVirtualMemoryRange::No); - bool res = space().deallocate_region(*old_region); - VERIFY(res); + region->unmap(Region::ShouldDeallocateVirtualMemoryRange::No); + + // otherwise just split the regions and collect them for future mapping + new_regions.append(space().split_region_around_range(*region, range_to_unmap)); } // Instead we give back the unwanted VM manually at the end. space().page_directory().range_allocator().deallocate(range_to_unmap); @@ -559,7 +570,8 @@ KResultOr Process::sys$mremap(Userspaceunmap(Region::ShouldDeallocateVirtualMemoryRange::No); - space().deallocate_region(*old_region); + bool success = space().deallocate_region(*old_region); + VERIFY(success); auto new_vmobject = PrivateInodeVMObject::create_with_inode(inode); diff --git a/Kernel/VM/Space.cpp b/Kernel/VM/Space.cpp index 94a7628f95..bb48d700d0 100644 --- a/Kernel/VM/Space.cpp +++ b/Kernel/VM/Space.cpp @@ -118,18 +118,21 @@ KResultOr Space::allocate_region_with_vmobject(const Range& range, Nonn bool Space::deallocate_region(Region& region) { - OwnPtr region_protector; + return take_region(region); +} + +OwnPtr Space::take_region(Region& region) +{ ScopedSpinLock lock(m_lock); if (m_region_lookup_cache.region.unsafe_ptr() == ®ion) m_region_lookup_cache.region = nullptr; for (size_t i = 0; i < m_regions.size(); ++i) { if (&m_regions[i] == ®ion) { - region_protector = m_regions.unstable_take(i); - return true; + return m_regions.unstable_take(i); } } - return false; + return {}; } Region* Space::find_region_from_range(const Range& range) diff --git a/Kernel/VM/Space.h b/Kernel/VM/Space.h index dde64230dc..fd5ae428b1 100644 --- a/Kernel/VM/Space.h +++ b/Kernel/VM/Space.h @@ -58,6 +58,7 @@ public: KResultOr allocate_region_with_vmobject(const Range&, NonnullRefPtr, size_t offset_in_vmobject, const String& name, int prot, bool shared); KResultOr allocate_region(const Range&, const String& name, int prot = PROT_READ | PROT_WRITE, AllocationStrategy strategy = AllocationStrategy::Reserve); bool deallocate_region(Region& region); + OwnPtr take_region(Region& region); Region& allocate_split_region(const Region& source_region, const Range&, size_t offset_in_vmobject); Vector split_region_around_range(const Region& source_region, const Range&);