Date: Mon, 6 Aug 2007 14:38:21 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Tijl Coosemans <tijl@ulyssis.org> Cc: wine-freebsd@hub.org, freebsd-current@freebsd.org Subject: Re: mmap(2) MAP_FIXED isn't thread-safe (+testcase) Message-ID: <20070806113821.GB2738@deviant.kiev.zoral.com.ua> In-Reply-To: <200708051656.50168.tijl@ulyssis.org> References: <200708051656.50168.tijl@ulyssis.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--2bJ57vwr75KGnr5s
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
[I removed personal addresses from Cc:]
On Sun, Aug 05, 2007 at 04:56:46PM +0200, Tijl Coosemans wrote:
> Hi all,
>=20
> 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?
> #include <sys/mman.h>
> #include <errno.h>
> #include <pthread.h>
> #include <stdio.h>
>=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, add=
r_fix, errno, i );
>=20
> 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
@@ -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)
{
@@ -1160,7 +1176,7 @@ vm_map_find(vm_map_t map, vm_object_t object, vm_ooff=
set_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;
=20
start =3D *addr;
@@ -1171,9 +1187,14 @@ vm_map_find(vm_map_t map, vm_object_t object, vm_oof=
fset_t offset,
return (KERN_NO_SPACE);
}
start =3D *addr;
+ end =3D start + length;
+ } else {
+ 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, start + length, prot, max, cow);
+ start, end, prot, max, cow);
vm_map_unlock(map);
return (result);
}
@@ -1355,22 +1376,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_mmap.c b/sys/vm/vm_mmap.c
index 9522fec..a48fd77 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.
--2bJ57vwr75KGnr5s
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (FreeBSD)
iD8DBQFGtwgtC3+MBN1Mb4gRAqanAKCgdwEDv2eKL8hZLyck6V7XwhG+dwCgrWi6
rtOjbPxmRr7XfVOlXDTd4eQ=
=9J1w
-----END PGP SIGNATURE-----
--2bJ57vwr75KGnr5s--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070806113821.GB2738>