* [PATCH v2 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag()
2021-09-22 4:56 [PATCH v2 0/9] bsd-user mmap fixes Warner Losh
@ 2021-09-22 4:56 ` Warner Losh
2021-09-23 17:35 ` Richard Henderson
2021-09-22 4:56 ` [PATCH v2 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; 28+ messages in thread
From: Warner Losh @ 2021-09-22 4:56 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>
---
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] 28+ messages in thread* Re: [PATCH v2 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag()
2021-09-22 4:56 ` [PATCH v2 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag() Warner Losh
@ 2021-09-23 17:35 ` Richard Henderson
0 siblings, 0 replies; 28+ messages in thread
From: Richard Henderson @ 2021-09-23 17:35 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, f4bug, Mikaël Urankar
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~
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE
2021-09-22 4:56 [PATCH v2 0/9] bsd-user mmap fixes Warner Losh
2021-09-22 4:56 ` [PATCH v2 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag() Warner Losh
@ 2021-09-22 4:56 ` Warner Losh
2021-09-23 17:36 ` Richard Henderson
2021-09-25 10:54 ` Philippe Mathieu-Daudé
2021-09-22 4:56 ` [PATCH v2 3/9] bsd-user/mmap.c: MAP_ symbols are defined, so no need for ifdefs Warner Losh
` (6 subsequent siblings)
8 siblings, 2 replies; 28+ messages in thread
From: Warner Losh @ 2021-09-22 4:56 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.
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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE
2021-09-22 4:56 ` [PATCH v2 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE Warner Losh
@ 2021-09-23 17:36 ` Richard Henderson
2021-09-24 15:07 ` Warner Losh
2021-09-25 10:54 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 28+ messages in thread
From: Richard Henderson @ 2021-09-23 17:36 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, f4bug, Mikaël Urankar
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~
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE
2021-09-23 17:36 ` Richard Henderson
@ 2021-09-24 15:07 ` Warner Losh
0 siblings, 0 replies; 28+ messages in thread
From: Warner Losh @ 2021-09-24 15:07 UTC (permalink / raw)
To: Richard Henderson
Cc: Kyle Evans, QEMU Developers, Mikaël Urankar,
Philippe Mathieu-Daudé
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE
2021-09-22 4:56 ` [PATCH v2 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE Warner Losh
2021-09-23 17:36 ` Richard Henderson
@ 2021-09-25 10:54 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-25 10:54 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, richard.henderson, Mikaël Urankar
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>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 3/9] bsd-user/mmap.c: MAP_ symbols are defined, so no need for ifdefs
2021-09-22 4:56 [PATCH v2 0/9] bsd-user mmap fixes Warner Losh
2021-09-22 4:56 ` [PATCH v2 1/9] bsd-user/mmap.c: Always zero MAP_ANONYMOUS memory in mmap_frag() Warner Losh
2021-09-22 4:56 ` [PATCH v2 2/9] bsd-user/mmap.c: check pread's return value to fix warnings with _FORTIFY_SOURCE Warner Losh
@ 2021-09-22 4:56 ` Warner Losh
2021-09-23 17:37 ` Richard Henderson
2021-09-22 4:56 ` [PATCH v2 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow Warner Losh
` (5 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Warner Losh @ 2021-09-22 4:56 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>
---
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
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v2 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow
2021-09-22 4:56 [PATCH v2 0/9] bsd-user mmap fixes Warner Losh
` (2 preceding siblings ...)
2021-09-22 4:56 ` [PATCH v2 3/9] bsd-user/mmap.c: MAP_ symbols are defined, so no need for ifdefs Warner Losh
@ 2021-09-22 4:56 ` Warner Losh
2021-09-23 17:38 ` Richard Henderson
2021-09-25 10:55 ` Philippe Mathieu-Daudé
2021-09-22 4:56 ` [PATCH v2 5/9] bsd-user/mmap.c: mmap prefer MAP_ANON for BSD Warner Losh
` (4 subsequent siblings)
8 siblings, 2 replies; 28+ messages in thread
From: Warner Losh @ 2021-09-22 4:56 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>
---
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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow
2021-09-22 4:56 ` [PATCH v2 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow Warner Losh
@ 2021-09-23 17:38 ` Richard Henderson
2021-09-25 10:55 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 28+ messages in thread
From: Richard Henderson @ 2021-09-23 17:38 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, f4bug
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~
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow
2021-09-22 4:56 ` [PATCH v2 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow Warner Losh
2021-09-23 17:38 ` Richard Henderson
@ 2021-09-25 10:55 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-25 10:55 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, richard.henderson
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>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 5/9] bsd-user/mmap.c: mmap prefer MAP_ANON for BSD
2021-09-22 4:56 [PATCH v2 0/9] bsd-user mmap fixes Warner Losh
` (3 preceding siblings ...)
2021-09-22 4:56 ` [PATCH v2 4/9] bsd-user/mmap.c: mmap return ENOMEM on overflow Warner Losh
@ 2021-09-22 4:56 ` Warner Losh
2021-09-23 17:38 ` Richard Henderson
2021-09-22 4:56 ` [PATCH v2 6/9] bsd-user/mmap.c: line wrap change Warner Losh
` (3 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Warner Losh @ 2021-09-22 4:56 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>
---
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
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v2 6/9] bsd-user/mmap.c: line wrap change
2021-09-22 4:56 [PATCH v2 0/9] bsd-user mmap fixes Warner Losh
` (4 preceding siblings ...)
2021-09-22 4:56 ` [PATCH v2 5/9] bsd-user/mmap.c: mmap prefer MAP_ANON for BSD Warner Losh
@ 2021-09-22 4:56 ` Warner Losh
2021-09-23 17:41 ` Richard Henderson
2021-09-22 4:56 ` [PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag Warner Losh
` (2 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Warner Losh @ 2021-09-22 4:56 UTC (permalink / raw)
To: qemu-devel; +Cc: kevans, richard.henderson, f4bug, 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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 6/9] bsd-user/mmap.c: line wrap change
2021-09-22 4:56 ` [PATCH v2 6/9] bsd-user/mmap.c: line wrap change Warner Losh
@ 2021-09-23 17:41 ` Richard Henderson
2021-09-26 16:46 ` Warner Losh
0 siblings, 1 reply; 28+ messages in thread
From: Richard Henderson @ 2021-09-23 17:41 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, f4bug
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~
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 6/9] bsd-user/mmap.c: line wrap change
2021-09-23 17:41 ` Richard Henderson
@ 2021-09-26 16:46 ` Warner Losh
0 siblings, 0 replies; 28+ messages in thread
From: Warner Losh @ 2021-09-26 16:46 UTC (permalink / raw)
To: Richard Henderson
Cc: Kyle Evans, QEMU Developers, Philippe Mathieu-Daudé
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag
2021-09-22 4:56 [PATCH v2 0/9] bsd-user mmap fixes Warner Losh
` (5 preceding siblings ...)
2021-09-22 4:56 ` [PATCH v2 6/9] bsd-user/mmap.c: line wrap change Warner Losh
@ 2021-09-22 4:56 ` Warner Losh
2021-09-23 17:43 ` Richard Henderson
2021-09-22 4:56 ` [PATCH v2 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head Warner Losh
2021-09-22 4:56 ` [PATCH v2 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail Warner Losh
8 siblings, 1 reply; 28+ messages in thread
From: Warner Losh @ 2021-09-22 4:56 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.
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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag
2021-09-22 4:56 ` [PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag Warner Losh
@ 2021-09-23 17:43 ` Richard Henderson
2021-09-26 17:08 ` Warner Losh
0 siblings, 1 reply; 28+ messages in thread
From: Richard Henderson @ 2021-09-23 17:43 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, Guy Yur, f4bug
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~
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag
2021-09-23 17:43 ` Richard Henderson
@ 2021-09-26 17:08 ` Warner Losh
2021-09-26 19:07 ` Guy Yur
0 siblings, 1 reply; 28+ messages in thread
From: Warner Losh @ 2021-09-26 17:08 UTC (permalink / raw)
To: Richard Henderson
Cc: Kyle Evans, Guy Yur, QEMU Developers, Philippe Mathieu-Daudé
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag
2021-09-26 17:08 ` Warner Losh
@ 2021-09-26 19:07 ` Guy Yur
2021-09-27 0:17 ` Warner Losh
0 siblings, 1 reply; 28+ messages in thread
From: Guy Yur @ 2021-09-26 19:07 UTC (permalink / raw)
To: Warner Losh, Richard Henderson
Cc: Kyle Evans, QEMU Developers, Philippe Mathieu-Daudé
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~
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag
2021-09-26 19:07 ` Guy Yur
@ 2021-09-27 0:17 ` Warner Losh
0 siblings, 0 replies; 28+ messages in thread
From: Warner Losh @ 2021-09-27 0:17 UTC (permalink / raw)
To: Guy Yur
Cc: Kyle Evans, Richard Henderson, QEMU Developers,
Philippe Mathieu-Daudé
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head
2021-09-22 4:56 [PATCH v2 0/9] bsd-user mmap fixes Warner Losh
` (6 preceding siblings ...)
2021-09-22 4:56 ` [PATCH v2 7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag Warner Losh
@ 2021-09-22 4:56 ` Warner Losh
2021-09-23 17:52 ` Richard Henderson
2021-09-22 4:56 ` [PATCH v2 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail Warner Losh
8 siblings, 1 reply; 28+ messages in thread
From: Warner Losh @ 2021-09-22 4:56 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.
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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head
2021-09-22 4:56 ` [PATCH v2 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head Warner Losh
@ 2021-09-23 17:52 ` Richard Henderson
2021-09-26 18:38 ` Warner Losh
0 siblings, 1 reply; 28+ messages in thread
From: Richard Henderson @ 2021-09-23 17:52 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, f4bug
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~
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head
2021-09-23 17:52 ` Richard Henderson
@ 2021-09-26 18:38 ` Warner Losh
0 siblings, 0 replies; 28+ messages in thread
From: Warner Losh @ 2021-09-26 18:38 UTC (permalink / raw)
To: Richard Henderson
Cc: Kyle Evans, QEMU Developers, Philippe Mathieu-Daudé
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail
2021-09-22 4:56 [PATCH v2 0/9] bsd-user mmap fixes Warner Losh
` (7 preceding siblings ...)
2021-09-22 4:56 ` [PATCH v2 8/9] bsd-user/mmap.c: Implement MAP_EXCL, required by jemalloc in head Warner Losh
@ 2021-09-22 4:56 ` Warner Losh
2021-09-23 17:53 ` Richard Henderson
2021-09-25 10:58 ` Philippe Mathieu-Daudé
8 siblings, 2 replies; 28+ messages in thread
From: Warner Losh @ 2021-09-22 4:56 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>
---
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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail
2021-09-22 4:56 ` [PATCH v2 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail Warner Losh
@ 2021-09-23 17:53 ` Richard Henderson
2021-09-25 10:58 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 28+ messages in thread
From: Richard Henderson @ 2021-09-23 17:53 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, f4bug, Mikaël Urankar
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~
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail
2021-09-22 4:56 ` [PATCH v2 9/9] bsd-user/mmap.c: assert that target_mprotect cannot fail Warner Losh
2021-09-23 17:53 ` Richard Henderson
@ 2021-09-25 10:58 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-25 10:58 UTC (permalink / raw)
To: Warner Losh, qemu-devel; +Cc: kevans, richard.henderson, Mikaël Urankar
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>
^ permalink raw reply [flat|nested] 28+ messages in thread