Date: Mon, 6 Aug 2007 20:32:08 +0200 From: Tijl Coosemans <tijl@ulyssis.org> To: Kostik Belousov <kostikbel@gmail.com> Cc: wine-freebsd@hub.org, freebsd-current@freebsd.org Subject: Re: mmap(2) MAP_FIXED isn't thread-safe (+testcase) Message-ID: <200708062032.10514.tijl@ulyssis.org> In-Reply-To: <20070806113821.GB2738@deviant.kiev.zoral.com.ua> References: <200708051656.50168.tijl@ulyssis.org> <20070806113821.GB2738@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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
> @@ -1160,7 +1176,7 @@ 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;
> @@ -1171,9 +1187,14 @@ 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?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200708062032.10514.tijl>