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>

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);
-- 
2.20.1

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


-- 
Alex Bennée

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