From owner-freebsd-current@FreeBSD.ORG Mon Aug 6 18:33:10 2007 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9E99616A417 for ; Mon, 6 Aug 2007 18:33:10 +0000 (UTC) (envelope-from tijl@ulyssis.org) Received: from mailrelay011.isp.belgacom.be (mailrelay011.isp.belgacom.be [195.238.6.178]) by mx1.freebsd.org (Postfix) with ESMTP id 4469613C45A for ; Mon, 6 Aug 2007 18:33:10 +0000 (UTC) (envelope-from tijl@ulyssis.org) Received: from 199.121-247-81.adsl-dyn.isp.belgacom.be (HELO kalimero.kotnet.org) ([81.247.121.199]) by mailrelay011.isp.belgacom.be with ESMTP; 06 Aug 2007 20:32:12 +0200 Received: from localhost (localhost [127.0.0.1]) by kalimero.kotnet.org (8.14.1/8.14.1) with ESMTP id l76IWBNG065776; Mon, 6 Aug 2007 20:32:11 +0200 (CEST) (envelope-from tijl@ulyssis.org) From: Tijl Coosemans To: Kostik Belousov Date: Mon, 6 Aug 2007 20:32:08 +0200 User-Agent: KMail/1.9.7 References: <200708051656.50168.tijl@ulyssis.org> <20070806113821.GB2738@deviant.kiev.zoral.com.ua> In-Reply-To: <20070806113821.GB2738@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200708062032.10514.tijl@ulyssis.org> Cc: wine-freebsd@hub.org, freebsd-current@freebsd.org Subject: Re: mmap(2) MAP_FIXED isn't thread-safe (+testcase) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Aug 2007 18:33:10 -0000 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 >> #include >> #include >> #include >> >> #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?