kernel/vm: handle page protections in cut_area

- Resize the `page_protections` array in `cut_area` and also shift
the bits if necessary.
- Set the correct protection array as well as the real page
protections for the second area produced by `cut_area`.

Change-Id: I62293480487e869420ebe5a3bc729cec2a14c687
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6395
Reviewed-by: Jérôme Duval <[email protected]>
Tested-by: Commit checker robot <[email protected]>

headers/private/kernel/util/BitUtils.h

@@ -9,6 +9,8 @@
 #define KERNEL_UTIL_BITUTIL_H
 
 
+#include <string.h>
+
 #include <SupportDefs.h>
 
 
@@ -56,5 +58,58 @@ log2(uint32 v)
 }
 
 
+template<typename T>
+void
+bitmap_shift(T* bits, size_t bitCount, ssize_t shift)
+{
+	if (shift == 0)
+		return;
+
+	const size_t bitsPerElement = sizeof(T) * 8;
+	const size_t elementsCount = (bitCount + bitsPerElement - 1) / bitsPerElement;
+	const size_t absoluteShift = (shift > 0) ? shift : -shift;
+	const size_t nElements = absoluteShift / bitsPerElement;
+	const size_t nBits = absoluteShift % bitsPerElement;
+	if (nElements != 0) {
+		if (shift > 0) {
+			// "Left" shift.
+			memmove(&bits[nElements], bits, sizeof(T) * (elementsCount - nElements));
+			memset(bits, 0, sizeof(T) * nElements);
+		} else if (shift < 0) {
+			// "Right" shift.
+			memmove(bits, &bits[nElements], sizeof(T) * (elementsCount - nElements));
+			memset(&bits[elementsCount - nElements], 0, sizeof(T) * nElements);
+		}
+	}
+
+	// If the shift was by a multiple of the element size, nothing more to do.
+	if (nBits == 0)
+		return;
+
+	// One set of bits comes from the "current" element and are shifted in the
+	// direction of the shift; the other set comes from the next-processed
+	// element and are shifted in the opposite direction.
+	if (shift > 0) {
+		// "Left" shift.
+		for (ssize_t i = elementsCount - 1; i >= 0; i--) {
+			T low = 0;
+			if (i != 0)
+				low = bits[i - 1] >> (bitsPerElement - nBits);
+			const T high = bits[i] << nBits;
+			bits[i] = low | high;
+		}
+	} else if (shift < 0) {
+		// "Right" shift.
+		for (size_t i = 0; i < elementsCount; i++) {
+			const T low = bits[i] >> nBits;
+			T high = 0;
+			if (i != (elementsCount - 1))
+				high = bits[i + 1] << (bitsPerElement - nBits);
+			bits[i] = low | high;
+		}
+	}
+}
+
+
 #endif	// KERNEL_UTIL_BITUTIL_H
 

src/system/kernel/util/Bitmap.cpp

@@ -66,49 +66,7 @@ Bitmap::Resize(size_t bitCount)
 void
 Bitmap::Shift(ssize_t bitCount)
 {
-	if (bitCount == 0)
+	return bitmap_shift<addr_t>(fBits, fSize, bitCount);
-		return;
-
-	const size_t shift = (bitCount > 0) ? bitCount : -bitCount;
-	const size_t nElements = shift / kBitsPerElement, nBits = shift % kBitsPerElement;
-	if (nElements != 0) {
-		if (bitCount > 0) {
-			// "Left" shift.
-			memmove(&fBits[nElements], fBits, sizeof(addr_t) * (fElementsCount - nElements));
-			memset(fBits, 0, sizeof(addr_t) * nElements);
-		} else if (bitCount < 0) {
-			// "Right" shift.
-			memmove(fBits, &fBits[nElements], sizeof(addr_t) * (fElementsCount - nElements));
-			memset(&fBits[fElementsCount - nElements], 0, sizeof(addr_t) * nElements);
-		}
-	}
-
-	// If the shift was by a multiple of the element size, nothing more to do.
-	if (nBits == 0)
-		return;
-
-	// One set of bits comes from the "current" element and are shifted in the
-	// direction of the shift; the other set comes from the next-processed
-	// element and are shifted in the opposite direction.
-	if (bitCount > 0) {
-		// "Left" shift.
-		for (ssize_t i = fElementsCount - 1; i >= 0; i--) {
-			addr_t low = 0;
-			if (i != 0)
-				low = fBits[i - 1] >> (kBitsPerElement - nBits);
-			const addr_t high = fBits[i] << nBits;
-			fBits[i] = low | high;
-		}
-	} else if (bitCount < 0) {
-		// "Right" shift.
-		for (size_t i = 0; i < fElementsCount; i++) {
-			const addr_t low = fBits[i] >> nBits;
-			addr_t high = 0;
-			if (i != (fElementsCount - 1))
-				high = fBits[i + 1] << (kBitsPerElement - nBits);
-			fBits[i] = low | high;
-		}
-	}
 }
 
 

