[Qemu-devel] [PATCH] qemu-user mmap strange behavior on multi-threaded applications

[Alexander Graf]

Hi,

when using multi-threading, the page table gets modified (I did not find
the place where that happened though). Because of this the internal
mmap() calculation (a static counter) does not work reliably, because it
tries to map regions, which are already mapped onto (at least when
taking a look at page_dump()).
Additionally two memory syscalls should not have to be executed
simultaneously, so I added a spin_lock on everything that smelled like
it could break something.
While trying to get mplayer running I found a check in mprotect to only
support PROT_READ, PROT_WRITE and PROT_EXEC, but mplayer tries to load a
library with PROT_GROWSDOWN. To get it running I just ignored the flag
and had no problems with that. I'm not sure weather this is always the case.

This patch is not meant to be included. It is a hacky workaround for
bugs I found on my way getting NPTL applications to work.

If anyone more experienced in this could take a look at the main
problem, I would really very much appreciate it. As soon as I trigger a
syscall in a new thread, the internal page table gets modified. I sent a
test case that triggers this problem ca. 2 weeks ago. I suspect this has
something to do with copy-on-write?


Regards,

Alexander Graf

Index: qemu-0.9.0/linux-user/syscall.c
===================================================================
--- qemu-0.9.0.orig/linux-user/syscall.c
+++ qemu-0.9.0/linux-user/syscall.c
@@ -185,6 +185,9 @@ extern int getresgid(gid_t *, gid_t *, g
 extern int setgroups(int, gid_t *);
 extern int uselib(const char*);
 
+#include "exec-all.h"
+long mmap_lock;
+
 static inline long get_errno(long ret)
 {
     if (ret == -1)
@@ -227,9 +235,11 @@ long do_brk(target_ulong new_brk)
 
     /* We need to allocate more memory after the brk... */
     new_alloc_size = HOST_PAGE_ALIGN(new_brk - brk_page + 1);
+    spin_lock(&mmap_lock);
     mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size, 
                                         PROT_READ|PROT_WRITE,
                                         MAP_ANON|MAP_FIXED|MAP_PRIVATE, 0, 0));
