[PATCH v2 0/9] bsd-user mmap fixes

[PATCH v2 0/9] bsd-user mmap fixes

[Warner Losh]

This series synchronizes mmap.c with the bsd-user fork. This is a mix of old bug
fixes pulled in from linux-user, as well as some newer fixes to adress bugs
found in check-tcg and recent FreeBSD developments. There are also a couple of
style commits.

v2: do the cherry-picks from linux-user in qemu-style.

Guy Yur (1):
  bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag

Kyle Evans (1):
  bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head

Mikaël Urankar (2):
  bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag()
  bsd-user/mmap.c: check pread's return value to fix warnings with
    _FORTIFY_SOURCE

Warner Losh (5):
  bsd-user/mmap.c: MAP_ symbols are defined, so no need for ifdefs
  bsd-user/mmap.c: mmap return ENOMEM on overflow
  bsd-user/mmap.c: mmap prefer MAP_ANON for BSD
  bsd-user/mmap.c: line wrap change
  bsd-user/mmap.c: assert that target_mprotect cannot fail

 bsd-user/mmap.c | 69 +++++++++++++++++++++++++------------------------
 1 file changed, 35 insertions(+), 34 deletions(-)

-- 
2.32.0

[PATCH v2 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag()

[Warner Losh]

From: Mikaël Urankar <mikael.urankar@gmail.com>

Similar to the equivalent linux-user commit e6deac9cf99

When mapping MAP_ANONYMOUS memory fragments, still need notice about to
set it zero, or it will cause issues.

Signed-off-by: Mikaël Urankar <mikael.urankar@gmail.com>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/mmap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index b40ab9045f..fc3c1480f5 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -180,10 +180,12 @@ static int mmap_frag(abi_ulong real_start,
         if (prot_new != (prot1 | PROT_WRITE))
             mprotect(host_start, qemu_host_page_size, prot_new);
     } else {
-        /* just update the protection */
         if (prot_new != prot1) {
             mprotect(host_start, qemu_host_page_size, prot_new);
         }
+        if (prot_new & PROT_WRITE) {
+            memset(g2h_untagged(start), 0, end - start);
+        }
     }
     return 0;
 }
-- 
2.32.0

