From owner-freebsd-current@FreeBSD.ORG Tue Aug 7 11:00:31 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 11E0B16A420 for ; Tue, 7 Aug 2007 11:00:31 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from relay01.kiev.sovam.com (relay01.kiev.sovam.com [62.64.120.200]) by mx1.freebsd.org (Postfix) with ESMTP id 82E2913C48A for ; Tue, 7 Aug 2007 11:00:30 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from [89.162.146.170] (helo=skuns.kiev.zoral.com.ua) by relay01.kiev.sovam.com with esmtps (TLSv1:AES256-SHA:256) (Exim 4.67) (envelope-from ) id 1IIMnU-000MCK-0b for freebsd-current@freebsd.org; Tue, 07 Aug 2007 14:00:28 +0300 Received: from deviant.kiev.zoral.com.ua (root@[10.1.1.148]) by skuns.kiev.zoral.com.ua (8.14.1/8.14.1) with ESMTP id l77AdDqZ084796 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 7 Aug 2007 13:39:13 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.1/8.14.1) with ESMTP id l77AdDLw020185; Tue, 7 Aug 2007 13:39:13 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.1/8.14.1/Submit) id l77AdB92020184; Tue, 7 Aug 2007 13:39:11 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Tue, 7 Aug 2007 13:39:11 +0300 From: Kostik Belousov To: Tijl Coosemans Message-ID: <20070807103910.GI2738@deviant.kiev.zoral.com.ua> References: <200708051656.50168.tijl@ulyssis.org> <20070806113821.GB2738@deviant.kiev.zoral.com.ua> <200708062032.10514.tijl@ulyssis.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="SDQiEKxzCZ35UpgS" Content-Disposition: inline In-Reply-To: <200708062032.10514.tijl@ulyssis.org> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: ClamAV version 0.91.1, clamav-milter version 0.91.1 on skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-1.4 required=5.0 tests=ALL_TRUSTED autolearn=failed version=3.2.1 X-Spam-Checker-Version: SpamAssassin 3.2.1 (2007-05-02) on skuns.kiev.zoral.com.ua X-Scanner-Signature: 9733fe9a83ca81746226be428b8a047e X-DrWeb-checked: yes X-SpamTest-Envelope-From: kostikbel@gmail.com X-SpamTest-Group-ID: 00000000 X-SpamTest-Header: Not Detected X-SpamTest-Info: Profiles 1340 [August 7 2007] X-SpamTest-Info: helo_type=3 X-SpamTest-Method: none X-SpamTest-Rate: 0 X-SpamTest-Status: Not detected X-SpamTest-Status-Extended: not_detected X-SpamTest-Version: SMTP-Filter Version 3.0.0 [0255], KAS30/Release 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: Tue, 07 Aug 2007 11:00:31 -0000 --SDQiEKxzCZ35UpgS Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. > >>=20 > >> 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. > >>=20 > >> Some lines are commented out. If you remove the commenting, I hit on > >> the case where mmap(MAP_FIXED) returns -1. > >>=20 > >> 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? > >>=20 > >> #include > >> #include > >> #include > >> #include > >>=20 > >> #define MAPSIZE ( 16 * 4096 ) > >>=20 > >> void *second_thr( void *addr ) { > >> int i; > >> void *addr2; > >> for( i =3D 0; ; i++ ) { > >> addr2 =3D mmap( NULL, MAPSIZE, PROT_NONE, MAP_ANON | MAP_PRIVATE, -1= , 0 ); > >> /* > >> if( addr2 !=3D ( addr + MAPSIZE )) > >> break; > >> */ > >> munmap( addr2, MAPSIZE ); > >> } > >> printf( "thread2: addr(%p), addr2(%p), i(%d)\n", addr, addr2, i ); > >> return NULL; > >> } > >>=20 > >> int main( int argc, char **argv ) { > >> int i; > >> void *addr; > >> volatile char *addr_fix; > >> pthread_t thr; > >>=20 > >> addr =3D mmap( NULL, MAPSIZE, PROT_NONE, MAP_ANON | MAP_PRIVATE, -1, = 0 ); > >> pthread_create( &thr, NULL, &second_thr, addr ); > >>=20 > >> for( i =3D 0; ; i++ ) { > >> addr_fix =3D mmap( addr, MAPSIZE, PROT_WRITE, MAP_ANON | MAP_PRIVATE= | MAP_FIXED, -1, 0 ); > >> if( addr_fix =3D=3D addr ) > >> *addr_fix =3D i; /* should be writable */ > >> /* > >> else > >> break; > >> */ > >> } > >> printf( "thread1: addr(%p), addr_fix(%p), errno(%d), i(%d)\n", addr, = addr_fix, errno, i ); > >>=20 > >> pthread_join( thr, NULL ); > >> return 0; > >> } > >=20 [Old patch skipped]. > I don't think you can do this. Now, when vm_map_find() is called with > find_space=3DFALSE, 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 @@ -155,6 +155,22 @@ static void vmspace_zdtor(void *mem, int size, void *a= rg); #define PROC_VMSPACE_LOCK(p) do { } while (0) #define PROC_VMSPACE_UNLOCK(p) do { } while (0) =20 +/* + * 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 =3D vm_map_min(map); \ + if (end > vm_map_max(map)) \ + end =3D vm_map_max(map); \ + if (start > end) \ + start =3D end; \ + } + void vm_map_startup(void) { @@ -1178,6 +1194,26 @@ vm_map_find(vm_map_t map, vm_object_t object, vm_oof= fset_t offset, return (result); } =20 +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 =3D *addr; + vm_map_lock(map); + end =3D start + length; + VM_MAP_RANGE_CHECK(map, start, end); + (void) vm_map_delete(map, start, end); + result =3D vm_map_insert(map, object, offset, + start, end, prot, max, cow); + vm_map_unlock(map); + return (result); +} + /* * vm_map_simplify_entry: * @@ -1355,22 +1391,6 @@ _vm_map_clip_end(vm_map_t map, vm_map_entry_t entry,= vm_offset_t end) } =20 /* - * 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 =3D vm_map_min(map); \ - if (end > vm_map_max(map)) \ - end =3D vm_map_max(map); \ - if (start > end) \ - start =3D 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 @@ -333,6 +333,7 @@ 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_si= ze_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_s= ize_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 @@ -1341,7 +1341,6 @@ vm_mmap(vm_map_t map, vm_offset_t *addr, vm_size_t si= ze, vm_prot_t prot, if (*addr !=3D trunc_page(*addr)) return (EINVAL); fitit =3D FALSE; - (void) vm_map_remove(map, *addr, *addr + size); } /* * Lookup/allocate object. @@ -1400,8 +1399,11 @@ vm_mmap(vm_map_t map, vm_offset_t *addr, vm_size_t s= ize, vm_prot_t prot, if (flags & MAP_STACK) rv =3D vm_map_stack(map, *addr, size, prot, maxprot, docow | MAP_STACK_GROWS_DOWN); + else if (fitit) + rv =3D vm_map_find(map, object, foff, addr, size, TRUE, + prot, maxprot, docow); else - rv =3D vm_map_find(map, object, foff, addr, size, fitit, + rv =3D vm_map_fixed(map, object, foff, addr, size, prot, maxprot, docow); =20 if (rv !=3D KERN_SUCCESS) { --SDQiEKxzCZ35UpgS Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (FreeBSD) iD8DBQFGuEvOC3+MBN1Mb4gRAlk4AJ9wDdefcdTmQil2GCMGURMWYBsyMQCeIP4W f+Vtf3xEVZETqTrM9AhXeAg= =HFT6 -----END PGP SIGNATURE----- --SDQiEKxzCZ35UpgS--