[Qemu-devel] [PATCH v3] linux-user: Remove extra mapping

[Qemu-devel] [PATCH v3] linux-user: Remove extra mapping

[Steve Mcpolin]

When a guest mmap()'d a file, an anonymous mapping was created to
align different host and client page granularity and provide for
beyond EOF mappings.  The file was then mapped ontop of this mapping,
releasing the memory reserved by it.  A file based mmap does not reserve
memory, but the anonymous map does; thus this anonymous mapping could
could cause spurious failures.

This patch minimizes the size of the anonymous mapping to prevent spurious
failures.

Signed-off-by: Steve Mcpolin <smcpolin@vmware.com>
---
 linux-user/mmap.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 9168a20..7a975c8 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -453,21 +453,29 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
         /* Note: we prefer to control the mapping address. It is
            especially important if qemu_host_page_size >
            qemu_real_host_page_size */
-        p = mmap(g2h(start), host_len, prot,
-                 flags | MAP_FIXED | MAP_ANONYMOUS, -1, 0);
-        if (p == MAP_FAILED)
-            goto fail;
-        /* update start so that it points to the file position at 'offset' */
-        host_start = (unsigned long)p;
-        if (!(flags & MAP_ANONYMOUS)) {
-            p = mmap(g2h(start), len, prot,
+        if (flags & MAP_ANONYMOUS) {
+            offset = 0;
+            host_offset = 0;
+            fd = -1;
+        }
+        p = mmap(g2h(start), len, prot,
                      flags | MAP_FIXED, fd, host_offset);
-            if (p == MAP_FAILED) {
-                munmap(g2h(start), host_len);
-                goto fail;
+        host_start = (uintptr_t)p;
+        if (p != MAP_FAILED && host_len > REAL_HOST_PAGE_ALIGN(len)) {
+            void *q;
+            q = mmap(g2h(start + len), host_len - REAL_HOST_PAGE_ALIGN(len),
+                     prot, MAP_FIXED | (flags & MAP_TYPE) | MAP_ANONYMOUS,
+                     -1, 0);
+            if (q == MAP_FAILED) {
+                p = MAP_FAILED;
             }
-            host_start += offset - host_offset;
         }
+        if (p == MAP_FAILED) {
+            munmap(g2h(start), host_len);
+            goto fail;
+        }
+        host_start += offset - host_offset;
+        /* update start so that it points to the file position at 'offset' */
         start = h2g(host_start);
     } else {
         if (start & ~TARGET_PAGE_MASK) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3] linux-user: Remove extra mapping

[Laurent Vivier]

Le 29/05/2018 à 20:15, Steve Mcpolin a écrit :
> When a guest mmap()'d a file, an anonymous mapping was created to
> align different host and client page granularity and provide for
> beyond EOF mappings.  The file was then mapped ontop of this mapping,
> releasing the memory reserved by it.  A file based mmap does not reserve
> memory, but the anonymous map does; thus this anonymous mapping could
> could cause spurious failures.
> 
> This patch minimizes the size of the anonymous mapping to prevent spurious
> failures.
> 
> Signed-off-by: Steve Mcpolin <smcpolin@vmware.com>
> ---

v3:
  add missing flags in the second mmap()

v2:
  use REAL_HOST_PAGE_ALIGN(len) instead of HOST_PAGE_ALIGN(len)


>  linux-user/mmap.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 9168a20..7a975c8 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -453,21 +453,29 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>          /* Note: we prefer to control the mapping address. It is
>             especially important if qemu_host_page_size >
>             qemu_real_host_page_size */
> -        p = mmap(g2h(start), host_len, prot,
> -                 flags | MAP_FIXED | MAP_ANONYMOUS, -1, 0);
> -        if (p == MAP_FAILED)
> -            goto fail;
> -        /* update start so that it points to the file position at 'offset' */
> -        host_start = (unsigned long)p;
> -        if (!(flags & MAP_ANONYMOUS)) {
> -            p = mmap(g2h(start), len, prot,
> +        if (flags & MAP_ANONYMOUS) {
> +            offset = 0;
> +            host_offset = 0;
> +            fd = -1;
> +        }
> +        p = mmap(g2h(start), len, prot,
>                       flags | MAP_FIXED, fd, host_offset);
> -            if (p == MAP_FAILED) {
> -                munmap(g2h(start), host_len);
> -                goto fail;
> +        host_start = (uintptr_t)p;
> +        if (p != MAP_FAILED && host_len > REAL_HOST_PAGE_ALIGN(len)) {
> +            void *q;
> +            q = mmap(g2h(start + len), host_len - REAL_HOST_PAGE_ALIGN(len),
> +                     prot, MAP_FIXED | (flags & MAP_TYPE) | MAP_ANONYMOUS,

Why do you restrict the flags with MAP_TYPE? Original code was not doing
that. Is this needed?

Thanks,
Laurent