mmap(2) MAP_FIXED isn't thread-safe (+testcase)
Kostik Belousov
kostikbel at gmail.com
Tue Aug 7 04:00:31 PDT 2007
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.
> >>
> >> 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;
> >> }
> >
[Old patch skipped].
> 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?
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 *arg);
#define PROC_VMSPACE_LOCK(p) do { } while (0)
#define PROC_VMSPACE_UNLOCK(p) do { } while (0)
+/*
+ * 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 = vm_map_min(map); \
+ if (end > vm_map_max(map)) \
+ end = vm_map_max(map); \
+ if (start > end) \
+ start = end; \
+ }
+
void
vm_map_startup(void)
{
@@ -1178,6 +1194,26 @@ vm_map_find(vm_map_t map, vm_object_t object, vm_ooffset_t offset,
return (result);
}
+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 = *addr;
+ vm_map_lock(map);
+ end = start + length;
+ VM_MAP_RANGE_CHECK(map, start, end);
+ (void) vm_map_delete(map, start, end);
+ result = 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)
}
/*
- * 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 = vm_map_min(map); \
- if (end > vm_map_max(map)) \
- end = vm_map_max(map); \
- if (start > end) \
- start = 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_size_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_size_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 size, vm_prot_t prot,
if (*addr != trunc_page(*addr))
return (EINVAL);
fitit = 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 size, vm_prot_t prot,
if (flags & MAP_STACK)
rv = vm_map_stack(map, *addr, size, prot, maxprot,
docow | MAP_STACK_GROWS_DOWN);
+ else if (fitit)
+ rv = vm_map_find(map, object, foff, addr, size, TRUE,
+ prot, maxprot, docow);
else
- rv = vm_map_find(map, object, foff, addr, size, fitit,
+ rv = vm_map_fixed(map, object, foff, addr, size,
prot, maxprot, docow);
if (rv != KERN_SUCCESS) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-current/attachments/20070807/87bc881a/attachment.pgp
More information about the freebsd-current
mailing list