From 5dae85afe7eadfa0ef8c33252ef2cdd7e35faafd Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sat, 2 Jan 2021 16:38:05 +0100 Subject: [PATCH] Kernel: Pass "shared" flag to Region constructor Before this change, we would sometimes map a region into the address space with !is_shared(), and then moments later call set_shared(true). I found this very confusing while debugging, so this patch makes us pass the initial shared flag to the Region constructor, ensuring that it's in the correct state by the time we first map the region. --- Kernel/Devices/BXVGADevice.cpp | 3 ++- Kernel/Devices/MBVGADevice.cpp | 3 ++- Kernel/FileSystem/InodeFile.cpp | 2 +- Kernel/Process.cpp | 8 ++++---- Kernel/Process.h | 4 ++-- Kernel/SharedBuffer.cpp | 3 +-- Kernel/Syscalls/execve.cpp | 3 +-- Kernel/Syscalls/mmap.cpp | 2 +- Kernel/VM/Region.cpp | 9 +++++---- Kernel/VM/Region.h | 4 ++-- 10 files changed, 21 insertions(+), 20 deletions(-) diff --git a/Kernel/Devices/BXVGADevice.cpp b/Kernel/Devices/BXVGADevice.cpp index f6e8a749d8a..e85c01b2108 100644 --- a/Kernel/Devices/BXVGADevice.cpp +++ b/Kernel/Devices/BXVGADevice.cpp @@ -191,7 +191,8 @@ KResultOr BXVGADevice::mmap(Process& process, FileDescription&, Virtual vmobject.release_nonnull(), 0, "BXVGA Framebuffer", - prot); + prot, + shared); if (!region) return KResult(-ENOMEM); dbg() << "BXVGADevice: mmap with size " << region->size() << " at " << region->vaddr(); diff --git a/Kernel/Devices/MBVGADevice.cpp b/Kernel/Devices/MBVGADevice.cpp index 5b3bfdbe609..a62915156cf 100644 --- a/Kernel/Devices/MBVGADevice.cpp +++ b/Kernel/Devices/MBVGADevice.cpp @@ -67,7 +67,8 @@ KResultOr MBVGADevice::mmap(Process& process, FileDescription&, Virtual vmobject.release_nonnull(), 0, "MBVGA Framebuffer", - prot); + prot, + shared); if (!region) return KResult(-ENOMEM); dbg() << "MBVGADevice: mmap with size " << region->size() << " at " << region->vaddr(); diff --git a/Kernel/FileSystem/InodeFile.cpp b/Kernel/FileSystem/InodeFile.cpp index b1c3b2b45c4..c0c1c5f8304 100644 --- a/Kernel/FileSystem/InodeFile.cpp +++ b/Kernel/FileSystem/InodeFile.cpp @@ -79,7 +79,7 @@ KResultOr InodeFile::mmap(Process& process, FileDescription& descriptio vmobject = PrivateInodeVMObject::create_with_inode(inode()); if (!vmobject) return KResult(-ENOMEM); - auto* region = process.allocate_region_with_vmobject(preferred_vaddr, size, *vmobject, offset, description.absolute_path(), prot); + auto* region = process.allocate_region_with_vmobject(preferred_vaddr, size, *vmobject, offset, description.absolute_path(), prot, shared); if (!region) return KResult(-ENOMEM); return region; diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 06f7e06acf9..608772e872f 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -163,7 +163,7 @@ Region* Process::allocate_region(VirtualAddress vaddr, size_t size, const String return allocate_region(range, name, prot, strategy); } -Region* Process::allocate_region_with_vmobject(const Range& range, NonnullRefPtr vmobject, size_t offset_in_vmobject, const String& name, int prot) +Region* Process::allocate_region_with_vmobject(const Range& range, NonnullRefPtr vmobject, size_t offset_in_vmobject, const String& name, int prot, bool shared) { ASSERT(range.is_valid()); size_t end_in_vmobject = offset_in_vmobject + range.size(); @@ -180,18 +180,18 @@ Region* Process::allocate_region_with_vmobject(const Range& range, NonnullRefPtr return nullptr; } offset_in_vmobject &= PAGE_MASK; - auto& region = add_region(Region::create_user_accessible(this, range, move(vmobject), offset_in_vmobject, name, prot_to_region_access_flags(prot))); + auto& region = add_region(Region::create_user_accessible(this, range, move(vmobject), offset_in_vmobject, name, prot_to_region_access_flags(prot), true, shared)); if (!region.map(page_directory())) return nullptr; return ®ion; } -Region* Process::allocate_region_with_vmobject(VirtualAddress vaddr, size_t size, NonnullRefPtr vmobject, size_t offset_in_vmobject, const String& name, int prot) +Region* Process::allocate_region_with_vmobject(VirtualAddress vaddr, size_t size, NonnullRefPtr vmobject, size_t offset_in_vmobject, const String& name, int prot, bool shared) { auto range = allocate_range(vaddr, size); if (!range.is_valid()) return nullptr; - return allocate_region_with_vmobject(range, move(vmobject), offset_in_vmobject, name, prot); + return allocate_region_with_vmobject(range, move(vmobject), offset_in_vmobject, name, prot, shared); } bool Process::deallocate_region(Region& region) diff --git a/Kernel/Process.h b/Kernel/Process.h index 309f244d1f8..54a6c2f08a4 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -437,9 +437,9 @@ public: return m_euid == 0; } - Region* allocate_region_with_vmobject(VirtualAddress, size_t, NonnullRefPtr, size_t offset_in_vmobject, const String& name, int prot); + Region* allocate_region_with_vmobject(VirtualAddress, size_t, NonnullRefPtr, size_t offset_in_vmobject, const String& name, int prot, bool shared); Region* allocate_region(VirtualAddress, size_t, const String& name, int prot = PROT_READ | PROT_WRITE, AllocationStrategy strategy = AllocationStrategy::Reserve); - Region* allocate_region_with_vmobject(const Range&, NonnullRefPtr, size_t offset_in_vmobject, const String& name, int prot); + Region* allocate_region_with_vmobject(const Range&, NonnullRefPtr, size_t offset_in_vmobject, const String& name, int prot, bool shared); Region* allocate_region(const Range&, const String& name, int prot = PROT_READ | PROT_WRITE, AllocationStrategy strategy = AllocationStrategy::Reserve); bool deallocate_region(Region& region); diff --git a/Kernel/SharedBuffer.cpp b/Kernel/SharedBuffer.cpp index b077c3480f3..89d0fad1b04 100644 --- a/Kernel/SharedBuffer.cpp +++ b/Kernel/SharedBuffer.cpp @@ -89,11 +89,10 @@ void* SharedBuffer::ref_for_process_and_get_address(Process& process) for (auto& ref : m_refs) { if (ref.pid == process.pid()) { if (!ref.region) { - auto* region = process.allocate_region_with_vmobject(VirtualAddress(), size(), m_vmobject, 0, "SharedBuffer", PROT_READ | (m_writable ? PROT_WRITE : 0)); + auto* region = process.allocate_region_with_vmobject(VirtualAddress(), size(), m_vmobject, 0, "SharedBuffer", PROT_READ | (m_writable ? PROT_WRITE : 0), true); if (!region) return (void*)-ENOMEM; ref.region = region; - region->set_shared(true); } ref.count++; m_total_refs++; diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp index 3224ed071aa..3af99441e48 100644 --- a/Kernel/Syscalls/execve.cpp +++ b/Kernel/Syscalls/execve.cpp @@ -238,12 +238,11 @@ KResultOr Process::load_elf_object(FileDescription& object_ prot |= PROT_WRITE; if (program_header.is_executable()) prot |= PROT_EXEC; - auto* region = allocate_region_with_vmobject(program_header.vaddr().offset(load_offset), program_header.size_in_memory(), *vmobject, program_header.offset(), elf_name, prot); + auto* region = allocate_region_with_vmobject(program_header.vaddr().offset(load_offset), program_header.size_in_memory(), *vmobject, program_header.offset(), elf_name, prot, true); if (!region) { ph_load_result = KResult(-ENOMEM); return IterationDecision::Break; } - region->set_shared(true); if (program_header.offset() == 0) load_base_address = (FlatPtr)region->vaddr().as_ptr(); return IterationDecision::Continue; diff --git a/Kernel/Syscalls/mmap.cpp b/Kernel/Syscalls/mmap.cpp index 54a497f86c3..b1ccb31b8e3 100644 --- a/Kernel/Syscalls/mmap.cpp +++ b/Kernel/Syscalls/mmap.cpp @@ -438,7 +438,7 @@ void* Process::sys$mremap(Userspace user_param deallocate_region(*old_region); auto new_vmobject = PrivateInodeVMObject::create_with_inode(inode); - auto* new_region = allocate_region_with_vmobject(range.base(), range.size(), new_vmobject, 0, old_name, old_prot); + auto* new_region = allocate_region_with_vmobject(range.base(), range.size(), new_vmobject, 0, old_name, old_prot, false); new_region->set_mmap(true); if (!new_region) diff --git a/Kernel/VM/Region.cpp b/Kernel/VM/Region.cpp index 9f2545293a8..62ef18995ee 100644 --- a/Kernel/VM/Region.cpp +++ b/Kernel/VM/Region.cpp @@ -40,13 +40,14 @@ namespace Kernel { -Region::Region(const Range& range, NonnullRefPtr vmobject, size_t offset_in_vmobject, const String& name, u8 access, bool cacheable, bool kernel) +Region::Region(const Range& range, NonnullRefPtr vmobject, size_t offset_in_vmobject, const String& name, u8 access, bool cacheable, bool kernel, bool shared) : PurgeablePageRanges(vmobject) , m_range(range) , m_offset_in_vmobject(offset_in_vmobject) , m_vmobject(move(vmobject)) , m_name(name) , m_access(access) + , m_shared(shared) , m_cacheable(cacheable) , m_kernel(kernel) { @@ -236,9 +237,9 @@ size_t Region::amount_shared() const return bytes; } -NonnullOwnPtr Region::create_user_accessible(Process* owner, const Range& range, NonnullRefPtr vmobject, size_t offset_in_vmobject, const StringView& name, u8 access, bool cacheable) +NonnullOwnPtr Region::create_user_accessible(Process* owner, const Range& range, NonnullRefPtr vmobject, size_t offset_in_vmobject, const StringView& name, u8 access, bool cacheable, bool shared) { - auto region = make(range, move(vmobject), offset_in_vmobject, name, access, cacheable, false); + auto region = make(range, move(vmobject), offset_in_vmobject, name, access, cacheable, false, shared); if (owner) region->m_owner = owner->make_weak_ptr(); region->m_user_accessible = true; @@ -247,7 +248,7 @@ NonnullOwnPtr Region::create_user_accessible(Process* owner, const Range NonnullOwnPtr Region::create_kernel_only(const Range& range, NonnullRefPtr vmobject, size_t offset_in_vmobject, const StringView& name, u8 access, bool cacheable) { - auto region = make(range, move(vmobject), offset_in_vmobject, name, access, cacheable, true); + auto region = make(range, move(vmobject), offset_in_vmobject, name, access, cacheable, true, false); region->m_user_accessible = false; return region; } diff --git a/Kernel/VM/Region.h b/Kernel/VM/Region.h index fc48c530426..490de32cbff 100644 --- a/Kernel/VM/Region.h +++ b/Kernel/VM/Region.h @@ -61,7 +61,7 @@ public: ZeroedOnFork, }; - static NonnullOwnPtr create_user_accessible(Process*, const Range&, NonnullRefPtr, size_t offset_in_vmobject, const StringView& name, u8 access, bool cacheable = true); + static NonnullOwnPtr create_user_accessible(Process*, const Range&, NonnullRefPtr, size_t offset_in_vmobject, const StringView& name, u8 access, bool cacheable = true, bool shared = false); static NonnullOwnPtr create_kernel_only(const Range&, NonnullRefPtr, size_t offset_in_vmobject, const StringView& name, u8 access, bool cacheable = true); ~Region(); @@ -182,7 +182,7 @@ public: Region* m_prev { nullptr }; // NOTE: These are public so we can make<> them. - Region(const Range&, NonnullRefPtr, size_t offset_in_vmobject, const String&, u8 access, bool cacheable, bool kernel); + Region(const Range&, NonnullRefPtr, size_t offset_in_vmobject, const String&, u8 access, bool cacheable, bool kernel, bool shared); void set_inherit_mode(InheritMode inherit_mode) { m_inherit_mode = inherit_mode; }