Re: mmap(2) MAP_FIXED isn't thread-safe (+testcase)

[I removed personal addresses from Cc:]

On Sun, Aug 05, 2007 at 04:56:46PM +0200, Tijl Coosemans wrote:
> Hi all,
> 
> While investigating ports/115092 and other reports of seemingly random
> page faults when running Wine, I think I've found the cause to be mmap
> not being thread-safe when MAP_FIXED is used. It causes mmap(MAP_FIXED)
> to return -1(ENOMEM) sometimes when it shouldn't, but also to return an
> address with wrong protections, hence the protection faults occuring.
> 
> Attached is a test program that shows this. It runs two threads. The
> first mmap()'s a region, starts a second thread and then goes in a loop
> calling mmap(PROT_WRITE,MAP_FIXED) on that region, essentially
> replacing that mapping. This is basically what rtld does to map an ELF
> object for instance when dlopen(3) is called. The second thread tries
> to steal the mapping from the first by calling mmap(PROT_NONE) in a
> loop. After a while the program segfaults when the first thread tries
> to write to the mapped region.
> 
> Some lines are commented out. If you remove the commenting, I hit on
> the case where mmap(MAP_FIXED) returns -1.
> 
> The problem is in sys/vm/vm_mmap.c:vm_mmap(). In case of MAP_FIXED
> first vm_map_remove() is called and then later vm_map_find(). This
> would need some locking, but I don't know which lock or how to approach
> this, so can somebody have a look at this?

> #include <sys/mman.h>
> #include <errno.h>
> #include <pthread.h>
> #include <stdio.h>
> 
> #define MAPSIZE		( 16 * 4096 )
> 
> void *second_thr( void *addr ) {
> 	int i;
> 	void *addr2;
> 	for( i = 0; ; i++ ) {
> 		addr2 = mmap( NULL, MAPSIZE, PROT_NONE, MAP_ANON | MAP_PRIVATE, -1, 0 );
> /*
> 		if( addr2 != ( addr + MAPSIZE ))
> 			break;
> */
> 		munmap( addr2, MAPSIZE );
> 	}
> 	printf( "thread2: addr(%p), addr2(%p), i(%d)\n", addr, addr2, i );
> 	return NULL;
> }
> 
> int main( int argc, char **argv ) {
> 	int i;
> 	void *addr;
> 	volatile char *addr_fix;
> 	pthread_t thr;
> 
> 	addr = mmap( NULL, MAPSIZE, PROT_NONE, MAP_ANON | MAP_PRIVATE, -1, 0 );
> 	pthread_create( &thr, NULL, &second_thr, addr );
> 
> 	for( i = 0; ; i++ ) {
> 		addr_fix = mmap( addr, MAPSIZE, PROT_WRITE, MAP_ANON | MAP_PRIVATE | MAP_FIXED, -1, 0 );
> 		if( addr_fix == addr )
> 			*addr_fix = i; /* should be writable */
> /*
> 		else
> 			break;
> */
> 	}
> 	printf( "thread1: addr(%p), addr_fix(%p), errno(%d), i(%d)\n", addr, addr_fix, errno, i );
> 
> 	pthread_join( thr, NULL );
> 	return 0;
> }

Try the patch below. It changes behaviour for MAP_STACK|MAP_FIXED case, but
I am not sure whether it worth fixing.

diff --git a/sys/vm/vm_map.c b/sys/vm/vm_map.c
index 8799cc5..b9dabe8 100644
--- a/sys/vm/vm_map.c
+++ b/sys/vm/vm_map.c
_at__at_ -155,6 +155,22 _at__at_ static void vmspace_zdtor(void *mem, int size, void *arg);
 #define PROC_VMSPACE_LOCK(p) do { } while (0)
 #define PROC_VMSPACE_UNLOCK(p) do { } while (0)
 
+/*
+ *	VM_MAP_RANGE_CHECK:	[ internal use only ]
+ *
+ *	Asserts that the starting and ending region
+ *	addresses fall within the valid range of the map.
+ */
+#define	VM_MAP_RANGE_CHECK(map, start, end)		\
+		{					\
+		if (start < vm_map_min(map))		\
+			start = vm_map_min(map);	\
+		if (end > vm_map_max(map))		\
+			end = vm_map_max(map);		\
+		if (start > end)			\
+			start = end;			\
+		}
+
 void
 vm_map_startup(void)
 {
_at__at_ -1160,7 +1176,7 _at__at_ vm_map_find(vm_map_t map, vm_object_t object, vm_ooffset_t offset,
 	    vm_size_t length, boolean_t find_space, vm_prot_t prot,
 	    vm_prot_t max, int cow)
 {
-	vm_offset_t start;
+	vm_offset_t start, end;
 	int result;
 
 	start = *addr;
_at__at_ -1171,9 +1187,14 _at__at_ vm_map_find(vm_map_t map, vm_object_t object, vm_ooffset_t offset,
 			return (KERN_NO_SPACE);
 		}
 		start = *addr;
+		end = start + length;
+	} else {
+		end = start + length;
+		VM_MAP_RANGE_CHECK(map, start, end);
+		(void) vm_map_delete(map, start, end);
 	}
 	result = vm_map_insert(map, object, offset,
-		start, start + length, prot, max, cow);
+		start, end, prot, max, cow);
 	vm_map_unlock(map);
 	return (result);
 }
_at__at_ -1355,22 +1376,6 _at__at_ _vm_map_clip_end(vm_map_t map, vm_map_entry_t entry, vm_offset_t end)
 }
 
 /*
- *	VM_MAP_RANGE_CHECK:	[ internal use only ]
- *
- *	Asserts that the starting and ending region
- *	addresses fall within the valid range of the map.
- */
-#define	VM_MAP_RANGE_CHECK(map, start, end)		\
-		{					\
-		if (start < vm_map_min(map))		\
-			start = vm_map_min(map);	\
-		if (end > vm_map_max(map))		\
-			end = vm_map_max(map);		\
-		if (start > end)			\
-			start = end;			\
-		}
-
-/*
  *	vm_map_submap:		[ kernel use only ]
  *
  *	Mark the given range as handled by a subordinate map.
diff --git a/sys/vm/vm_mmap.c b/sys/vm/vm_mmap.c
index 9522fec..a48fd77 100644
--- a/sys/vm/vm_mmap.c
+++ b/sys/vm/vm_mmap.c
_at__at_ -1341,7 +1341,6 _at__at_ vm_mmap(vm_map_t map, vm_offset_t *addr, vm_size_t size, vm_prot_t prot,
 		if (*addr != trunc_page(*addr))
 			return (EINVAL);
 		fitit = FALSE;
-		(void) vm_map_remove(map, *addr, *addr + size);
 	}
 	/*
 	 * Lookup/allocate object.