Kernel: Simplify the File memory-mapping API

Before this change, we had File::mmap() which did all the work of
setting up a VMObject, and then creating a Region in the current
process's address space.

This patch simplifies the interface by removing the region part.
Files now only have to return a suitable VMObject from
vmobject_for_mmap(), and then sys$mmap() itself will take care of
actually mapping it into the address space.

This fixes an issue where we'd try to block on I/O (for inode metadata
lookup) while holding the address space spinlock. It also reduces time
spent holding the address space lock.

Kernel/Devices/KCOVDevice.cpp

@@ -116,7 +116,7 @@ ErrorOr<void> KCOVDevice::ioctl(OpenFileDescription&, unsigned request, Userspac
     }
 }
 
-ErrorOr<Memory::Region*> KCOVDevice::mmap(Process& process, Memory::AddressSpace& address_space, OpenFileDescription&, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
+ErrorOr<NonnullLockRefPtr<Memory::VMObject>> KCOVDevice::vmobject_for_mmap(Process& process, Memory::VirtualRange const&, u64&, bool)
 {
     auto pid = process.pid();
     auto maybe_kcov_instance = proc_instance->get(pid);
@@ -126,7 +126,7 @@ ErrorOr<Memory::Region*> KCOVDevice::mmap(Process& process, Memory::AddressSpace
     if (!kcov_instance->vmobject())
         return ENOBUFS; // mmaped, before KCOV_SETBUFSIZE
 
-    return address_space.allocate_region_with_vmobject(range, *kcov_instance->vmobject(), offset, {}, prot, shared);
+    return *kcov_instance->vmobject();
 }
 
 }

Kernel/Devices/KCOVDevice.h

@@ -22,7 +22,7 @@ public:
     static void free_process();
 
     // ^File
-    ErrorOr<Memory::Region*> mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
+    ErrorOr<NonnullLockRefPtr<Memory::VMObject>> vmobject_for_mmap(Process&, Memory::VirtualRange const&, u64& offset, bool shared) override;
     ErrorOr<NonnullLockRefPtr<OpenFileDescription>> open(int options) override;
 
 protected:

Kernel/Devices/MemoryDevice.cpp

@@ -42,7 +42,7 @@ ErrorOr<size_t> MemoryDevice::read(OpenFileDescription&, u64 offset, UserOrKerne
     return length;
 }
 
-ErrorOr<Memory::Region*> MemoryDevice::mmap(Process&, Memory::AddressSpace& address_space, OpenFileDescription&, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
+ErrorOr<NonnullLockRefPtr<Memory::VMObject>> MemoryDevice::vmobject_for_mmap(Process&, Memory::VirtualRange const& range, u64& offset, bool)
 {
     auto viewed_address = PhysicalAddress(offset);
 
@@ -61,15 +61,8 @@ ErrorOr<Memory::Region*> MemoryDevice::mmap(Process&, Memory::AddressSpace& addr
         return EINVAL;
     }
 
-    auto vmobject = TRY(Memory::AnonymousVMObject::try_create_for_physical_range(viewed_address, range.size()));
+    offset = 0;
-
+    return TRY(Memory::AnonymousVMObject::try_create_for_physical_range(viewed_address, range.size()));
-    return address_space.allocate_region_with_vmobject(
-        range,
-        move(vmobject),
-        0,
-        "Mapped Physical Memory"sv,
-        prot,
-        shared);
 }
 
 }

Kernel/Devices/MemoryDevice.h

@@ -19,7 +19,7 @@ public:
     static NonnullLockRefPtr<MemoryDevice> must_create();
     ~MemoryDevice();
 
-    virtual ErrorOr<Memory::Region*> mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
+    virtual ErrorOr<NonnullLockRefPtr<Memory::VMObject>> vmobject_for_mmap(Process&, Memory::VirtualRange const&, u64& offset, bool shared) override;
 
 private:
     MemoryDevice();

Kernel/FileSystem/AnonymousFile.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2021-2022, Andreas Kling <kling@serenityos.org>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -17,12 +17,12 @@ AnonymousFile::AnonymousFile(NonnullLockRefPtr<Memory::AnonymousVMObject> vmobje
 
 AnonymousFile::~AnonymousFile() = default;
 
-ErrorOr<Memory::Region*> AnonymousFile::mmap(Process&, Memory::AddressSpace& address_space, OpenFileDescription&, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
+ErrorOr<NonnullLockRefPtr<Memory::VMObject>> AnonymousFile::vmobject_for_mmap(Process&, Memory::VirtualRange const&, u64& offset, bool)
 {
     if (offset != 0)
         return EINVAL;
 
-    return address_space.allocate_region_with_vmobject(range, m_vmobject, offset, {}, prot, shared);
+    return m_vmobject;
 }
 
 ErrorOr<NonnullOwnPtr<KString>> AnonymousFile::pseudo_path(OpenFileDescription const&) const

Kernel/FileSystem/AnonymousFile.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2021-2022, Andreas Kling <kling@serenityos.org>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -20,7 +20,7 @@ public:
 
     virtual ~AnonymousFile() override;
 
-    virtual ErrorOr<Memory::Region*> mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
+    virtual ErrorOr<NonnullLockRefPtr<Memory::VMObject>> vmobject_for_mmap(Process&, Memory::VirtualRange const&, u64& offset, bool shared) override;
 
 private:
     virtual StringView class_name() const override { return "AnonymousFile"sv; }

Kernel/FileSystem/File.cpp

@@ -35,7 +35,7 @@ ErrorOr<void> File::ioctl(OpenFileDescription&, unsigned, Userspace<void*>)
     return ENOTTY;
 }
 
-ErrorOr<Memory::Region*> File::mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64, int, bool)
+ErrorOr<NonnullLockRefPtr<Memory::VMObject>> File::vmobject_for_mmap(Process&, Memory::VirtualRange const&, u64&, bool)
 {
     return ENODEV;
 }

Kernel/FileSystem/File.h

@@ -64,11 +64,11 @@ public:
 //   - Can be overridden in subclasses to implement arbitrary functionality.
 //   - Subclasses should take care to validate incoming addresses before dereferencing.
 //
-// mmap()
+// vmobject_for_mmap()
 //
 //   - Optional. If unimplemented, mmap() on this File will fail with -ENODEV.
 //   - Called by mmap() when userspace wants to memory-map this File somewhere.
-//   - Should create a Region in the Process and return it if successful.
+//   - Should return a VMObject suitable for mapping into the calling process.
 
 class File
     : public AtomicRefCounted<File>
@@ -90,7 +90,7 @@ public:
     virtual ErrorOr<size_t> read(OpenFileDescription&, u64, UserOrKernelBuffer&, size_t) = 0;
     virtual ErrorOr<size_t> write(OpenFileDescription&, u64, UserOrKernelBuffer const&, size_t) = 0;
     virtual ErrorOr<void> ioctl(OpenFileDescription&, unsigned request, Userspace<void*> arg);
-    virtual ErrorOr<Memory::Region*> mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared);
+    virtual ErrorOr<NonnullLockRefPtr<Memory::VMObject>> vmobject_for_mmap(Process&, Memory::VirtualRange const&, u64& offset, bool shared);
     virtual ErrorOr<struct stat> stat() const { return EBADF; }
 
     // Although this might be better described "name" or "description", these terms already have other meanings.