src/system/kernel/vm/vm.cpp

@@ -47,6 +47,7 @@
 #include <team.h>
 #include <tracing.h>
 #include <util/AutoLock.h>
+#include <util/BitUtils.h>
 #include <util/ThreadAutoLock.h>
 #include <vm/vm_page.h>
 #include <vm/vm_priv.h>
@@ -458,12 +459,19 @@ lookup_area(VMAddressSpace* addressSpace, area_id id)
 }
 
 
-static status_t
+static inline size_t
-allocate_area_page_protections(VMArea* area)
+area_page_protections_size(size_t areaSize)
 {
 	// In the page protections we store only the three user protections,
 	// so we use 4 bits per page.
-	size_t bytes = (area->Size() / B_PAGE_SIZE + 1) / 2;
+	return (areaSize / B_PAGE_SIZE + 1) / 2;
+}
+
+
+static status_t
+allocate_area_page_protections(VMArea* area)
+{
+	size_t bytes = area_page_protections_size(area->Size());
 	area->page_protections = (uint8*)malloc_etc(bytes,
 		area->address_space == VMAddressSpace::Kernel()
 			? HEAP_DONT_LOCK_KERNEL_SPACE : 0);
@@ -519,6 +527,16 @@ get_area_page_protection(VMArea* area, addr_t pageAddress)
 }
 
 