+    spin_unlock(&mmap_lock);
     if (is_error(mapped_addr)) {
        return mapped_addr;
     } else {
@@ -2133,6 +2133,7 @@ static long do_futex(target_ulong uaddr,
        struct timespec host_utime;
        unsigned long val2 = utime;
 
+       spin_lock(&mmap_lock);
        if (utime && (op == FUTEX_WAIT || op == FUTEX_LOCK_PI)) {
                target_to_host_timespec(&host_utime, utime);
                val2 = (unsigned long)&host_utime;
@@ -2230,6 +2265,7 @@ static long do_futex(target_ulong uaddr,
        }
 #endif
 #endif
+       spin_unlock(&mmap_lock);
        return syscall(__NR_futex, g2h(uaddr), op, val, val2, g2h(uaddr2), 
val3);
 }
 
@@ -2985,15 +3021,19 @@ long do_syscall(void *cpu_env, int num, 
             v5 = tswapl(v[4]);
             v6 = tswapl(v[5]);
             unlock_user(v, arg1, 0);
+           spin_lock(&mmap_lock);
             ret = get_errno(target_mmap(v1, v2, v3, 
                                         target_to_host_bitmask(v4, 
mmap_flags_tbl),
                                         v5, v6));
+           spin_unlock(&mmap_lock);
         }
 #else
+       spin_lock(&mmap_lock);
         ret = get_errno(target_mmap(arg1, arg2, arg3, 
                                     target_to_host_bitmask(arg4, 
mmap_flags_tbl), 
                                     arg5,
                                     arg6));
+       spin_unlock(&mmap_lock);
 #endif
         break;
 #ifdef TARGET_NR_mmap2
@@ -3003,36 +3043,54 @@ long do_syscall(void *cpu_env, int num, 
 #else
 #define MMAP_SHIFT TARGET_PAGE_BITS
 #endif
+       spin_lock(&mmap_lock);
         ret = get_errno(target_mmap(arg1, arg2, arg3, 
                                     target_to_host_bitmask(arg4, 
mmap_flags_tbl), 
                                     arg5,
                                     arg6 << MMAP_SHIFT));
+       spin_unlock(&mmap_lock);
         break;
 #endif
     case TARGET_NR_munmap:
+       spin_lock(&mmap_lock);
         ret = get_errno(target_munmap(arg1, arg2));
+       spin_unlock(&mmap_lock);
         break;
     case TARGET_NR_mprotect:
+       spin_lock(&mmap_lock);
         ret = get_errno(target_mprotect(arg1, arg2, arg3));
+       spin_unlock(&mmap_lock);
         break;
     case TARGET_NR_mremap:
+       spin_lock(&mmap_lock);
         ret = get_errno(target_mremap(arg1, arg2, arg3, arg4, arg5));
+       spin_unlock(&mmap_lock);
         break;
         /* ??? msync/mlock/munlock are broken for softmmu.  */
     case TARGET_NR_msync:
+       spin_lock(&mmap_lock);
         ret = get_errno(msync(g2h(arg1), arg2, arg3));
+       spin_unlock(&mmap_lock);
         break;
     case TARGET_NR_mlock:
+       spin_lock(&mmap_lock);
         ret = get_errno(mlock(g2h(arg1), arg2));
+       spin_unlock(&mmap_lock);
         break;
     case TARGET_NR_munlock:
+       spin_lock(&mmap_lock);
         ret = get_errno(munlock(g2h(arg1), arg2));
+       spin_unlock(&mmap_lock);
         break;
     case TARGET_NR_mlockall:
+       spin_lock(&mmap_lock);
         ret = get_errno(mlockall(arg1));
+       spin_unlock(&mmap_lock);
         break;
     case TARGET_NR_munlockall:
+       spin_lock(&mmap_lock);
         ret = get_errno(munlockall());
+       spin_unlock(&mmap_lock);
         break;
     case TARGET_NR_truncate:
         p = lock_user_string(arg1);
Index: qemu-0.9.0/exec.c
===================================================================
--- qemu-0.9.0.orig/exec.c
+++ qemu-0.9.0/exec.c
@@ -1676,6 +1684,50 @@ void page_dump(FILE *f)
     }
 }
 
+/* dump memory mappings */
+target_ulong page_find_end()
+{
+    unsigned long start, end;
+    int i, j, prot, prot1;
+    void *firsttb;
+    PageDesc *p;
+    target_ulong last = 0;
+
+    start = -1;
+    end = -1;
+    prot = 0;
+    for(i = 0; i <= L1_SIZE; i++) {
+        if (i < L1_SIZE)
+            p = l1_map[i];
+        else
+            p = NULL;
+        for(j = 0;j < L2_SIZE; j++) {
+            if (!p) {
+               firsttb = NULL;
+                prot1 = 0;
+           }
+            else {
+                prot1 = p[j].flags;
+               firsttb = p[j].first_tb;
+           }
+            if (prot1 != prot) {
+                end = (i << (32 - L1_BITS)) | (j << TARGET_PAGE_BITS);
+                if (start != -1) {
+                   last = end;
+                }
+                if (prot1 != 0)
+                    start = end;
+                else
+                    start = -1;
+                prot = prot1;
+            }
+            if (!p)
+                break;
+        }
+    }
+    return last;
+}
+
 int page_get_flags(target_ulong address)
 {
     PageDesc *p;
Index: qemu-0.9.0/linux-user/mmap.c
===================================================================
--- qemu-0.9.0.orig/linux-user/mmap.c
+++ qemu-0.9.0/linux-user/mmap.c
@@ -48,8 +48,14 @@ int target_mprotect(target_ulong start, 
     end = start + len;
     if (end < start)
         return -EINVAL;
-    if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC))
-        return -EINVAL;
+    if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC)) {
+#ifdef DEBUG_MMAP
+       gemu_log("mprotect: ERROR (prot & ~(PROT_READ | PROT_WRITE | 
PROT_EXEC)\n");
+#endif
+       // dirty hack to get mplayer running (sets PROT_GROWSDOWN) we just 
ignore advanced flags
+       prot &= (PROT_READ | PROT_WRITE | PROT_EXEC);
+//        return -EINVAL;
+    }
     if (len == 0)
         return 0;
     
@@ -205,9 +233,23 @@ long target_mmap(target_ulong start, tar
     defined(__ia64) || defined(__CYGWIN__)
         /* tell the kenel to search at the same place as i386 */
         if (real_start == 0) {
-            real_start = last_start;
-            last_start += HOST_PAGE_ALIGN(len);
+           target_ulong curend = page_find_end();
+
+           if(curend > last_start) {
+#ifdef DEBUG_MMAP
+               gemu_log("qemu: set last_start from %p to %p\n", last_start, 
curend + HOST_PAGE_ALIGN(len)); fflush(stdout); fflush(stderr);
+#endif
+               last_start = curend;
+#ifdef DEBUG_MMAP
+           } else {
+               gemu_log("qemu: curend(%p) <= last_start(%p)\n", curend, 
last_start); fflush(stdout); fflush(stderr);
+#endif
+           }
+           
+           real_start = last_start;
+           last_start += HOST_PAGE_ALIGN(len);
         }
+
 #endif
         if (0 && qemu_host_page_size != qemu_real_host_page_size) {
             /* NOTE: this code is only for debugging with '-p' option */