[PATCH v2 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE

[Warner Losh]

From: Mikaël Urankar <mikael.urankar@gmail.com>

Simmilar to the equivalent linux-user: commit fb7e378cf9c, which added
checking to pread's return value.

Signed-off-by: Mikaël Urankar <mikael.urankar@gmail.com>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/mmap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index fc3c1480f5..90b6313161 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -174,7 +174,8 @@ static int mmap_frag(abi_ulong real_start,
             mprotect(host_start, qemu_host_page_size, prot1 | PROT_WRITE);
 
         /* read the corresponding file data */
-        pread(fd, g2h_untagged(start), end - start, offset);
+        if (pread(fd, g2h_untagged(start), end - start, offset) == -1)
+            return -1;
 
         /* put final protection */
         if (prot_new != (prot1 | PROT_WRITE))
@@ -593,7 +594,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
                                   -1, 0);
             if (retaddr == -1)
                 goto fail;
-            pread(fd, g2h_untagged(start), len, offset);
+            if (pread(fd, g2h_untagged(start), len, offset) == -1)
+                goto fail;
             if (!(prot & PROT_WRITE)) {
                 ret = target_mprotect(start, len, prot);
                 if (ret != 0) {
-- 
2.32.0

[PATCH v2 3/9] bsd-user/mmap.c: MAP_ symbols are defined, so no need for ifdefs

[Warner Losh]

All these MAP_ symbols are always defined on supported FreeBSD versions
(12.2 and newer), so remove the #ifdefs since they aren't needed.

Signed-off-by: Warner Losh <imp@bsdimp.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 bsd-user/mmap.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index 90b6313161..c40059d7fc 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -285,13 +285,9 @@ static abi_ulong mmap_find_vma_aligned(abi_ulong start, abi_ulong size,
     wrapped = repeat = 0;
     prev = 0;
     flags = MAP_ANONYMOUS | MAP_PRIVATE;
-#ifdef MAP_ALIGNED
     if (alignment != 0) {
         flags |= MAP_ALIGNED(alignment);
     }
-#else
-    /* XXX TODO */
-#endif
 
     for (;; prev = ptr) {
         /*
@@ -406,22 +402,18 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
             printf("MAP_ALIGNED(%u) ", (flags & MAP_ALIGNMENT_MASK)
                     >> MAP_ALIGNMENT_SHIFT);
         }
-#if MAP_GUARD
         if (flags & MAP_GUARD) {
             printf("MAP_GUARD ");
         }
-#endif
         if (flags & MAP_FIXED) {
             printf("MAP_FIXED ");
         }
         if (flags & MAP_ANONYMOUS) {
             printf("MAP_ANON ");
         }
-#ifdef MAP_EXCL
         if (flags & MAP_EXCL) {
             printf("MAP_EXCL ");
         }
-#endif
         if (flags & MAP_PRIVATE) {
             printf("MAP_PRIVATE ");
         }
@@ -431,11 +423,9 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
         if (flags & MAP_NOCORE) {
             printf("MAP_NOCORE ");
         }
-#ifdef MAP_STACK
         if (flags & MAP_STACK) {
             printf("MAP_STACK ");
         }
-#endif
         printf("fd=%d offset=0x%llx\n", fd, offset);
     }
 #endif
@@ -444,7 +434,6 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
         errno = EINVAL;
         goto fail;
     }
-#ifdef MAP_STACK
     if (flags & MAP_STACK) {
         if ((fd != -1) || ((prot & (PROT_READ | PROT_WRITE)) !=
                     (PROT_READ | PROT_WRITE))) {
@@ -452,8 +441,6 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
             goto fail;
         }
     }
-#endif /* MAP_STACK */
-#ifdef MAP_GUARD
     if ((flags & MAP_GUARD) && (prot != PROT_NONE || fd != -1 ||
         offset != 0 || (flags & (MAP_SHARED | MAP_PRIVATE |
         /* MAP_PREFAULT | */ /* MAP_PREFAULT not in mman.h */
@@ -461,7 +448,6 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
         errno = EINVAL;
         goto fail;
     }
-#endif
 
     if (offset & ~TARGET_PAGE_MASK) {
         errno = EINVAL;
-- 
2.32.0

[PATCH v2 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow

[Warner Losh]

mmap should return ENOMEM on len overflow rather than EINVAL. Return
EINVAL when len == 0 and ENOMEM when the rounded to a page length is 0.
Found by make check-tcg.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/mmap.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index c40059d7fc..0acc2db712 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -454,11 +454,18 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
         goto fail;
     }
 
-    len = TARGET_PAGE_ALIGN(len);
     if (len == 0) {
         errno = EINVAL;
         goto fail;
     }
+
+    /* Check for overflows */
+    len = TARGET_PAGE_ALIGN(len);
+    if (len == 0) {
+        errno = ENOMEM;
+        goto fail;
+    }
+
     real_start = start & qemu_host_page_mask;
     host_offset = offset & qemu_host_page_mask;
 
-- 
2.32.0

[PATCH v2 5/9] bsd-user/mmap.c: mmap prefer MAP_ANON for BSD

[Warner Losh]

MAP_ANON and MAP_ANONYMOUS are identical. Prefer MAP_ANON for BSD since
the file is now a confusing mix of the two.

Signed-off-by: Warner Losh <imp@bsdimp.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 bsd-user/mmap.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index 0acc2db712..bafbdacd31 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -284,7 +284,7 @@ static abi_ulong mmap_find_vma_aligned(abi_ulong start, abi_ulong size,
     addr = start;
     wrapped = repeat = 0;
     prev = 0;
-    flags = MAP_ANONYMOUS | MAP_PRIVATE;
+    flags = MAP_ANON | MAP_PRIVATE;
     if (alignment != 0) {
         flags |= MAP_ALIGNED(alignment);
     }
@@ -408,7 +408,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
         if (flags & MAP_FIXED) {
             printf("MAP_FIXED ");
         }
-        if (flags & MAP_ANONYMOUS) {
+        if (flags & MAP_ANON) {
             printf("MAP_ANON ");
         }
         if (flags & MAP_EXCL) {
@@ -430,7 +430,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
     }
 #endif
 
-    if ((flags & MAP_ANONYMOUS) && fd != -1) {
+    if ((flags & MAP_ANON) && fd != -1) {
         errno = EINVAL;
         goto fail;
     }
@@ -532,7 +532,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
          * qemu_real_host_page_size
          */
         p = mmap(g2h_untagged(start), host_len, prot,
-                 flags | MAP_FIXED | ((fd != -1) ? MAP_ANONYMOUS : 0), -1, 0);
+                 flags | MAP_FIXED | ((fd != -1) ? MAP_ANON : 0), -1, 0);
         if (p == MAP_FAILED)
             goto fail;
         /* update start so that it points to the file position at 'offset' */
@@ -694,8 +694,7 @@ static void mmap_reserve(abi_ulong start, abi_ulong size)
     }
     if (real_start != real_end) {
         mmap(g2h_untagged(real_start), real_end - real_start, PROT_NONE,
-                 MAP_FIXED | MAP_ANONYMOUS | MAP_PRIVATE,
-                 -1, 0);
+                 MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
     }
 }
 
-- 
2.32.0

[PATCH v2 6/9] bsd-user/mmap.c: line wrap change

[Warner Losh]

Keep the shifted expression on one line. It's the same number of lines
and easier to read like this.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/mmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index bafbdacd31..8b763fffc3 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -399,8 +399,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
                prot & PROT_WRITE ? 'w' : '-',
                prot & PROT_EXEC ? 'x' : '-');
         if (flags & MAP_ALIGNMENT_MASK) {
-            printf("MAP_ALIGNED(%u) ", (flags & MAP_ALIGNMENT_MASK)
-                    >> MAP_ALIGNMENT_SHIFT);
+            printf("MAP_ALIGNED(%u) ",
+                   (flags & MAP_ALIGNMENT_MASK) >> MAP_ALIGNMENT_SHIFT);
         }
         if (flags & MAP_GUARD) {
             printf("MAP_GUARD ");
-- 
2.32.0

[PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag

[Warner Losh]

From: Guy Yur <guyyur@gmail.com>

Switch checks for !(flags & MAP_ANONYMOUS) with checks for fd != -1.
MAP_STACK and MAP_GUARD both require fd == -1 and don't require mapping
the fd either.

Signed-off-by: Guy Yur <guyyur@gmail.com>
[ partially merged before, finishing the job and documenting origin]
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/mmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index 8b763fffc3..347d314aa9 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -154,7 +154,7 @@ static int mmap_frag(abi_ulong real_start,
     if (prot1 == 0) {
         /* no page was there, so we allocate one */
         void *p = mmap(host_start, qemu_host_page_size, prot,
-                       flags | MAP_ANON, -1, 0);
+                       flags | ((fd != -1) ? MAP_ANON : 0), -1, 0);
         if (p == MAP_FAILED)
             return -1;
         prot1 = prot;
@@ -162,7 +162,7 @@ static int mmap_frag(abi_ulong real_start,
     prot1 &= PAGE_BITS;
 
     prot_new = prot | prot1;
-    if (!(flags & MAP_ANON)) {
+    if (fd != -1) {
         /* msync() won't work here, so we return an error if write is
            possible while it is a shared mapping */
         if ((flags & TARGET_BSD_MAP_FLAGMASK) == MAP_SHARED &&
@@ -571,7 +571,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
          * worst case: we cannot map the file because the offset is not
          * aligned, so we read it
          */
-        if (!(flags & MAP_ANON) &&
+        if (fd != -1 &&
             (offset & ~qemu_host_page_mask) != (start & ~qemu_host_page_mask)) {
             /*
              * msync() won't work here, so we return an error if write is
-- 
2.32.0

[PATCH v2 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head

[Warner Losh]

From: Kyle Evans <kevans@FreeBSD.org>

jemalloc requires a working MAP_EXCL. Ensure that no page is double
mapped when specified.

Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/mmap.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index 347d314aa9..792ff00548 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -387,7 +387,7 @@ abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size)
 abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
                      int flags, int fd, off_t offset)
 {
-    abi_ulong ret, end, real_start, real_end, retaddr, host_offset, host_len;
+    abi_ulong addr, ret, end, real_start, real_end, retaddr, host_offset, host_len;
 
     mmap_lock();
 #ifdef DEBUG_MMAP
@@ -599,6 +599,14 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
             goto the_end;
         }
 
+        /* Reject the mapping if any page within the range is mapped */
+        if (flags & MAP_EXCL) {
+            for (addr = start; addr < end; addr++) {
+                if (page_get_flags(addr) != 0)
+                    goto fail;
+            }
+        }
+
         /* handle the start of the mapping */
         if (start > real_start) {
             if (real_end == real_start + qemu_host_page_size) {
-- 
2.32.0

[PATCH v2 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail

[Warner Losh]

Similar to the equivalent linux-user change 86abac06c14. All error
conditions that target_mprotect checks are also checked by target_mmap.
EACCESS cannot happen because we are just removing PROT_WRITE.  ENOMEM
should not happen because we are modifying a whole VMA (and we have
bigger problems anyway if it happens).

Fixes a Coverity false positive, where Coverity complains about
target_mprotect's return value being passed to tb_invalidate_phys_range.

Signed-off-by: Mikaël Urankar <mikael.urankar@gmail.com>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/mmap.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index 792ff00548..4ddbd50b62 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -591,10 +591,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
                 goto fail;
             if (!(prot & PROT_WRITE)) {
                 ret = target_mprotect(start, len, prot);
-                if (ret != 0) {
-                    start = ret;
-                    goto the_end;
-                }
+                assert(ret == 0);
             }
             goto the_end;
         }
-- 
2.32.0

Re: [PATCH v2 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag()

[Richard Henderson]

On 9/21/21 9:56 PM, Warner Losh wrote:
> From: Mikaël Urankar<mikael.urankar@gmail.com>
> 
> Similar to the equivalent linux-user commit e6deac9cf99
> 
> When mapping MAP_ANONYMOUS memory fragments, still need notice about to
> set it zero, or it will cause issues.
> 
> Signed-off-by: Mikaël Urankar<mikael.urankar@gmail.com>
> Signed-off-by: Warner Losh<imp@bsdimp.com>
> ---
>   bsd-user/mmap.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE

[Richard Henderson]

On 9/21/21 9:56 PM, Warner Losh wrote:
> From: Mikaël Urankar <mikael.urankar@gmail.com>
> 
> Simmilar to the equivalent linux-user: commit fb7e378cf9c, which added
> checking to pread's return value.
> 
> Signed-off-by: Mikaël Urankar <mikael.urankar@gmail.com>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>   bsd-user/mmap.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

> -        pread(fd, g2h_untagged(start), end - start, offset);
> +        if (pread(fd, g2h_untagged(start), end - start, offset) == -1)
> +            return -1;

If it's not too annoying wrt rebasing other cleanups, please add the braces now.


r~

Re: [PATCH v2 3/9] bsd-user/mmap.c: MAP_ symbols are defined, so no need for ifdefs

[Richard Henderson]

On 9/21/21 9:56 PM, Warner Losh wrote:
> All these MAP_ symbols are always defined on supported FreeBSD versions
> (12.2 and newer), so remove the #ifdefs since they aren't needed.
> 
> Signed-off-by: Warner Losh<imp@bsdimp.com>
> Reviewed-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   bsd-user/mmap.c | 14 --------------
>   1 file changed, 14 deletions(-)

Acked-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow

[Richard Henderson]

On 9/21/21 9:56 PM, Warner Losh wrote:
> mmap should return ENOMEM on len overflow rather than EINVAL. Return
> EINVAL when len == 0 and ENOMEM when the rounded to a page length is 0.
> Found by make check-tcg.
> 
> Signed-off-by: Warner Losh<imp@bsdimp.com>
> ---
>   bsd-user/mmap.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 5/9] bsd-user/mmap.c: mmap prefer MAP_ANON for BSD

[Richard Henderson]

On 9/21/21 9:56 PM, Warner Losh wrote:
> MAP_ANON and MAP_ANONYMOUS are identical. Prefer MAP_ANON for BSD since
> the file is now a confusing mix of the two.
> 
> Signed-off-by: Warner Losh<imp@bsdimp.com>
> Reviewed-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   bsd-user/mmap.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 6/9] bsd-user/mmap.c: line wrap change

[Richard Henderson]

On 9/21/21 9:56 PM, Warner Losh wrote:
> Keep the shifted expression on one line. It's the same number of lines
> and easier to read like this.
> 
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>   bsd-user/mmap.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
> index bafbdacd31..8b763fffc3 100644
> --- a/bsd-user/mmap.c
> +++ b/bsd-user/mmap.c
> @@ -399,8 +399,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>                  prot & PROT_WRITE ? 'w' : '-',
>                  prot & PROT_EXEC ? 'x' : '-');
>           if (flags & MAP_ALIGNMENT_MASK) {
> -            printf("MAP_ALIGNED(%u) ", (flags & MAP_ALIGNMENT_MASK)
> -                    >> MAP_ALIGNMENT_SHIFT);
> +            printf("MAP_ALIGNED(%u) ",
> +                   (flags & MAP_ALIGNMENT_MASK) >> MAP_ALIGNMENT_SHIFT);
>           }
>           if (flags & MAP_GUARD) {
>               printf("MAP_GUARD ");
> 

I suppose.

If you're touching these lines at all it might be better to convert them all to qemu_log, 
protected by CPU_LOG_PAGE.  Then you can drop the ifdefs as well.


r~

Re: [PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag

[Richard Henderson]

On 9/21/21 9:56 PM, Warner Losh wrote:
>           /* no page was there, so we allocate one */
>           void *p = mmap(host_start, qemu_host_page_size, prot,
> -                       flags | MAP_ANON, -1, 0);
> +                       flags | ((fd != -1) ? MAP_ANON : 0), -1, 0);

I don't understand this change, given that the actual fd passed is always -1.


r~

Re: [PATCH v2 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head

[Richard Henderson]

On 9/21/21 9:56 PM, Warner Losh wrote:
> +        /* Reject the mapping if any page within the range is mapped */
> +        if (flags & MAP_EXCL) {
> +            for (addr = start; addr < end; addr++) {
> +                if (page_get_flags(addr) != 0)
> +                    goto fail;
> +            }
> +        }

How about

     if ((flags & MAP_EXCL) &&
         page_check_range(start, len, 0) < 0) {
        goto fail;
     }

Hmm.  This (and your page_get_flags check) could assert due to out-of-range guest address. 
  You're currently attempting that,

         /*
          * Test if requested memory area fits target address space
          * It can fail only on 64-bit host with 32-bit target.
          * On any other target/host host mmap() handles this error correctly.
          */
#if TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64
         if ((unsigned long)start + len - 1 > (abi_ulong) -1) {
             errno = EINVAL;
             goto fail;
         }
#endif

but the test isn't correct.  Note that reserved_va may be applied to 64-bit guests, and 
certainly may be smaller than (abi_ulong)-1.

You want guest_range_valid_untagged here.


r~

Re: [PATCH v2 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail

[Richard Henderson]

On 9/21/21 9:56 PM, Warner Losh wrote:
> Similar to the equivalent linux-user change 86abac06c14. All error
> conditions that target_mprotect checks are also checked by target_mmap.
> EACCESS cannot happen because we are just removing PROT_WRITE.  ENOMEM
> should not happen because we are modifying a whole VMA (and we have
> bigger problems anyway if it happens).
> 
> Fixes a Coverity false positive, where Coverity complains about
> target_mprotect's return value being passed to tb_invalidate_phys_range.
> 
> Signed-off-by: Mikaël Urankar<mikael.urankar@gmail.com>
> Signed-off-by: Warner Losh<imp@bsdimp.com>
> ---
>   bsd-user/mmap.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

On Fri, Sep 24, 2021 at 5:59 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 9/21/21 9:56 PM, Warner Losh wrote:
> > From: Mikaël Urankar <mikael.urankar@gmail.com>
> >
> > Simmilar to the equivalent linux-user: commit fb7e378cf9c, which added
> > checking to pread's return value.
> >
> > Signed-off-by: Mikaël Urankar <mikael.urankar@gmail.com>
> > Signed-off-by: Warner Losh <imp@bsdimp.com>
> > ---
> >   bsd-user/mmap.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> > -        pread(fd, g2h_untagged(start), end - start, offset);
> > +        if (pread(fd, g2h_untagged(start), end - start, offset) == -1)
> > +            return -1;
>
> If it's not too annoying wrt rebasing other cleanups, please add the
> braces now.
>

You bet.


>
> r~
>

[-- Attachment #2: Type: text/html, Size: 1735 bytes --]

Re: [PATCH v2 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE

[Philippe Mathieu-Daudé]

On 9/22/21 06:56, Warner Losh wrote:
> From: Mikaël Urankar <mikael.urankar@gmail.com>
> 
> Simmilar to the equivalent linux-user: commit fb7e378cf9c, which added
> checking to pread's return value.
> 
> Signed-off-by: Mikaël Urankar <mikael.urankar@gmail.com>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>   bsd-user/mmap.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
> index fc3c1480f5..90b6313161 100644
> --- a/bsd-user/mmap.c
> +++ b/bsd-user/mmap.c
> @@ -174,7 +174,8 @@ static int mmap_frag(abi_ulong real_start,
>               mprotect(host_start, qemu_host_page_size, prot1 | PROT_WRITE);
>   
>           /* read the corresponding file data */
> -        pread(fd, g2h_untagged(start), end - start, offset);
> +        if (pread(fd, g2h_untagged(start), end - start, offset) == -1)
> +            return -1;

Missing { } for QEMU style, otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow

[Philippe Mathieu-Daudé]

On 9/22/21 06:56, Warner Losh wrote:
> mmap should return ENOMEM on len overflow rather than EINVAL. Return
> EINVAL when len == 0 and ENOMEM when the rounded to a page length is 0.
> Found by make check-tcg.
> 
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>   bsd-user/mmap.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail

[Philippe Mathieu-Daudé]

On 9/22/21 06:56, Warner Losh wrote:
> Similar to the equivalent linux-user change 86abac06c14. All error
> conditions that target_mprotect checks are also checked by target_mmap.
> EACCESS cannot happen because we are just removing PROT_WRITE.  ENOMEM
> should not happen because we are modifying a whole VMA (and we have
> bigger problems anyway if it happens).
> 
> Fixes a Coverity false positive, where Coverity complains about
> target_mprotect's return value being passed to tb_invalidate_phys_range.
> 
> Signed-off-by: Mikaël Urankar <mikael.urankar@gmail.com>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>   bsd-user/mmap.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 6/9] bsd-user/mmap.c: line wrap change

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1526 bytes --]

On Fri, Sep 24, 2021 at 5:59 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 9/21/21 9:56 PM, Warner Losh wrote:
> > Keep the shifted expression on one line. It's the same number of lines
> > and easier to read like this.
> >
> > Signed-off-by: Warner Losh <imp@bsdimp.com>
> > ---
> >   bsd-user/mmap.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
> > index bafbdacd31..8b763fffc3 100644
> > --- a/bsd-user/mmap.c
> > +++ b/bsd-user/mmap.c
> > @@ -399,8 +399,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len,
> int prot,
> >                  prot & PROT_WRITE ? 'w' : '-',
> >                  prot & PROT_EXEC ? 'x' : '-');
> >           if (flags & MAP_ALIGNMENT_MASK) {
> > -            printf("MAP_ALIGNED(%u) ", (flags & MAP_ALIGNMENT_MASK)
> > -                    >> MAP_ALIGNMENT_SHIFT);
> > +            printf("MAP_ALIGNED(%u) ",
> > +                   (flags & MAP_ALIGNMENT_MASK) >> MAP_ALIGNMENT_SHIFT);
> >           }
> >           if (flags & MAP_GUARD) {
> >               printf("MAP_GUARD ");
> >
>
> I suppose.
>
> If you're touching these lines at all it might be better to convert them
> all to qemu_log,
> protected by CPU_LOG_PAGE.  Then you can drop the ifdefs as well.
>

I'll drop this patch and add one that does that. I was resistant to doing
that, so I
thought I'd give it a few days to mull over. I bit the bullet and saw how
trivial it
really is, so there's nothing really to mull :).

Warner

[-- Attachment #2: Type: text/html, Size: 2325 bytes --]

Re: [PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

On Fri, Sep 24, 2021 at 6:00 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 9/21/21 9:56 PM, Warner Losh wrote:
> >           /* no page was there, so we allocate one */
> >           void *p = mmap(host_start, qemu_host_page_size, prot,
> > -                       flags | MAP_ANON, -1, 0);
> > +                       flags | ((fd != -1) ? MAP_ANON : 0), -1, 0);
>
> I don't understand this change, given that the actual fd passed is always
> -1.
>

That's a very good question. I'll have to trace down why that was made
because
I'm having trouble with it as well now that I'm trying to defend it.

Warner


>
> r~
>

[-- Attachment #2: Type: text/html, Size: 1247 bytes --]

Re: [PATCH v2 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

On Fri, Sep 24, 2021 at 6:00 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 9/21/21 9:56 PM, Warner Losh wrote:
> > +        /* Reject the mapping if any page within the range is mapped */
> > +        if (flags & MAP_EXCL) {
> > +            for (addr = start; addr < end; addr++) {
> > +                if (page_get_flags(addr) != 0)
> > +                    goto fail;
> > +            }
> > +        }
>
> How about
>
>      if ((flags & MAP_EXCL) &&
>          page_check_range(start, len, 0) < 0) {
>         goto fail;
>      }
>
> Hmm.  This (and your page_get_flags check) could assert due to
> out-of-range guest address.
>   You're currently attempting that,
>
>          /*
>           * Test if requested memory area fits target address space
>           * It can fail only on 64-bit host with 32-bit target.
>           * On any other target/host host mmap() handles this error
> correctly.
>           */
> #if TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64
>          if ((unsigned long)start + len - 1 > (abi_ulong) -1) {
>              errno = EINVAL;
>              goto fail;
>          }
> #endif
>
> but the test isn't correct.  Note that reserved_va may be applied to
> 64-bit guests, and
> certainly may be smaller than (abi_ulong)-1.
>
> You want guest_range_valid_untagged here.
>

Great! Thanks for the tip!

Warner

[-- Attachment #2: Type: text/html, Size: 1999 bytes --]

Re: [PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag

[Guy Yur]

On 26/9/21 20:08, Warner Losh wrote:
>
>
> On Fri, Sep 24, 2021 at 6:00 AM Richard Henderson 
> <richard.henderson@linaro.org> wrote:
>
>     On 9/21/21 9:56 PM, Warner Losh wrote:
>     >           /* no page was there, so we allocate one */
>     >           void *p = mmap(host_start, qemu_host_page_size, prot,
>     > -                       flags | MAP_ANON, -1, 0);
>     > +                       flags | ((fd != -1) ? MAP_ANON : 0), -1, 0);
>
>     I don't understand this change, given that the actual fd passed is
>     always -1.
>
>
> That's a very good question. I'll have to trace down why that was made 
> because
> I'm having trouble with it as well now that I'm trying to defend it.
>
mmap_frag can be called with a valid fd, if flags doesn't contain one of 
MAP_ANON, MAP_STACK, MAP_GUARD.
The passed fd to mmap is -1 but if flags contains MAP_GUARD then 
MAP_ANON cannot be added.

* If fd is valid (not -1) we want to map the pages with MAP_ANON.
* If flags contains MAP_GUARD we don't want to add MAP_ANON because it 
will be rejected.
https://github.com/freebsd/freebsd-src/blob/master/sys/vm/vm_mmap.c#L302
* If flags contains MAP_ANON it doesn't matter if we add it or not.
* If flags contains MAP_STACK, mmap adds MAP_ANON when called so doesn't 
matter if we add it or not either.
https://github.com/freebsd/freebsd-src/blob/master/sys/vm/vm_mmap.c#L284

The intention was to not pass MAP_ANON for the flags that use fd == -1 
without specifying the flags directly,
with the assumption that future flags that don't require fd will also 
not require MAP_ANON.
Changing to !(flags & MAP_GUARD) will also work.

Guy Yur

> Warner
>
>
>     r~
>

Re: [PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 2072 bytes --]

On Sun, Sep 26, 2021 at 1:07 PM Guy Yur <guyyur@gmail.com> wrote:

> On 26/9/21 20:08, Warner Losh wrote:
> >
> >
> > On Fri, Sep 24, 2021 at 6:00 AM Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >
> >     On 9/21/21 9:56 PM, Warner Losh wrote:
> >     >           /* no page was there, so we allocate one */
> >     >           void *p = mmap(host_start, qemu_host_page_size, prot,
> >     > -                       flags | MAP_ANON, -1, 0);
> >     > +                       flags | ((fd != -1) ? MAP_ANON : 0), -1,
> 0);
> >
> >     I don't understand this change, given that the actual fd passed is
> >     always -1.
> >
> >
> > That's a very good question. I'll have to trace down why that was made
> > because
> > I'm having trouble with it as well now that I'm trying to defend it.
> >
> mmap_frag can be called with a valid fd, if flags doesn't contain one of
> MAP_ANON, MAP_STACK, MAP_GUARD.
> The passed fd to mmap is -1 but if flags contains MAP_GUARD then
> MAP_ANON cannot be added.
>
> * If fd is valid (not -1) we want to map the pages with MAP_ANON.
> * If flags contains MAP_GUARD we don't want to add MAP_ANON because it
> will be rejected.
> https://github.com/freebsd/freebsd-src/blob/master/sys/vm/vm_mmap.c#L302
> * If flags contains MAP_ANON it doesn't matter if we add it or not.
> * If flags contains MAP_STACK, mmap adds MAP_ANON when called so doesn't
> matter if we add it or not either.
> https://github.com/freebsd/freebsd-src/blob/master/sys/vm/vm_mmap.c#L284
>
> The intention was to not pass MAP_ANON for the flags that use fd == -1
> without specifying the flags directly,
> with the assumption that future flags that don't require fd will also
> not require MAP_ANON.
> Changing to !(flags & MAP_GUARD) will also work.
>

Thanks Guy. that fills in the missing pieces for me of the range of
possibilities
for calling. I've added it as a comment since it's tricky enough I've had
to ask
twice and Richard asked as well :). It will be in the next spin of the mmap
series.


> Guy Yur
>
> > Warner
> >
> >
> >     r~
> >
>

[-- Attachment #2: Type: text/html, Size: 3220 bytes --]