From 74c037e938a315d0aff6fb5eb107bcaba5bd89df Mon Sep 17 00:00:00 2001
From: Augustin Cavalier <waddlesplash@gmail.com>
Date: Mon, 16 Dec 2024 22:52:59 -0500
Subject: [PATCH] kernel/vm: Fix problems in merging caches of differing sizes.

Previously, we'd wind up adding pages from the source to the consumer
that were potentially or actually outside the consumer's bounds.
Now we check the consumer's size and ignore any pages that we don't
want or need; they'll just be freed along with the source cache.

While at it, drop VMAnonymousCache::_MergePagesSmallerSource; it
was the same as the base class's implementation of Merge preceding
this commit; and add a comment to _MergePagesSmallerConsumer noting
that some of the pages may be busy (indeed, I manage to trigger an
assert related to copy-on-write in here at least once.)

I discovered this problem because the page commitment size ASSERT()s
triggered inside Resize() and Rebase(); but the out-of-range pages
already existed in the cache before those functions were called. So,
I've also added an ASSERT to MovePage() that would have caught this
problem more directly.
---
 src/system/kernel/vm/VMAnonymousCache.cpp | 39 +++++++----------------
 src/system/kernel/vm/VMAnonymousCache.h   |  2 --
 src/system/kernel/vm/VMCache.cpp          | 11 +++++--
 3 files changed, 21 insertions(+), 31 deletions(-)

diff --git a/src/system/kernel/vm/VMAnonymousCache.cpp b/src/system/kernel/vm/VMAnonymousCache.cpp
index eff47ce2f6..34b6fa0ebd 100644
--- a/src/system/kernel/vm/VMAnonymousCache.cpp
+++ b/src/system/kernel/vm/VMAnonymousCache.cpp
@@ -1045,10 +1045,13 @@ VMAnonymousCache::Merge(VMCache* _source)
 	_MergeSwapPages(source);
 
 	// Move all not shadowed pages from the source to the consumer cache.
-	if (source->page_count < page_count)
-		_MergePagesSmallerSource(source);
-	else
+	if (source->page_count > page_count
+			&& source->virtual_base == virtual_base
+			&& source->virtual_end == virtual_end) {
 		_MergePagesSmallerConsumer(source);
+	} else {
+		VMCache::Merge(source);
+	}
 }
 
 
@@ -1215,28 +1218,6 @@ VMAnonymousCache::_Commit(off_t size, int priority)
 }
 
 
-void
-VMAnonymousCache::_MergePagesSmallerSource(VMAnonymousCache* source)
-{
-	// The source cache has less pages than the consumer (this cache), so we
-	// iterate through the source's pages and move the ones that are not
-	// shadowed up to the consumer.
-
-	for (VMCachePagesTree::Iterator it = source->pages.GetIterator();
-			vm_page* page = it.Next();) {
-		// Note: Removing the current node while iterating through a
-		// IteratableSplayTree is safe.
-		vm_page* consumerPage = LookupPage(
-			(off_t)page->cache_offset << PAGE_SHIFT);
-		if (consumerPage == NULL) {
-			// the page is not yet in the consumer cache - move it upwards
-			ASSERT_PRINT(!page->busy, "page: %p", page);
-			MovePage(page);
-		}
-	}
-}
-
-
 void
 VMAnonymousCache::_MergePagesSmallerConsumer(VMAnonymousCache* source)
 {
@@ -1244,8 +1225,12 @@ VMAnonymousCache::_MergePagesSmallerConsumer(VMAnonymousCache* source)
 	// consumer's pages to the source (freeing shadowed ones) and finally just
 	// all pages of the source back to the consumer.
 
-	for (VMCachePagesTree::Iterator it = pages.GetIterator();
-		vm_page* page = it.Next();) {
+	// It is possible that some of the pages we are moving here are actually "busy".
+	// Since all the pages that belong to this cache will belong to it again by
+	// the time we unlock, that should be fine.
+
+	VMCachePagesTree::Iterator it = pages.GetIterator();
+	while (vm_page* page = it.Next()) {
 		// If a source page is in the way, remove and free it.
 		vm_page* sourcePage = source->LookupPage(
 			(off_t)page->cache_offset << PAGE_SHIFT);
diff --git a/src/system/kernel/vm/VMAnonymousCache.h b/src/system/kernel/vm/VMAnonymousCache.h
index 1d1142869d..e7150a9fac 100644
--- a/src/system/kernel/vm/VMAnonymousCache.h
+++ b/src/system/kernel/vm/VMAnonymousCache.h
@@ -90,8 +90,6 @@ private:
 			swap_addr_t			_SwapBlockGetAddress(off_t pageIndex);
 			status_t			_Commit(off_t size, int priority);
 
-			void				_MergePagesSmallerSource(
-									VMAnonymousCache* source);
 			void				_MergePagesSmallerConsumer(
 									VMAnonymousCache* source);
 			void				_MergeSwapPages(VMAnonymousCache* source);
diff --git a/src/system/kernel/vm/VMCache.cpp b/src/system/kernel/vm/VMCache.cpp
index 18642aa518..18b83b8e83 100644
--- a/src/system/kernel/vm/VMCache.cpp
+++ b/src/system/kernel/vm/VMCache.cpp
@@ -852,6 +852,7 @@ VMCache::MovePage(vm_page* page, off_t offset)
 
 	AssertLocked();
 	oldCache->AssertLocked();
+	ASSERT(offset >= virtual_base && (offset + B_PAGE_SIZE) <= virtual_end);
 
 	// remove from old cache
 	oldCache->pages.Remove(page);
@@ -1391,8 +1392,14 @@ VMCache::Fault(struct VMAddressSpace *aspace, off_t offset)
 void
 VMCache::Merge(VMCache* source)
 {
-	for (VMCachePagesTree::Iterator it = source->pages.GetIterator();
-			vm_page* page = it.Next();) {
+	const page_num_t firstOffset = ROUNDDOWN(virtual_base, B_PAGE_SIZE) >> PAGE_SHIFT,
+		endOffset = (page_num_t)((virtual_end + B_PAGE_SIZE - 1) >> PAGE_SHIFT);
+
+	VMCachePagesTree::Iterator it = source->pages.GetIterator();
+	while (vm_page* page = it.Next()) {
+		if (page->cache_offset < firstOffset || page->cache_offset >= endOffset)
+			continue;
+
 		// Note: Removing the current node while iterating through a
 		// IteratableSplayTree is safe.
 		vm_page* consumerPage = LookupPage(