Kernel/FileSystem/InodeFile.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018-2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2018-2022, Andreas Kling <kling@serenityos.org>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -85,16 +85,11 @@ ErrorOr<void> InodeFile::ioctl(OpenFileDescription& description, unsigned reques
     }
 }
 
-ErrorOr<Memory::Region*> InodeFile::mmap(Process&, Memory::AddressSpace& address_space, OpenFileDescription& description, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
+ErrorOr<NonnullLockRefPtr<Memory::VMObject>> InodeFile::vmobject_for_mmap(Process&, Memory::VirtualRange const&, u64&, bool shared)
 {
-    // FIXME: If PROT_EXEC, check that the underlying file system isn't mounted noexec.
-    LockRefPtr<Memory::InodeVMObject> vmobject;
     if (shared)
-        vmobject = TRY(Memory::SharedInodeVMObject::try_create_with_inode(inode()));
+        return TRY(Memory::SharedInodeVMObject::try_create_with_inode(inode()));
-    else
+    return TRY(Memory::PrivateInodeVMObject::try_create_with_inode(inode()));
-        vmobject = TRY(Memory::PrivateInodeVMObject::try_create_with_inode(inode()));
-    auto path = TRY(description.pseudo_path());
-    return address_space.allocate_region_with_vmobject(range, vmobject.release_nonnull(), offset, path->view(), prot, shared);
 }
 
 ErrorOr<NonnullOwnPtr<KString>> InodeFile::pseudo_path(OpenFileDescription const&) const

Kernel/FileSystem/InodeFile.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018-2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2018-2022, Andreas Kling <kling@serenityos.org>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -33,7 +33,7 @@ public:
     virtual ErrorOr<size_t> read(OpenFileDescription&, u64, UserOrKernelBuffer&, size_t) override;
     virtual ErrorOr<size_t> write(OpenFileDescription&, u64, UserOrKernelBuffer const&, size_t) override;
     virtual ErrorOr<void> ioctl(OpenFileDescription&, unsigned request, Userspace<void*> arg) override;
-    virtual ErrorOr<Memory::Region*> mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
+    virtual ErrorOr<NonnullLockRefPtr<Memory::VMObject>> vmobject_for_mmap(Process&, Memory::VirtualRange const&, u64& offset, bool shared) override;
     virtual ErrorOr<struct stat> stat() const override { return inode().metadata().stat(); }
 
     virtual ErrorOr<NonnullOwnPtr<KString>> pseudo_path(OpenFileDescription const&) const override;

Kernel/FileSystem/OpenFileDescription.cpp

@@ -374,9 +374,9 @@ InodeMetadata OpenFileDescription::metadata() const
     return {};
 }
 
