* [PATCH v3 0/9] bsd-user mmap fixes
@ 2021-10-08 21:23 Warner Losh
2021-10-08 21:23 ` [PATCH v3 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag() Warner Losh
` (8 more replies)
0 siblings, 9 replies; 22+ messages in thread
From: Warner Losh @ 2021-10-08 21:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kevans, richard.henderson, f4bug, 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. Updated to migrate debugging to qemu_log.
v3: reimplement the logging with qemu_log
redo MAP_EXCL based on review feedback
update fd == -1 MAP_ANON fix with description of what's going on
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: Convert to qemu_log logging for mmap debugging
bsd-user/mmap.c: assert that target_mprotect cannot fail
bsd-user/mmap.c | 146 ++++++++++++++++++++++++++----------------------
1 file changed, 79 insertions(+), 67 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag()
2021-10-08 21:23 [PATCH v3 0/9] bsd-user mmap fixes Warner Losh
@ 2021-10-08 21:23 ` Warner Losh
2021-10-14 15:05 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE Warner Losh
` (7 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Warner Losh @ 2021-10-08 21:23 UTC (permalink / raw)
To: qemu-devel
Cc: kevans, Mikaël Urankar, richard.henderson, f4bug, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE
2021-10-08 21:23 [PATCH v3 0/9] bsd-user mmap fixes Warner Losh
2021-10-08 21:23 ` [PATCH v3 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag() Warner Losh
@ 2021-10-08 21:23 ` Warner Losh
2021-10-14 15:06 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 3/9] bsd-user/mmap.c: MAP_ symbols are defined, so no need for ifdefs Warner Losh
` (6 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Warner Losh @ 2021-10-08 21:23 UTC (permalink / raw)
To: qemu-devel
Cc: kevans, Mikaël Urankar, richard.henderson, f4bug, 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. Update to current qemu standards with
{} around the if statement.
Signed-off-by: Mikaël Urankar <mikael.urankar@gmail.com>
Signed-off-by: Warner Losh <imp@bsdimp.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
bsd-user/mmap.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index fc3c1480f5..4f4fa3ab46 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -174,7 +174,9 @@ 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 +595,9 @@ 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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 3/9] bsd-user/mmap.c: MAP_ symbols are defined, so no need for ifdefs
2021-10-08 21:23 [PATCH v3 0/9] bsd-user mmap fixes Warner Losh
2021-10-08 21:23 ` [PATCH v3 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag() Warner Losh
2021-10-08 21:23 ` [PATCH v3 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE Warner Losh
@ 2021-10-08 21:23 ` Warner Losh
2021-10-14 15:06 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow Warner Losh
` (5 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Warner Losh @ 2021-10-08 21:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kevans, richard.henderson, f4bug, 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>
Acked-by: Richard Henderson <richard.henderson@linaro.org>
---
bsd-user/mmap.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index 4f4fa3ab46..6f33aec58b 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -286,13 +286,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) {
/*
@@ -407,22 +403,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 ");
}
@@ -432,11 +424,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
@@ -445,7 +435,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))) {
@@ -453,8 +442,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 */
@@ -462,7 +449,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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow
2021-10-08 21:23 [PATCH v3 0/9] bsd-user mmap fixes Warner Losh
` (2 preceding siblings ...)
2021-10-08 21:23 ` [PATCH v3 3/9] bsd-user/mmap.c: MAP_ symbols are defined, so no need for ifdefs Warner Losh
@ 2021-10-08 21:23 ` Warner Losh
2021-10-14 15:13 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 5/9] bsd-user/mmap.c: mmap prefer MAP_ANON for BSD Warner Losh
` (4 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Warner Losh @ 2021-10-08 21:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kevans, richard.henderson, f4bug, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
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 6f33aec58b..f0be3b12cf 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -455,11 +455,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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 5/9] bsd-user/mmap.c: mmap prefer MAP_ANON for BSD
2021-10-08 21:23 [PATCH v3 0/9] bsd-user mmap fixes Warner Losh
` (3 preceding siblings ...)
2021-10-08 21:23 ` [PATCH v3 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow Warner Losh
@ 2021-10-08 21:23 ` Warner Losh
2021-10-14 15:06 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 6/9] bsd-user/mmap.c: Convert to qemu_log logging for mmap debugging Warner Losh
` (3 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Warner Losh @ 2021-10-08 21:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kevans, richard.henderson, f4bug, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.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 f0be3b12cf..301108ed25 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -285,7 +285,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);
}
@@ -409,7 +409,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) {
@@ -431,7 +431,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;
}
@@ -533,7 +533,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' */
@@ -696,8 +696,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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 6/9] bsd-user/mmap.c: Convert to qemu_log logging for mmap debugging
2021-10-08 21:23 [PATCH v3 0/9] bsd-user mmap fixes Warner Losh
` (4 preceding siblings ...)
2021-10-08 21:23 ` [PATCH v3 5/9] bsd-user/mmap.c: mmap prefer MAP_ANON for BSD Warner Losh
@ 2021-10-08 21:23 ` Warner Losh
2021-10-09 16:01 ` Richard Henderson
` (2 more replies)
2021-10-08 21:23 ` [PATCH v3 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag Warner Losh
` (2 subsequent siblings)
8 siblings, 3 replies; 22+ messages in thread
From: Warner Losh @ 2021-10-08 21:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kevans, richard.henderson, f4bug, Warner Losh
Convert DEBUG_MMAP to qemu_log CPU_LOG_PAGE.
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
bsd-user/mmap.c | 53 +++++++++++++++++++++----------------------------
1 file changed, 23 insertions(+), 30 deletions(-)
diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index 301108ed25..face98573f 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -21,8 +21,6 @@
#include "qemu.h"
#include "qemu-common.h"
-//#define DEBUG_MMAP
-
static pthread_mutex_t mmap_mutex = PTHREAD_MUTEX_INITIALIZER;
static __thread int mmap_lock_count;
@@ -67,14 +65,11 @@ int target_mprotect(abi_ulong start, abi_ulong len, int prot)
abi_ulong end, host_start, host_end, addr;
int prot1, ret;
-#ifdef DEBUG_MMAP
- printf("mprotect: start=0x" TARGET_ABI_FMT_lx
- "len=0x" TARGET_ABI_FMT_lx " prot=%c%c%c\n", start, len,
- prot & PROT_READ ? 'r' : '-',
- prot & PROT_WRITE ? 'w' : '-',
- prot & PROT_EXEC ? 'x' : '-');
-#endif
-
+ qemu_log_mask(CPU_LOG_PAGE, "mprotect: start=0x" TARGET_ABI_FMT_lx
+ " len=0x" TARGET_ABI_FMT_lx " prot=%c%c%c\n", start, len,
+ prot & PROT_READ ? 'r' : '-',
+ prot & PROT_WRITE ? 'w' : '-',
+ prot & PROT_EXEC ? 'x' : '-');
if ((start & ~TARGET_PAGE_MASK) != 0)
return -EINVAL;
len = TARGET_PAGE_ALIGN(len);
@@ -391,45 +386,43 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
abi_ulong ret, end, real_start, real_end, retaddr, host_offset, host_len;
mmap_lock();
-#ifdef DEBUG_MMAP
- {
- printf("mmap: start=0x" TARGET_ABI_FMT_lx
- " len=0x" TARGET_ABI_FMT_lx " prot=%c%c%c flags=",
- start, len,
- prot & PROT_READ ? 'r' : '-',
- prot & PROT_WRITE ? 'w' : '-',
- prot & PROT_EXEC ? 'x' : '-');
+ if (qemu_loglevel_mask(CPU_LOG_PAGE)) {
+ qemu_log("mmap: start=0x" TARGET_ABI_FMT_lx
+ " len=0x" TARGET_ABI_FMT_lx " prot=%c%c%c flags=",
+ start, len,
+ prot & PROT_READ ? 'r' : '-',
+ prot & PROT_WRITE ? 'w' : '-',
+ prot & PROT_EXEC ? 'x' : '-');
if (flags & MAP_ALIGNMENT_MASK) {
- printf("MAP_ALIGNED(%u) ", (flags & MAP_ALIGNMENT_MASK)
- >> MAP_ALIGNMENT_SHIFT);
+ qemu_log("MAP_ALIGNED(%u) ",
+ (flags & MAP_ALIGNMENT_MASK) >> MAP_ALIGNMENT_SHIFT);
}
if (flags & MAP_GUARD) {
- printf("MAP_GUARD ");
+ qemu_log("MAP_GUARD ");
}
if (flags & MAP_FIXED) {
- printf("MAP_FIXED ");
+ qemu_log("MAP_FIXED ");
}
if (flags & MAP_ANON) {
- printf("MAP_ANON ");
+ qemu_log("MAP_ANON ");
}
if (flags & MAP_EXCL) {
- printf("MAP_EXCL ");
+ qemu_log("MAP_EXCL ");
}
if (flags & MAP_PRIVATE) {
- printf("MAP_PRIVATE ");
+ qemu_log("MAP_PRIVATE ");
}
if (flags & MAP_SHARED) {
- printf("MAP_SHARED ");
+ qemu_log("MAP_SHARED ");
}
if (flags & MAP_NOCORE) {
- printf("MAP_NOCORE ");
+ qemu_log("MAP_NOCORE ");
}
if (flags & MAP_STACK) {
- printf("MAP_STACK ");
+ qemu_log("MAP_STACK ");
}
- printf("fd=%d offset=0x%llx\n", fd, offset);
+ qemu_log("fd=%d offset=0x%lx\n", fd, offset);
}
-#endif
if ((flags & MAP_ANON) && fd != -1) {
errno = EINVAL;
--
2.32.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag
2021-10-08 21:23 [PATCH v3 0/9] bsd-user mmap fixes Warner Losh
` (5 preceding siblings ...)
2021-10-08 21:23 ` [PATCH v3 6/9] bsd-user/mmap.c: Convert to qemu_log logging for mmap debugging Warner Losh
@ 2021-10-08 21:23 ` Warner Losh
2021-10-09 16:03 ` Richard Henderson
2021-10-18 3:43 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head Warner Losh
2021-10-08 21:23 ` [PATCH v3 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail Warner Losh
8 siblings, 2 replies; 22+ messages in thread
From: Warner Losh @ 2021-10-08 21:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kevans, Guy Yur, richard.henderson, f4bug, 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. Add analysis from Guy Yur detailing the different cases
for MAP_GUARD and MAP_STACK.
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 | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index face98573f..4ecd949a10 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -127,7 +127,27 @@ error:
return ret;
}
-/* map an incomplete host page */
+/*
+ * map an incomplete host page
+ *
+ * mmap_frag can be called with a valid fd, if flags doesn't contain one of
+ * MAP_ANON, MAP_STACK, MAP_GUARD. If we need to map a page in those cases, we
+ * pass fd == -1. However, 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. See kern_mmap's enforcing of constraints for MAP_GUARD
+ * in sys/vm/vm_mmap.c.
+ * * 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. See enforcing of constraints for
+ * MAP_STACK in kern_mmap.
+ *
+ * Don't add MAP_ANON for the flags that use fd == -1 without specifying the
+ * flags directly, with the assumption that future flags that require fd == -1
+ * will also not require MAP_ANON.
+ */
static int mmap_frag(abi_ulong real_start,
abi_ulong start, abi_ulong end,
int prot, int flags, int fd, abi_ulong offset)
@@ -147,9 +167,9 @@ static int mmap_frag(abi_ulong real_start,
}
if (prot1 == 0) {
- /* no page was there, so we allocate one */
+ /* no page was there, so we allocate one. See also above. */
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;
@@ -157,7 +177,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 &&
@@ -565,7 +585,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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head
2021-10-08 21:23 [PATCH v3 0/9] bsd-user mmap fixes Warner Losh
` (6 preceding siblings ...)
2021-10-08 21:23 ` [PATCH v3 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag Warner Losh
@ 2021-10-08 21:23 ` Warner Losh
2021-10-09 16:04 ` Richard Henderson
2021-10-08 21:23 ` [PATCH v3 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail Warner Losh
8 siblings, 1 reply; 22+ messages in thread
From: Warner Losh @ 2021-10-08 21:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Kyle Evans, richard.henderson, f4bug, Warner Losh
From: Kyle Evans <kevans@FreeBSD.org>
jemalloc requires a working MAP_EXCL. Ensure that no page is double
mapped when specified. In addition, use guest_range_valid_untagged to
test for valid ranges of pages rather than an incomplete inlined version
of the test that might be wrong.
Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
bsd-user/mmap.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index 4ecd949a10..066d9c10ff 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -403,7 +403,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();
if (qemu_loglevel_mask(CPU_LOG_PAGE)) {
@@ -574,12 +574,10 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
* 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) {
+ if (!guest_range_valid_untagged(start, len)) {
errno = EINVAL;
goto fail;
}
-#endif
/*
* worst case: we cannot map the file because the offset is not
@@ -614,6 +612,12 @@ 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) && page_check_range(start, len, 0) < 0) {
+ errno = EINVAL;
+ goto fail;
+ }
+
/* handle the start of the mapping */
if (start > real_start) {
if (real_end == real_start + qemu_host_page_size) {
--
2.32.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail
2021-10-08 21:23 [PATCH v3 0/9] bsd-user mmap fixes Warner Losh
` (7 preceding siblings ...)
2021-10-08 21:23 ` [PATCH v3 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head Warner Losh
@ 2021-10-08 21:23 ` Warner Losh
2021-10-14 15:13 ` Kyle Evans
8 siblings, 1 reply; 22+ messages in thread
From: Warner Losh @ 2021-10-08 21:23 UTC (permalink / raw)
To: qemu-devel
Cc: kevans, Mikaël Urankar, richard.henderson, f4bug, 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
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 066d9c10ff..4586ad27d0 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -604,10 +604,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
}
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 6/9] bsd-user/mmap.c: Convert to qemu_log logging for mmap debugging
2021-10-08 21:23 ` [PATCH v3 6/9] bsd-user/mmap.c: Convert to qemu_log logging for mmap debugging Warner Losh
@ 2021-10-09 16:01 ` Richard Henderson
2021-10-11 18:52 ` Philippe Mathieu-Daudé
2021-10-18 3:44 ` Kyle Evans
2 siblings, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2021-10-09 16:01 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, f4bug
On 10/8/21 2:23 PM, Warner Losh wrote:
> Convert DEBUG_MMAP to qemu_log CPU_LOG_PAGE.
>
> Signed-off-by: Warner Losh<imp@bsdimp.com>
> ---
> bsd-user/mmap.c | 53 +++++++++++++++++++++----------------------------
> 1 file changed, 23 insertions(+), 30 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag
2021-10-08 21:23 ` [PATCH v3 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag Warner Losh
@ 2021-10-09 16:03 ` Richard Henderson
2021-10-18 3:43 ` Kyle Evans
1 sibling, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2021-10-09 16:03 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, Guy Yur, f4bug
On 10/8/21 2:23 PM, Warner Losh wrote:
> 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. Add analysis from Guy Yur detailing the different cases
> for MAP_GUARD and MAP_STACK.
>
> 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 | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head
2021-10-08 21:23 ` [PATCH v3 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head Warner Losh
@ 2021-10-09 16:04 ` Richard Henderson
0 siblings, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2021-10-09 16:04 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, f4bug
On 10/8/21 2:23 PM, Warner Losh wrote:
> From: Kyle Evans<kevans@FreeBSD.org>
>
> jemalloc requires a working MAP_EXCL. Ensure that no page is double
> mapped when specified. In addition, use guest_range_valid_untagged to
> test for valid ranges of pages rather than an incomplete inlined version
> of the test that might be wrong.
>
> Signed-off-by: Kyle Evans<kevans@FreeBSD.org>
> Signed-off-by: Warner Losh<imp@bsdimp.com>
> ---
> bsd-user/mmap.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 6/9] bsd-user/mmap.c: Convert to qemu_log logging for mmap debugging
2021-10-08 21:23 ` [PATCH v3 6/9] bsd-user/mmap.c: Convert to qemu_log logging for mmap debugging Warner Losh
2021-10-09 16:01 ` Richard Henderson
@ 2021-10-11 18:52 ` Philippe Mathieu-Daudé
2021-10-18 3:44 ` Kyle Evans
2 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-11 18:52 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, richard.henderson
On 10/8/21 23:23, Warner Losh wrote:
> Convert DEBUG_MMAP to qemu_log CPU_LOG_PAGE.
>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
> bsd-user/mmap.c | 53 +++++++++++++++++++++----------------------------
> 1 file changed, 23 insertions(+), 30 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag()
2021-10-08 21:23 ` [PATCH v3 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag() Warner Losh
@ 2021-10-14 15:05 ` Kyle Evans
0 siblings, 0 replies; 22+ messages in thread
From: Kyle Evans @ 2021-10-14 15:05 UTC (permalink / raw)
To: Warner Losh
Cc: Richard Henderson, QEMU Developers, Mikaël Urankar,
Philippe Mathieu-Daudé
On Fri, Oct 8, 2021 at 4:24 PM Warner Losh <imp@bsdimp.com> 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 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
>
Reviewed-by: Kyle Evans <kevans@FreeBSD.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE
2021-10-08 21:23 ` [PATCH v3 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE Warner Losh
@ 2021-10-14 15:06 ` Kyle Evans
0 siblings, 0 replies; 22+ messages in thread
From: Kyle Evans @ 2021-10-14 15:06 UTC (permalink / raw)
To: Warner Losh
Cc: Kyle Evans, Richard Henderson, QEMU Developers,
Mikaël Urankar, Philippe Mathieu-Daudé
On Fri, Oct 8, 2021 at 4:24 PM Warner Losh <imp@bsdimp.com> 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. Update to current qemu standards with
> {} around the if statement.
>
> Signed-off-by: Mikaël Urankar <mikael.urankar@gmail.com>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> bsd-user/mmap.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
> index fc3c1480f5..4f4fa3ab46 100644
> --- a/bsd-user/mmap.c
> +++ b/bsd-user/mmap.c
> @@ -174,7 +174,9 @@ 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 +595,9 @@ 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
>
Reviewed-by: Kyle Evans <kevans@FreeBSD.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 5/9] bsd-user/mmap.c: mmap prefer MAP_ANON for BSD
2021-10-08 21:23 ` [PATCH v3 5/9] bsd-user/mmap.c: mmap prefer MAP_ANON for BSD Warner Losh
@ 2021-10-14 15:06 ` Kyle Evans
0 siblings, 0 replies; 22+ messages in thread
From: Kyle Evans @ 2021-10-14 15:06 UTC (permalink / raw)
To: Warner Losh
Cc: Kyle Evans, Richard Henderson, QEMU Developers,
Philippe Mathieu-Daudé
On Fri, Oct 8, 2021 at 4:24 PM Warner Losh <imp@bsdimp.com> 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.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 f0be3b12cf..301108ed25 100644
> --- a/bsd-user/mmap.c
> +++ b/bsd-user/mmap.c
> @@ -285,7 +285,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);
> }
> @@ -409,7 +409,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) {
> @@ -431,7 +431,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;
> }
> @@ -533,7 +533,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' */
> @@ -696,8 +696,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
>
Reviewed-by: Kyle Evans <kevans@FreeBSD.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/9] bsd-user/mmap.c: MAP_ symbols are defined, so no need for ifdefs
2021-10-08 21:23 ` [PATCH v3 3/9] bsd-user/mmap.c: MAP_ symbols are defined, so no need for ifdefs Warner Losh
@ 2021-10-14 15:06 ` Kyle Evans
0 siblings, 0 replies; 22+ messages in thread
From: Kyle Evans @ 2021-10-14 15:06 UTC (permalink / raw)
To: Warner Losh
Cc: Kyle Evans, Richard Henderson, QEMU Developers,
Philippe Mathieu-Daudé
On Fri, Oct 8, 2021 at 4:24 PM Warner Losh <imp@bsdimp.com> 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>
> Acked-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> bsd-user/mmap.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
> diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
> index 4f4fa3ab46..6f33aec58b 100644
> --- a/bsd-user/mmap.c
> +++ b/bsd-user/mmap.c
> @@ -286,13 +286,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) {
> /*
> @@ -407,22 +403,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 ");
> }
> @@ -432,11 +424,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
> @@ -445,7 +435,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))) {
> @@ -453,8 +442,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 */
> @@ -462,7 +449,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
>
>
Reviewed-by: Kyle Evans <kevans@FreeBSD.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow
2021-10-08 21:23 ` [PATCH v3 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow Warner Losh
@ 2021-10-14 15:13 ` Kyle Evans
0 siblings, 0 replies; 22+ messages in thread
From: Kyle Evans @ 2021-10-14 15:13 UTC (permalink / raw)
To: Warner Losh
Cc: Kyle Evans, Richard Henderson, QEMU Developers,
Philippe Mathieu-Daudé
On Fri, Oct 8, 2021 at 4:27 PM Warner Losh <imp@bsdimp.com> 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> 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 6f33aec58b..f0be3b12cf 100644
> --- a/bsd-user/mmap.c
> +++ b/bsd-user/mmap.c
> @@ -455,11 +455,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
>
>
Reviewed-by: Kyle Evans <kevans@FreeBSD.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail
2021-10-08 21:23 ` [PATCH v3 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail Warner Losh
@ 2021-10-14 15:13 ` Kyle Evans
0 siblings, 0 replies; 22+ messages in thread
From: Kyle Evans @ 2021-10-14 15:13 UTC (permalink / raw)
To: Warner Losh
Cc: Kyle Evans, Richard Henderson, QEMU Developers,
Mikaël Urankar, Philippe Mathieu-Daudé
On Fri, Oct 8, 2021 at 4:31 PM Warner Losh <imp@bsdimp.com> 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> 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 066d9c10ff..4586ad27d0 100644
> --- a/bsd-user/mmap.c
> +++ b/bsd-user/mmap.c
> @@ -604,10 +604,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
> }
> 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
>
>
Reviewed-by: Kyle Evans <kevans@FreeBSD.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag
2021-10-08 21:23 ` [PATCH v3 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag Warner Losh
2021-10-09 16:03 ` Richard Henderson
@ 2021-10-18 3:43 ` Kyle Evans
1 sibling, 0 replies; 22+ messages in thread
From: Kyle Evans @ 2021-10-18 3:43 UTC (permalink / raw)
To: Warner Losh
Cc: Kyle Evans, Guy Yur, Richard Henderson, QEMU Developers,
Philippe Mathieu-Daudé
On Fri, Oct 8, 2021 at 4:29 PM Warner Losh <imp@bsdimp.com> wrote:
>
> 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. Add analysis from Guy Yur detailing the different cases
> for MAP_GUARD and MAP_STACK.
>
> 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 | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
> index face98573f..4ecd949a10 100644
> --- a/bsd-user/mmap.c
> +++ b/bsd-user/mmap.c
> @@ -127,7 +127,27 @@ error:
> return ret;
> }
>
> -/* map an incomplete host page */
> +/*
> + * map an incomplete host page
> + *
> + * mmap_frag can be called with a valid fd, if flags doesn't contain one of
> + * MAP_ANON, MAP_STACK, MAP_GUARD. If we need to map a page in those cases, we
> + * pass fd == -1. However, 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. See kern_mmap's enforcing of constraints for MAP_GUARD
> + * in sys/vm/vm_mmap.c.
> + * * 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. See enforcing of constraints for
> + * MAP_STACK in kern_mmap.
> + *
> + * Don't add MAP_ANON for the flags that use fd == -1 without specifying the
> + * flags directly, with the assumption that future flags that require fd == -1
> + * will also not require MAP_ANON.
> + */
> static int mmap_frag(abi_ulong real_start,
> abi_ulong start, abi_ulong end,
> int prot, int flags, int fd, abi_ulong offset)
> @@ -147,9 +167,9 @@ static int mmap_frag(abi_ulong real_start,
> }
>
> if (prot1 == 0) {
> - /* no page was there, so we allocate one */
> + /* no page was there, so we allocate one. See also above. */
> 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;
> @@ -157,7 +177,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 &&
> @@ -565,7 +585,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
>
Reviewed-by: Kyle Evans <kevans@FreeBSD.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 6/9] bsd-user/mmap.c: Convert to qemu_log logging for mmap debugging
2021-10-08 21:23 ` [PATCH v3 6/9] bsd-user/mmap.c: Convert to qemu_log logging for mmap debugging Warner Losh
2021-10-09 16:01 ` Richard Henderson
2021-10-11 18:52 ` Philippe Mathieu-Daudé
@ 2021-10-18 3:44 ` Kyle Evans
2 siblings, 0 replies; 22+ messages in thread
From: Kyle Evans @ 2021-10-18 3:44 UTC (permalink / raw)
To: Warner Losh
Cc: Kyle Evans, Richard Henderson, QEMU Developers,
Philippe Mathieu-Daudé
On Fri, Oct 8, 2021 at 4:25 PM Warner Losh <imp@bsdimp.com> wrote:
>
> Convert DEBUG_MMAP to qemu_log CPU_LOG_PAGE.
>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
> bsd-user/mmap.c | 53 +++++++++++++++++++++----------------------------
> 1 file changed, 23 insertions(+), 30 deletions(-)
>
> diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
> index 301108ed25..face98573f 100644
> --- a/bsd-user/mmap.c
> +++ b/bsd-user/mmap.c
> @@ -21,8 +21,6 @@
> #include "qemu.h"
> #include "qemu-common.h"
>
> -//#define DEBUG_MMAP
> -
> static pthread_mutex_t mmap_mutex = PTHREAD_MUTEX_INITIALIZER;
> static __thread int mmap_lock_count;
>
> @@ -67,14 +65,11 @@ int target_mprotect(abi_ulong start, abi_ulong len, int prot)
> abi_ulong end, host_start, host_end, addr;
> int prot1, ret;
>
> -#ifdef DEBUG_MMAP
> - printf("mprotect: start=0x" TARGET_ABI_FMT_lx
> - "len=0x" TARGET_ABI_FMT_lx " prot=%c%c%c\n", start, len,
> - prot & PROT_READ ? 'r' : '-',
> - prot & PROT_WRITE ? 'w' : '-',
> - prot & PROT_EXEC ? 'x' : '-');
> -#endif
> -
> + qemu_log_mask(CPU_LOG_PAGE, "mprotect: start=0x" TARGET_ABI_FMT_lx
> + " len=0x" TARGET_ABI_FMT_lx " prot=%c%c%c\n", start, len,
> + prot & PROT_READ ? 'r' : '-',
> + prot & PROT_WRITE ? 'w' : '-',
> + prot & PROT_EXEC ? 'x' : '-');
> if ((start & ~TARGET_PAGE_MASK) != 0)
> return -EINVAL;
> len = TARGET_PAGE_ALIGN(len);
> @@ -391,45 +386,43 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
> abi_ulong ret, end, real_start, real_end, retaddr, host_offset, host_len;
>
> mmap_lock();
> -#ifdef DEBUG_MMAP
> - {
> - printf("mmap: start=0x" TARGET_ABI_FMT_lx
> - " len=0x" TARGET_ABI_FMT_lx " prot=%c%c%c flags=",
> - start, len,
> - prot & PROT_READ ? 'r' : '-',
> - prot & PROT_WRITE ? 'w' : '-',
> - prot & PROT_EXEC ? 'x' : '-');
> + if (qemu_loglevel_mask(CPU_LOG_PAGE)) {
> + qemu_log("mmap: start=0x" TARGET_ABI_FMT_lx
> + " len=0x" TARGET_ABI_FMT_lx " prot=%c%c%c flags=",
> + start, len,
> + prot & PROT_READ ? 'r' : '-',
> + prot & PROT_WRITE ? 'w' : '-',
> + prot & PROT_EXEC ? 'x' : '-');
> if (flags & MAP_ALIGNMENT_MASK) {
> - printf("MAP_ALIGNED(%u) ", (flags & MAP_ALIGNMENT_MASK)
> - >> MAP_ALIGNMENT_SHIFT);
> + qemu_log("MAP_ALIGNED(%u) ",
> + (flags & MAP_ALIGNMENT_MASK) >> MAP_ALIGNMENT_SHIFT);
> }
> if (flags & MAP_GUARD) {
> - printf("MAP_GUARD ");
> + qemu_log("MAP_GUARD ");
> }
> if (flags & MAP_FIXED) {
> - printf("MAP_FIXED ");
> + qemu_log("MAP_FIXED ");
> }
> if (flags & MAP_ANON) {
> - printf("MAP_ANON ");
> + qemu_log("MAP_ANON ");
> }
> if (flags & MAP_EXCL) {
> - printf("MAP_EXCL ");
> + qemu_log("MAP_EXCL ");
> }
> if (flags & MAP_PRIVATE) {
> - printf("MAP_PRIVATE ");
> + qemu_log("MAP_PRIVATE ");
> }
> if (flags & MAP_SHARED) {
> - printf("MAP_SHARED ");
> + qemu_log("MAP_SHARED ");
> }
> if (flags & MAP_NOCORE) {
> - printf("MAP_NOCORE ");
> + qemu_log("MAP_NOCORE ");
> }
> if (flags & MAP_STACK) {
> - printf("MAP_STACK ");
> + qemu_log("MAP_STACK ");
> }
> - printf("fd=%d offset=0x%llx\n", fd, offset);
> + qemu_log("fd=%d offset=0x%lx\n", fd, offset);
> }
> -#endif
>
> if ((flags & MAP_ANON) && fd != -1) {
> errno = EINVAL;
> --
> 2.32.0
>
>
Reviewed-by: Kyle Evans <kevans@FreeBSD.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-10-18 4:54 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-08 21:23 [PATCH v3 0/9] bsd-user mmap fixes Warner Losh
2021-10-08 21:23 ` [PATCH v3 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag() Warner Losh
2021-10-14 15:05 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE Warner Losh
2021-10-14 15:06 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 3/9] bsd-user/mmap.c: MAP_ symbols are defined, so no need for ifdefs Warner Losh
2021-10-14 15:06 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow Warner Losh
2021-10-14 15:13 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 5/9] bsd-user/mmap.c: mmap prefer MAP_ANON for BSD Warner Losh
2021-10-14 15:06 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 6/9] bsd-user/mmap.c: Convert to qemu_log logging for mmap debugging Warner Losh
2021-10-09 16:01 ` Richard Henderson
2021-10-11 18:52 ` Philippe Mathieu-Daudé
2021-10-18 3:44 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag Warner Losh
2021-10-09 16:03 ` Richard Henderson
2021-10-18 3:43 ` Kyle Evans
2021-10-08 21:23 ` [PATCH v3 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head Warner Losh
2021-10-09 16:04 ` Richard Henderson
2021-10-08 21:23 ` [PATCH v3 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail Warner Losh
2021-10-14 15:13 ` Kyle Evans
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).