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

On Mon, Aug 06, 2007 at 08:32:08PM +0200, Tijl Coosemans wrote:
> On Monday 06 August 2007 13:38:21 Kostik Belousov wrote:
> > On Sun, Aug 05, 2007 at 04:56:46PM +0200, Tijl Coosemans wrote:
> >> 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;
> >> }
> > 
[Old patch skipped].

> I don't think you can do this. Now, when vm_map_find() is called with
> find_space=FALSE, it will delete any existing mapping instead of
> failing if the address is not available. Have you checked all locations
> where this function is called to make sure this isn't harmful?

Yes, I looked over this when writing the patch. I think this is what
actually supposed to happen in that case.

Anyway, to not diverge from the old behaviour when fixing the issue,
please test patch below. I do not like introducing recursive locks where
it is quite easy to avoid them.


diff --git a/sys/vm/vm_map.c b/sys/vm/vm_map.c
index 8799cc5..177e0db 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_ -1178,6 +1194,26 _at__at_ vm_map_find(vm_map_t map, vm_object_t object, vm_ooffset_t offset,
 	return (result);
 }
 
+int
+vm_map_fixed(vm_map_t map, vm_object_t object, vm_ooffset_t offset,
+	    vm_offset_t *addr,	/* IN/OUT */
+	    vm_size_t length, vm_prot_t prot,
+	    vm_prot_t max, int cow)
+{
+	vm_offset_t start, end;
+	int result;
+
+	start = *addr;
+	vm_map_lock(map);
+	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, end, prot, max, cow);
+	vm_map_unlock(map);
+	return (result);
+}
+
 /*
  *	vm_map_simplify_entry:
  *
_at__at_ -1355,22 +1391,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_map.h b/sys/vm/vm_map.h
index 93aad51..1805a79 100644
--- a/sys/vm/vm_map.h
+++ b/sys/vm/vm_map.h
_at__at_ -333,6 +333,7 _at__at_ boolean_t vm_map_check_protection (vm_map_t, vm_offset_t, vm_offset_t, vm_prot_t
 vm_map_t vm_map_create(pmap_t, vm_offset_t, vm_offset_t);
 int vm_map_delete (vm_map_t, vm_offset_t, vm_offset_t);
 int vm_map_find (vm_map_t, vm_object_t, vm_ooffset_t, vm_offset_t *, vm_size_t, boolean_t, vm_prot_t, vm_prot_t, int);
+int vm_map_fixed (vm_map_t, vm_object_t, vm_ooffset_t, vm_offset_t *, vm_size_t, vm_prot_t, vm_prot_t, int);
 int vm_map_findspace (vm_map_t, vm_offset_t, vm_size_t, vm_offset_t *);
 int vm_map_inherit (vm_map_t, vm_offset_t, vm_offset_t, vm_inherit_t);
 void vm_map_init (struct vm_map *, vm_offset_t, vm_offset_t);
diff --git a/sys/vm/vm_mmap.c b/sys/vm/vm_mmap.c
index 9522fec..4f44817 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.
_at__at_ -1400,8 +1399,11 _at__at_ vm_mmap(vm_map_t map, vm_offset_t *addr, vm_size_t size, vm_prot_t prot,
 	if (flags & MAP_STACK)
 		rv = vm_map_stack(map, *addr, size, prot, maxprot,
 		    docow | MAP_STACK_GROWS_DOWN);
+	else if (fitit)
+		rv = vm_map_find(map, object, foff, addr, size, TRUE,
+				 prot, maxprot, docow);
 	else
-		rv = vm_map_find(map, object, foff, addr, size, fitit,
+		rv = vm_map_fixed(map, object, foff, addr, size,
 				 prot, maxprot, docow);
 
 	if (rv != KERN_SUCCESS) {