-ErrorOr<Memory::Region*> OpenFileDescription::mmap(Process& process, Memory::AddressSpace& address_space, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
+ErrorOr<NonnullLockRefPtr<Memory::VMObject>> OpenFileDescription::vmobject_for_mmap(Process& process, Memory::VirtualRange const& range, u64& offset, bool shared)
 {
-    return m_file->mmap(process, address_space, *this, range, offset, prot, shared);
+    return m_file->vmobject_for_mmap(process, range, offset, shared);
 }
 
 ErrorOr<void> OpenFileDescription::truncate(u64 length)

Kernel/FileSystem/OpenFileDescription.h

@@ -92,7 +92,7 @@ public:
     RefPtr<Custody> custody();
     RefPtr<Custody const> custody() const;
 
-    ErrorOr<Memory::Region*> mmap(Process&, Memory::AddressSpace&, Memory::VirtualRange const&, u64 offset, int prot, bool shared);
+    ErrorOr<NonnullLockRefPtr<Memory::VMObject>> vmobject_for_mmap(Process&, Memory::VirtualRange const&, u64& offset, bool shared);
 
     bool is_blocking() const;
     void set_blocking(bool b);

Kernel/Graphics/DisplayConnector.cpp

@@ -32,19 +32,13 @@ DisplayConnector::DisplayConnector(size_t framebuffer_resource_size, bool enable
 {
 }
 
-ErrorOr<Memory::Region*> DisplayConnector::mmap(Process&, Memory::AddressSpace& address_space, OpenFileDescription&, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
+ErrorOr<NonnullLockRefPtr<Memory::VMObject>> DisplayConnector::vmobject_for_mmap(Process&, Memory::VirtualRange const&, u64& offset, bool)
 {
     VERIFY(m_shared_framebuffer_vmobject);
     if (offset != 0)
         return Error::from_errno(ENOTSUP);
 
-    return address_space.allocate_region_with_vmobject(
+    return *m_shared_framebuffer_vmobject;
-        range,
-        *m_shared_framebuffer_vmobject,
-        0,
-        "Mapped Framebuffer"sv,
-        prot,
-        shared);
 }
 
 ErrorOr<size_t> DisplayConnector::read(OpenFileDescription&, u64, UserOrKernelBuffer&, size_t)

Kernel/Graphics/DisplayConnector.h

@@ -137,7 +137,7 @@ private:
     virtual bool can_write(OpenFileDescription const&, u64) const final override { return true; }
     virtual ErrorOr<size_t> read(OpenFileDescription&, u64, UserOrKernelBuffer&, size_t) override final;
     virtual ErrorOr<size_t> write(OpenFileDescription&, u64, UserOrKernelBuffer const&, size_t) override final;
-    virtual ErrorOr<Memory::Region*> mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64, int, bool) override final;
+    virtual ErrorOr<NonnullLockRefPtr<Memory::VMObject>> vmobject_for_mmap(Process&, Memory::VirtualRange const&, u64&, bool) override final;
     virtual ErrorOr<void> ioctl(OpenFileDescription&, unsigned request, Userspace<void*> arg) override final;
     virtual StringView class_name() const override final { return "DisplayConnector"sv; }
 

Kernel/Syscalls/mmap.cpp

@@ -190,10 +190,17 @@ ErrorOr<FlatPtr> Process::sys$mmap(Userspace<Syscall::SC_mmap_params const*> use
     if (map_stack && (!map_private || !map_anonymous))
         return EINVAL;
 
+    Memory::VirtualRange requested_range { VirtualAddress { addr }, rounded_size };
+    if (addr && !(map_fixed || map_fixed_noreplace)) {
+        // If there's an address but MAP_FIXED wasn't specified, the address is just a hint.
+        requested_range = { {}, rounded_size };
+    }
+
     Memory::Region* region = nullptr;
 
     LockRefPtr<OpenFileDescription> description;
-    LockRefPtr<Memory::AnonymousVMObject> vmobject;
+    LockRefPtr<Memory::VMObject> vmobject;
+    u64 used_offset = 0;
 
     if (map_anonymous) {
         auto strategy = map_noreserve ? AllocationStrategy::None : AllocationStrategy::Reserve;
@@ -206,6 +213,7 @@ ErrorOr<FlatPtr> Process::sys$mmap(Userspace<Syscall::SC_mmap_params const*> use
     } else {
         if (offset < 0)
             return EINVAL;
+        used_offset = static_cast<u64>(offset);
         if (static_cast<size_t>(offset) & ~PAGE_MASK)
             return EINVAL;
         description = TRY(open_file_description(fd));
@@ -220,6 +228,8 @@ ErrorOr<FlatPtr> Process::sys$mmap(Userspace<Syscall::SC_mmap_params const*> use
         }
         if (description->inode())
             TRY(validate_inode_mmap_prot(prot, *description->inode(), map_shared));
+
+        vmobject = TRY(description->vmobject_for_mmap(*this, requested_range, used_offset, map_shared));
     }
 
     return address_space().with([&](auto& space) -> ErrorOr<FlatPtr> {
@@ -227,17 +237,16 @@ ErrorOr<FlatPtr> Process::sys$mmap(Userspace<Syscall::SC_mmap_params const*> use
         if (map_fixed)
             TRY(space->unmap_mmap_range(VirtualAddress(addr), size));
 
-        Memory::VirtualRange requested_range { VirtualAddress { addr }, rounded_size };
+        region = TRY(space->allocate_region_with_vmobject(
-        if (addr && !(map_fixed || map_fixed_noreplace)) {
+            map_randomized ? Memory::RandomizeVirtualAddress::Yes : Memory::RandomizeVirtualAddress::No,
-            // If there's an address but MAP_FIXED wasn't specified, the address is just a hint.
+            requested_range.base(),
-            requested_range = { {}, rounded_size };
+            requested_range.size(),
-        }
+            alignment,
-
+            vmobject.release_nonnull(),
-        if (map_anonymous) {
+            used_offset,
-            region = TRY(space->allocate_region_with_vmobject(map_randomized ? Memory::RandomizeVirtualAddress::Yes : Memory::RandomizeVirtualAddress::No, requested_range.base(), requested_range.size(), alignment, vmobject.release_nonnull(), 0, {}, prot, map_shared));
+            {},
-        } else {
+            prot,
-            region = TRY(description->mmap(*this, *space, requested_range, static_cast<u64>(offset), prot, map_shared));
+            map_shared));
-        }
 
         if (!region)
             return ENOMEM;