+static inline uint8*
+realloc_page_protections(uint8* pageProtections, size_t areaSize,
+	uint32 allocationFlags)
+{
+	size_t bytes = area_page_protections_size(areaSize);
+	// TODO: Implement realloc_etc and pass allocationFlags.
+	return (uint8*)realloc(pageProtections, bytes);
+}
+
+
 /*!	The caller must have reserved enough pages the translation map
 	implementation might need to map this page.
 	The page's cache must be locked.
@@ -676,6 +694,8 @@ cut_area(VMAddressSpace* addressSpace, VMArea* area, addr_t address,
 	bool onlyCacheUser = cache->areas == area && area->cache_next == NULL
 		&& cache->consumers.IsEmpty() && area->cache_type == CACHE_TYPE_RAM;
 
+	const addr_t oldSize = area->Size();
+
 	// Cut the end only?
 	if (offset > 0 && size == area->Size() - offset) {
 		status_t error = addressSpace->ShrinkAreaTail(area, offset,
@@ -683,6 +703,18 @@ cut_area(VMAddressSpace* addressSpace, VMArea* area, addr_t address,
 		if (error != B_OK)
 			return error;
 
+		if (area->page_protections != NULL) {
+			uint8* newProtections = realloc_page_protections(
+				area->page_protections, area->Size(), allocationFlags);
+
+			if (newProtections == NULL) {
+				addressSpace->ShrinkAreaTail(area, oldSize, allocationFlags);
+				return B_NO_MEMORY;
+			}
+
+			area->page_protections = newProtections;
+		}
+
 		// unmap pages
 		unmap_pages(area, address, size);
 
@@ -699,11 +731,36 @@ cut_area(VMAddressSpace* addressSpace, VMArea* area, addr_t address,
 
 	// Cut the beginning only?
 	if (area->Base() == address) {
+		uint8* newProtections = NULL;
+		if (area->page_protections != NULL) {
+			// Allocate all memory before shifting as the shift might lose some
+			// bits.
+			newProtections = realloc_page_protections(NULL, area->Size(),
+				allocationFlags);
+
+			if (newProtections == NULL)
+				return B_NO_MEMORY;
+		}
+
 		// resize the area
 		status_t error = addressSpace->ShrinkAreaHead(area, area->Size() - size,
 			allocationFlags);
-		if (error != B_OK)
+		if (error != B_OK) {
+			if (newProtections != NULL)
+				free_etc(newProtections, allocationFlags);
 			return error;
+		}
+
+		if (area->page_protections != NULL) {
+			size_t oldBytes = area_page_protections_size(oldSize);
+			ssize_t pagesShifted = (oldSize - area->Size()) / B_PAGE_SIZE;
+			bitmap_shift<uint8>(area->page_protections, oldBytes * 8, -(pagesShifted * 4));
+
+			size_t bytes = area_page_protections_size(area->Size());
+			memcpy(newProtections, area->page_protections, bytes);
+			free_etc(area->page_protections, allocationFlags);
+			area->page_protections = newProtections;
+		}
 
 		// unmap pages
 		unmap_pages(area, address, size);
@@ -731,12 +788,30 @@ cut_area(VMAddressSpace* addressSpace, VMArea* area, addr_t address,
 	unmap_pages(area, address, area->Size() - firstNewSize);
 
 	// resize the area
-	addr_t oldSize = area->Size();
 	status_t error = addressSpace->ShrinkAreaTail(area, firstNewSize,
 		allocationFlags);
 	if (error != B_OK)
 		return error;
 
+	uint8* areaNewProtections = NULL;
+	uint8* secondAreaNewProtections = NULL;
+
+	// Try to allocate the new memory before making some hard to reverse
+	// changes.
+	if (area->page_protections != NULL) {
+		areaNewProtections = realloc_page_protections(NULL, area->Size(),
+			allocationFlags);
+		secondAreaNewProtections = realloc_page_protections(NULL, secondSize,
+			allocationFlags);
+
+		if (areaNewProtections == NULL || secondAreaNewProtections == NULL) {
+			addressSpace->ShrinkAreaTail(area, oldSize, allocationFlags);
+			free_etc(areaNewProtections, allocationFlags);
+			free_etc(secondAreaNewProtections, allocationFlags);
+			return B_NO_MEMORY;
+		}
+	}
+
 	virtual_address_restrictions addressRestrictions = {};
 	addressRestrictions.address = (void*)secondBase;
 	addressRestrictions.address_specification = B_EXACT_ADDRESS;
@@ -750,6 +825,8 @@ cut_area(VMAddressSpace* addressSpace, VMArea* area, addr_t address,
 			dynamic_cast<VMAnonymousNoSwapCache*>(cache) == NULL, priority);
 		if (error != B_OK) {
 			addressSpace->ShrinkAreaTail(area, oldSize, allocationFlags);
+			free_etc(areaNewProtections, allocationFlags);
+			free_etc(secondAreaNewProtections, allocationFlags);
 			return error;
 		}
 
@@ -798,6 +875,8 @@ cut_area(VMAddressSpace* addressSpace, VMArea* area, addr_t address,
 			cache->ReleaseRefAndUnlock();
 			secondCache->ReleaseRefAndUnlock();
 			addressSpace->ShrinkAreaTail(area, oldSize, allocationFlags);
+			free_etc(areaNewProtections, allocationFlags);
+			free_etc(secondAreaNewProtections, allocationFlags);
 			return error;
 		}
 
@@ -812,12 +891,57 @@ cut_area(VMAddressSpace* addressSpace, VMArea* area, addr_t address,
 			&addressRestrictions, kernel, &secondArea, NULL);
 		if (error != B_OK) {
 			addressSpace->ShrinkAreaTail(area, oldSize, allocationFlags);
+			free_etc(areaNewProtections, allocationFlags);
+			free_etc(secondAreaNewProtections, allocationFlags);
 			return error;
 		}
 		// We need a cache reference for the new area.
 		cache->AcquireRefLocked();
 	}
 
+	if (area->page_protections != NULL) {
+		// Copy the protection bits of the first area.
+		size_t areaBytes = area_page_protections_size(area->Size());
+		memcpy(areaNewProtections, area->page_protections, areaBytes);
+		uint8* areaOldProtections = area->page_protections;
+		area->page_protections = areaNewProtections;
+
+		// Shift the protection bits of the second area to the start of
+		// the old array.
+		size_t oldBytes = area_page_protections_size(oldSize);
+		addr_t secondAreaOffset = secondBase - area->Base();
+		ssize_t secondAreaPagesShifted = secondAreaOffset / B_PAGE_SIZE;
+		bitmap_shift<uint8>(areaOldProtections, oldBytes * 8, -(secondAreaPagesShifted * 4));
+
+		// Copy the protection bits of the second area.
+		size_t secondAreaBytes = area_page_protections_size(secondSize);
+		memcpy(secondAreaNewProtections, areaOldProtections, secondAreaBytes);
+		secondArea->page_protections = secondAreaNewProtections;
+
+		// We don't need this anymore.
+		free_etc(areaOldProtections, allocationFlags);
+
+		// Set the correct page protections for the second area.
+		VMTranslationMap* map = addressSpace->TranslationMap();
+		map->Lock();
+		page_num_t firstPageOffset
+			= secondArea->cache_offset / B_PAGE_SIZE;
+		page_num_t lastPageOffset
+			= firstPageOffset + secondArea->Size() / B_PAGE_SIZE;
+		for (VMCachePagesTree::Iterator it
+				= secondArea->cache->pages.GetIterator();
+				vm_page* page = it.Next();) {
+			if (page->cache_offset >= firstPageOffset
+				&& page->cache_offset <= lastPageOffset) {
+				addr_t address = virtual_page_address(secondArea, page);
+				uint32 pageProtection
+					= get_area_page_protection(secondArea, address);
+				map->ProtectPage(secondArea, address, pageProtection);
+			}
+		}
+		map->Unlock();
+	}
+
 	if (_secondArea != NULL)
 		*_secondArea = secondArea;
 
@@ -2664,7 +2788,7 @@ vm_copy_area(team_id team, const char* name, void** _address,
 	uint8* targetPageProtections = NULL;
 
 	if (source->page_protections != NULL) {
-		size_t bytes = (source->Size() / B_PAGE_SIZE + 1) / 2;
+		size_t bytes = area_page_protections_size(source->Size());
 		targetPageProtections = (uint8*)malloc_etc(bytes,
 			(source->address_space == VMAddressSpace::Kernel()
 					|| targetAddressSpace == VMAddressSpace::Kernel())
@@ -5289,7 +5413,7 @@ vm_resize_area(area_id areaID, size_t newSize, bool kernel)
 	if (status == B_OK) {
 		// Shrink or grow individual page protections if in use.
 		if (area->page_protections != NULL) {
-			size_t bytes = (newSize / B_PAGE_SIZE + 1) / 2;
+			size_t bytes = area_page_protections_size(newSize);
 			uint8* newProtections
 				= (uint8*)realloc(area->page_protections, bytes);
 			if (newProtections == NULL)
@@ -5299,7 +5423,7 @@ vm_resize_area(area_id areaID, size_t newSize, bool kernel)
 
 				if (oldSize < newSize) {
 					// init the additional page protections to that of the area
-					uint32 offset = (oldSize / B_PAGE_SIZE + 1) / 2;
+					uint32 offset = area_page_protections_size(oldSize);
 					uint32 areaProtection = area->protection
 						& (B_READ_AREA | B_WRITE_AREA | B_EXECUTE_AREA);
 					memset(area->page_protections + offset,

src/tests/system/kernel/mmap_cut_tests.cpp

@@ -10,21 +10,26 @@
 #include <OS.h>
 
 
-int
+int gTestFd = -1;
-main()
-{
-	int fd = open("/boot/system/lib/libroot.so", O_CLOEXEC | O_RDONLY);
-	if (fd < 0)
-		return -1;
 
+
+int
+map_negative_offset_test()
+{
 	// should fail (negative offset)
-	void* ptr0 = mmap(NULL, B_PAGE_SIZE, PROT_READ, MAP_PRIVATE, fd, -4096);
+	void* ptr = mmap(NULL, B_PAGE_SIZE, PROT_READ, MAP_PRIVATE, gTestFd, -4096);
-	if (ptr0 != NULL) {
+	if (ptr != MAP_FAILED) {
 		printf("map-negative-offset unexpectedly succeeded!\n");
 		return -1;
 	}
+	return 0;
+}
 
-	uint8* ptr1 = (uint8*)mmap(NULL, 16 * B_PAGE_SIZE, PROT_READ, MAP_PRIVATE, fd, 0);
+
+int
+map_cut_compare_test()
+{
+	uint8* ptr1 = (uint8*)mmap(NULL, 16 * B_PAGE_SIZE, PROT_READ, MAP_PRIVATE, gTestFd, 0);
 	uint8 chunk[128];
 	memcpy(chunk, &ptr1[3 * B_PAGE_SIZE], sizeof(chunk));
 
@@ -38,6 +43,52 @@ main()
 		printf("map-cut-compare test failed!\n");
 		return status;
 	}
+	return 0;
+}
+
+
+int
+map_protect_cut_test()
+{
+	uint8* ptr = (uint8*)mmap(NULL, B_PAGE_SIZE * 4, PROT_NONE,
+		MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+
+	// make the tail accessible
+	mprotect(ptr + B_PAGE_SIZE * 3, B_PAGE_SIZE, PROT_READ | PROT_WRITE);
+
+	// store any value
+	ptr[B_PAGE_SIZE * 3] = 'a';
+
+	// cut the area in the middle, before the accessible tail
+	mmap(ptr + B_PAGE_SIZE, B_PAGE_SIZE, PROT_READ | PROT_WRITE,
+		MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
+
+	// validate that this does not crash
+	if (ptr[B_PAGE_SIZE * 3] != 'a') {
+		printf("map-protect-cut test failed!\n");
+		return -1;
+	}
+	return 0;
+}
+
+
+int
+main()
+{
+	gTestFd = open("/boot/system/lib/libroot.so", O_CLOEXEC | O_RDONLY);
+	if (gTestFd < 0)
+		return -1;
+
+	int status;
+
+	if ((status = map_negative_offset_test()) != 0)
+		return status;
+
+	if ((status = map_cut_compare_test()) != 0)
+		return status;
+
+	if ((status = map_protect_cut_test()) != 0)
+		return status;
 
 	return 0;
 }