diff mbox series

[v7,13/14] linux-user: Adjust initial brk when interpreter is close to executable

Message ID 20230803015302.407219-14-richard.henderson@linaro.org
State Superseded
Headers show
Series linux-user: brk fixes | expand

Commit Message

Richard Henderson Aug. 3, 2023, 1:53 a.m. UTC 
From: Helge Deller <deller@gmx.de>

While we attempt to load a ET_DYN executable far away from
TASK_UNMAPPED_BASE, we are not completely in control of the
address space layout.  If the interpreter lands close to
the executable, leaving insufficient heap space, move brk.

Signed-off-by: Helge Deller <deller@gmx.de>
[rth: Re-order after ELF_ET_DYN_BASE patch so that we do not
 "temporarily break" tsan, and also to minimize the changes required.
 Remove image_info.reserve_brk as unused.]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/qemu.h    |  1 -
 linux-user/elfload.c | 51 +++++++++++++-------------------------------
 2 files changed, 15 insertions(+), 37 deletions(-)

Comments

Helge Deller Aug. 3, 2023, 1 p.m. UTC | #1 
Hi Richard,

Thanks for putting this all together!
I'll test asap.

I haven't checked yet, but Akihiko did send a revised v2 patch
series, while my v6 series included his older v1 patches.
We should consider his latest series...

One other thing below....

On 8/3/23 03:53, Richard Henderson wrote:
> From: Helge Deller <deller@gmx.de>
>
> While we attempt to load a ET_DYN executable far away from
> TASK_UNMAPPED_BASE, we are not completely in control of the
> address space layout.  If the interpreter lands close to
> the executable, leaving insufficient heap space, move brk.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> [rth: Re-order after ELF_ET_DYN_BASE patch so that we do not
>   "temporarily break" tsan, and also to minimize the changes required.
>   Remove image_info.reserve_brk as unused.]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/qemu.h    |  1 -
>   linux-user/elfload.c | 51 +++++++++++++-------------------------------
>   2 files changed, 15 insertions(+), 37 deletions(-)
>...
> @@ -3229,7 +3208,8 @@ static void load_elf_image(const char *image_name, int image_fd,
>       info->end_code = 0;
>       info->start_data = -1;
>       info->end_data = 0;
> -    info->brk = 0;
> +    /* Usual start for brk is after all sections of the main executable. */
> +    info->brk = TARGET_PAGE_ALIGN(hiaddr);

This is from my original patch, and is probably wrong.
I think this needs to be:
     info->brk = HOST_PAGE_ALIGN(hiaddr);

The brk page needs to be aligned to the host page size variable (which
is always >= target page size).
The page will be mapped +rw (on host), so may need the distance to code/shared
libs below it, and for that distance target-alignment may not be sufficient.

I think this fixes the problem which joel faced with armel static binary
on ppc64le.

Helge
diff mbox series

Patch

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 802794db63..4b0c9da0dc 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -31,7 +31,6 @@  struct image_info {
         abi_ulong       end_data;
         abi_ulong       start_brk;
         abi_ulong       brk;
-        abi_ulong       reserve_brk;
         abi_ulong       start_mmap;
         abi_ulong       start_stack;
         abi_ulong       stack_limit;
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index fef9a0bc8f..bf747a15b5 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -3120,27 +3120,6 @@  static void load_elf_image(const char *image_name, int image_fd,
         load_map_flags = 0;
     }
     if (pinterp_name != NULL) {
-        /*
-         * This is the main executable.
-         *
-         * Reserve extra space for brk.
-         * We hold on to this space while placing the interpreter
-         * and the stack, lest they be placed immediately after
-         * the data segment and block allocation from the brk.
-         *
-         * 16MB is chosen as "large enough" without being so large as
-         * to allow the result to not fit with a 32-bit guest on a
-         * 32-bit host. However some 64 bit guests (e.g. s390x)
-         * attempt to place their heap further ahead and currently
-         * nothing stops them smashing into QEMUs address space.
-         */
-#if TARGET_LONG_BITS == 64
-        info->reserve_brk = 32 * MiB;
-#else
-        info->reserve_brk = 16 * MiB;
-#endif
-        hiaddr += info->reserve_brk;
-
         if (ehdr->e_type == ET_EXEC) {
             /*
              * Make sure that the low address does not conflict with
@@ -3229,7 +3208,8 @@  static void load_elf_image(const char *image_name, int image_fd,
     info->end_code = 0;
     info->start_data = -1;
     info->end_data = 0;
-    info->brk = 0;
+    /* Usual start for brk is after all sections of the main executable. */
+    info->brk = TARGET_PAGE_ALIGN(hiaddr);
     info->elf_flags = ehdr->e_flags;
 
     prot_exec = PROT_EXEC;
@@ -3323,9 +3303,6 @@  static void load_elf_image(const char *image_name, int image_fd,
                     info->end_data = vaddr_ef;
                 }
             }
-            if (vaddr_em > info->brk) {
-                info->brk = vaddr_em;
-            }
 #ifdef TARGET_MIPS
         } else if (eppnt->p_type == PT_MIPS_ABIFLAGS) {
             Mips_elf_abiflags_v0 abiflags;
@@ -3654,6 +3631,19 @@  int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
     if (elf_interpreter) {
         load_elf_interp(elf_interpreter, &interp_info, bprm->buf);
 
+        /*
+         * While unusual because of ELF_ET_DYN_BASE, if we are unlucky
+         * with the mappings the interpreter can be loaded above but
+         * near the main executable, which can leave very little room
+         * for the heap.
+         * If the current brk has less than 16MB, use the end of the
+         * interpreter.
+         */
+        if (interp_info.brk > info->brk &&
+            interp_info.load_bias - info->brk < 16 * MiB)  {
+            info->brk = interp_info.brk;
+        }
+
         /* If the program interpreter is one of these two, then assume
            an iBCS2 image.  Otherwise assume a native linux image.  */
 
@@ -3707,17 +3697,6 @@  int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
     bprm->core_dump = &elf_core_dump;
 #endif
 
-    /*
-     * If we reserved extra space for brk, release it now.
-     * The implementation of do_brk in syscalls.c expects to be able
-     * to mmap pages in this space.
-     */
-    if (info->reserve_brk) {
-        abi_ulong start_brk = TARGET_PAGE_ALIGN(info->brk);
-        abi_ulong end_brk = TARGET_PAGE_ALIGN(info->brk + info->reserve_brk);
-        target_munmap(start_brk, end_brk - start_brk);
-    }
-
     return 0;
 }