Kernel/Memory: Various fixes for anonymous mmaps

[brody-qq]

This PR fixes the following problems related to anonymous mmaps:

• After a fork, writes to an anonymous mmap cause redundant page faults (Kernel/Memory: Redundant page faults on anonymous mmap regions after fork #24637)
• Writes to a shared anonymous mmap are not visible to other processes that share the mmap (Kernel/Memory: Shared anonymous mmaps not working correctly #24638)
• VMObject::try_clone() overcommits pages to m_shared_committed_cow_pages

This PR also adds more test cases for anonymous mmaps.

[BuggieBot]

Hello!

One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

[nico]

@spholz You know this code a bit, right? Want to look at this? :)

[Hendiadyoin1]

Apart from one nit
LGMT

[spholz]

One small question, but otherwise LGTM

[spholz]

AnonymousVMObject::should_cow returns true if the physical page is the shared_zero_page or the lazy_committed_page, so I am unsure if we have other code expecting that pages referencing these physical pages are CoW (it does kind of make sense to make these CoW, since when writing to those we have to zero them and therefore essentially copy the contents).

[spholz]

And AddressSpace::try_allocate_split_region also sets the CoW bit for all pages where Region::should_cow (which uses `AnonymousVMObjet::should_cow) returns true.

[spholz]

Ah oops, initially an AnonymousVMObject will of course be backed by the actual committed pages or references to either the lazy committed page or the shared zero page.
And those don't have a cow map allocated, so none of the pages are cow.

But AddressSpace::try_allocate_split_region should I think still probably not set the cow bit for shared zero pages and lazy committed pages.

[brody-qq]

Thanks for the feedback, I completely missed the set_should_cow call in AddressSpace::try_allocate_split_region.

After taking a closer look at AddressSpace::try_allocate_split_region, I think the best approach is to just remove the loop that calls set_should_cow.
Here is the diff:

diff --git a/Kernel/Memory/AddressSpace.cpp b/Kernel/Memory/AddressSpace.cpp
index 9ea53b88a1..e96406e8b1 100644
--- a/Kernel/Memory/AddressSpace.cpp
+++ b/Kernel/Memory/AddressSpace.cpp
@@ -148,11 +148,6 @@ ErrorOr<Region*> AddressSpace::try_allocate_split_region(Region const& source_re
     new_region->set_syscall_region(source_region.is_syscall_region());
     new_region->set_mmap(source_region.is_mmap(), source_region.mmapped_from_readable(), source_region.mmapped_from_writable());
     new_region->set_stack(source_region.is_stack());
-    size_t page_offset_in_source_region = (offset_in_vmobject - source_region.offset_in_vmobject()) / PAGE_SIZE;
-    for (size_t i = 0; i < new_region->page_count(); ++i) {
-        if (source_region.should_cow(page_offset_in_source_region + i))
-            TRY(new_region->set_should_cow(i, true));
-    }
     TRY(m_region_tree.place_specifically(*new_region, range));
     return new_region.leak_ptr();
 }

I think this works because if you look at this line of code, it sets new_region to have the same m_vmobject as source_region before the loop runs:

auto new_region = TRY(Region::create_unplaced(
        source_region.vmobject(), offset_in_vmobject, move(region_name), source_region.access(), source_region.is_cacheable() ? Region::Cacheable::Yes : Region::Cacheable::No, source_region.is_shared()));

So that set_should_cow loop really isn't doing anything, since source_region and new_region reference the same vmobject and cow map.

Does this change make sense? (I hope I'm not missing something obvious here)

[brody-qq]

Also ProcFS is the only other place I can find that may be impacted by not marking lazy/zero pages in the cow map.

Particularly this line for setting cow_pages:

           TRY(region_object.add("cow_pages"sv, region.cow_pages()));

So if you go with my changes, then ProcFS will report a smaller number than before for cow_pages, since it isn't counting the lazy/zero pages anymore.

I don't know enough about how this cow_pages number from ProcFS gets used to know if this is a problem, does anyone have any objections to this?

[spholz]

Removing this loop from AddressSpace::try_allocate_split_region seems fine.
This code was added in f4e7aec. Back then the m_cow_map was a member of Region:

serenity/Kernel/VM/Region.h

Line 193 in f4e7aec

mutable OwnPtr<Bitmap> m_cow_map;

So this loop should indeed be unnecessary now, since the m_cow_map is now owned by AnonymousVMObjects.

It may also be nice if we could have a test (in munmap-multi-region-unmapping.cpp maybe?) for splitting a region that is backed by an AnonymousVMObject with pages marked as CoW.

And about procfs potentially showing a different cow_pages count: This should be fine, we only use that procfs parameter in the pmap utility and the "Memory map" tab in SystemMonitor to display the number of CoW pages.

[brody-qq]

I added a new commit that removes the loop from AddressSpace::try_allocate_split_region and I added some new test code to munmap-multi-region-unmapping.cpp. Can you let me know if this test code is what you had in mind?

Edit: The new test code is part of the Tests/Kernel: Add new tests for anonymous mmaps commit

[spholz]

I don't think that test triggers the try_allocate_split_region code path.
try_allocate_split_region seems to get called on munmaps that don't cover an entire mapping and on mmaps with MAP_FIXED that overlap, but your mmaps don't seem to overlap.

I think the test should do something like this:

• mmap more than 1 page
• write some data into the mapping
• fork
• in the child:
  • munmap part of the mapping
  • write to the still mapped part
• in the parent:
  • wait for the child
  • verify that the original data is still there

Also, munmap-multi-region-unmapping.cpp is a legacy non-LibTest test. As you may have noticed, those are skipped by default. I am unsure if it is a good idea to add new tests to legacy test files even if they would be the right place for this test.
Sorry, I should have noticed this earlier (especially based on the filename).

(munmap-multi-region-unmapping.cpp seems like a test which we should convert to LibTest, you don't need to do that in this pr though)

[spholz]

I also don't think that not having an accompanying test should be a blocker (I am not a maintainer). I just thought it might be nice to test this behavior.

[brody-qq]

Ah ok, I misunderstood your original comment. I put the test case you described in TestAnonymousMmaps.cpp and got rid of the test I added to the legacy file.

[Hendiadyoin1]

Seems like one of the other kernel PRs created some conflicts

[brody-qq]

Seems like one of the other kernel PRs created some conflicts

Fixed, it was some new code in Region.cpp that didn't impact any functionality of this PR.

[nico]

Thanks!

Since you're in this part of the code: If you're looking for things to do, it'd be nice if posix_spawn() could become a syscall instead of being implemented in userspace. We could save the time needed to copy page metadata then. Most processes are launched using posix_spawn() in serenity, so that might help with performance.