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

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;
>> }
> 
> 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_ -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);

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?