| Message ID | 20200727122357.31263-9-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/16] shippable: add one more qemu to registry url | expand |
On Mon, 27 Jul 2020 at 13:24, Alex Bennée <alex.bennee@linaro.org> wrote: > > Plain MAP_FIXED has the undesirable behaviour of splatting exiting > maps so we don't actually achieve what we want when looking for gaps. > We should be using MAP_FIXED_NOREPLACE. As this isn't always available > we need to potentially check the returned address to see if the kernel > gave us what we asked for. > > Fixes: ad592e37dfc ("linux-user: provide fallback pgd_find_hole for bare chroots") > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Message-Id: <20200724064509.331-9-alex.bennee@linaro.org> Hi; Coverity thinks this conditional expression is suspicious (CID 1431059): > if (mmap_start != MAP_FAILED) { > munmap((void *) align_start, guest_size); > - return (uintptr_t) mmap_start + offset; > + if (MAP_FIXED_NOREPLACE || mmap_start == (void *) align_start) { because it's performing a logical OR operation where the left operand is an integer constant that's neither 0 nor 1 (it's 1048576). What was this intended to be? > + return (uintptr_t) mmap_start + offset; > + } > } thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 27 Jul 2020 at 13:24, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> Plain MAP_FIXED has the undesirable behaviour of splatting exiting >> maps so we don't actually achieve what we want when looking for gaps. >> We should be using MAP_FIXED_NOREPLACE. As this isn't always available >> we need to potentially check the returned address to see if the kernel >> gave us what we asked for. >> >> Fixes: ad592e37dfc ("linux-user: provide fallback pgd_find_hole for bare chroots") >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Message-Id: <20200724064509.331-9-alex.bennee@linaro.org> > > Hi; Coverity thinks this conditional expression is suspicious > (CID 1431059): > >> if (mmap_start != MAP_FAILED) { >> munmap((void *) align_start, guest_size); >> - return (uintptr_t) mmap_start + offset; >> + if (MAP_FIXED_NOREPLACE || mmap_start == (void *) align_start) { > > because it's performing a logical OR operation where the left > operand is an integer constant that's neither 0 nor 1 > (it's 1048576). What was this intended to be? It's 0 if the header doesn't provide it. If it's !0 we don't need to check the address because it should have been in the correct place. > >> + return (uintptr_t) mmap_start + offset; >> + } >> } > > thanks > -- PMM
On Tue, 28 Jul 2020 at 17:04, Alex Bennée <alex.bennee@linaro.org> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > > Hi; Coverity thinks this conditional expression is suspicious > > (CID 1431059): > > > >> if (mmap_start != MAP_FAILED) { > >> munmap((void *) align_start, guest_size); > >> - return (uintptr_t) mmap_start + offset; > >> + if (MAP_FIXED_NOREPLACE || mmap_start == (void *) align_start) { > > > > because it's performing a logical OR operation where the left > > operand is an integer constant that's neither 0 nor 1 > > (it's 1048576). What was this intended to be? > > It's 0 if the header doesn't provide it. If it's !0 we don't need to > check the address because it should have been in the correct place. OK. "if (MAP_FIXED_NOREPLACE != 0 || ...)" will probably satisfy Coverity then. -- PMM
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index 0b1298b3c91..20872e793e4 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -173,6 +173,9 @@ extern int daemon(int, int); #ifndef MAP_ANONYMOUS #define MAP_ANONYMOUS MAP_ANON #endif +#ifndef MAP_FIXED_NOREPLACE +#define MAP_FIXED_NOREPLACE 0 +#endif #ifndef ENOMEDIUM #define ENOMEDIUM ENODEV #endif diff --git a/linux-user/elfload.c b/linux-user/elfload.c index 7e7f642332d..fe9dfe795dd 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -2134,12 +2134,15 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk, /* we have run out of space */ return -1; } else { - int flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE | MAP_FIXED; + int flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE | + MAP_FIXED_NOREPLACE; void * mmap_start = mmap((void *) align_start, guest_size, PROT_NONE, flags, -1, 0); if (mmap_start != MAP_FAILED) { munmap((void *) align_start, guest_size); - return (uintptr_t) mmap_start + offset; + if (MAP_FIXED_NOREPLACE || mmap_start == (void *) align_start) { + return (uintptr_t) mmap_start + offset; + } } base += qemu_host_page_size; } @@ -2307,9 +2310,8 @@ static void pgb_reserved_va(const char *image_name, abi_ulong guest_loaddr, /* Widen the "image" to the entire reserved address space. */ pgb_static(image_name, 0, reserved_va, align); -#ifdef MAP_FIXED_NOREPLACE + /* osdep.h defines this as 0 if it's missing */ flags |= MAP_FIXED_NOREPLACE; -#endif /* Reserve the memory on the host. */ assert(guest_base != 0);
Plain MAP_FIXED has the undesirable behaviour of splatting exiting maps so we don't actually achieve what we want when looking for gaps. We should be using MAP_FIXED_NOREPLACE. As this isn't always available we need to potentially check the returned address to see if the kernel gave us what we asked for. Fixes: ad592e37dfc ("linux-user: provide fallback pgd_find_hole for bare chroots") Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-Id: <20200724064509.331-9-alex.bennee@linaro.org>