Re: [PATCH v6 7/8] linux-user: Optimize memory layout for static and dynamic executables

[Helge Deller]

On 8/2/23 20:25, Richard Henderson wrote:
> On 8/1/23 16:27, Helge Deller wrote:
> > Reorganize the guest memory layout to get as much memory as possible for
> > heap for the guest application.
> >
> > This patch optimizes the memory layout by loading pie executables
> > into lower memory and shared libs into higher memory (at
> > TASK_UNMAPPED_BASE). This leaves a bigger memory area usable for heap
> > space which will be located directly after the executable.
> > Up to now, pie executable and shared libs were loaded directly behind
> > each other in the area at TASK_UNMAPPED_BASE, which leaves very little
> > space for heap.
> >
> > I tested this patchset with chroots of alpha, arm, armel, arm64, hppa, m68k,
> > mips64el, mipsel, powerpc, ppc64, ppc64el, s390x, sh4 and sparc64 on a x86-64
> > host, and with a static armhf binary (which fails to run without this patch).
> >
> > This patch temporarily breaks the Thread Sanitizer (TSan) application
> > which expects specific boundary definitions for memory mappings on
> > different platforms [1], see commit aab613fb9597 ("linux-user: Update
> > TASK_UNMAPPED_BASE for aarch64") for aarch64. The follow-up patch fixes it
> > again.
> >
> > [1] 
> > https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/tsan/rtl/tsan_platform.h
> >
> > Signed-off-by: Helge Deller <deller@gmx.de>
> > ---
> >   linux-user/elfload.c | 55 +++++++++++++-------------------------------
> >   linux-user/mmap.c    |  8 ++++---
> >   2 files changed, 21 insertions(+), 42 deletions(-)
> >
> > diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> > index 2aee2298ec..47a118e430 100644
> > --- a/linux-user/elfload.c
> > +++ b/linux-user/elfload.c
> > @@ -3023,6 +3023,7 @@ static void load_elf_image(const char *image_name, int 
> > image_fd,
> >       abi_ulong load_addr, load_bias, loaddr, hiaddr, error;
> >       int i, retval, prot_exec;
> >       Error *err = NULL;
> > +    bool is_main_executable;
> >
> >       /* First of all, some simple consistency checks */
> >       if (!elf_check_ident(ehdr)) {
> > @@ -3106,28 +3107,8 @@ static void load_elf_image(const char *image_name, int 
> > image_fd,
> >           }
> >       }
> >
> > -    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;
> > -
> > +    is_main_executable = (pinterp_name != NULL);
>
> This will be false for static main executables.

[deller@p100 qemu-helge-user-armhf]$ file fstype
fstype: ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV), statically 
linked, interpreter /lib/klibc-PupSAGgtpafMlSLXOLgje1kXFo8.so, 
BuildID[sha1]=45aac32edcd204fd6fa06febf3abff274ab39b26, stripped

And for real static binaries (without interpreter), the "loadaddr" is set in 
the mmap below,
and MAP_FIX isn't needed then.

> > +    if (is_main_executable) {
> >           if (ehdr->e_type == ET_EXEC) {
> >               /*
> >                * Make sure that the low address does not conflict with
> > @@ -3136,7 +3117,7 @@ static void load_elf_image(const char *image_name, int 
> > image_fd,
> >               probe_guest_base(image_name, loaddr, hiaddr);
> >           } else {
> >               /*
> > -             * The binary is dynamic, but we still need to
> > +             * The binary is dynamic (pie-executabe), but we still need to
> >                * select guest_base.  In this case we pass a size.
> >                */
> >               probe_guest_base(image_name, 0, hiaddr - loaddr);
> > @@ -3159,7 +3140,7 @@ static void load_elf_image(const char *image_name, int 
> > image_fd,
> >        */
> >       load_addr = target_mmap(loaddr, (size_t)hiaddr - loaddr + 1, PROT_NONE,
> >                               MAP_PRIVATE | MAP_ANON | MAP_NORESERVE |
> > -                            (ehdr->e_type == ET_EXEC ? MAP_FIXED : 0),
> > +                            (is_main_executable ? MAP_FIXED : 0),
>
> This is definitely wrong, as all ET_EXEC require FIXED.

Not if the PIE flag is set too and even here the loadaddr defines where
it will be loaded then.

> (In addition to static, it is possible to write an ET_EXEC interpreter.)
>
> > --- a/linux-user/mmap.c
> > +++ b/linux-user/mmap.c
> > @@ -299,14 +299,16 @@ static bool mmap_frag(abi_ulong real_start, abi_ulong 
> > start, abi_ulong last,
> >   #ifdef TARGET_AARCH64
> >   # define TASK_UNMAPPED_BASE  0x5500000000
> >   #else
> > -# define TASK_UNMAPPED_BASE  (1ul << 38)
> > +# define TASK_UNMAPPED_BASE  0x4000000000
> >   #endif
> > -#else
> > +#elif HOST_LONG_BITS == 64 && TARGET_ABI_BITS == 32
> >   #ifdef TARGET_HPPA
> >   # define TASK_UNMAPPED_BASE  0xfa000000
> >   #else
> > -# define TASK_UNMAPPED_BASE  0x40000000
> > +# define TASK_UNMAPPED_BASE  0xe0000000
> >   #endif
> > +#else /* HOST_LONG_BITS == 32 && TARGET_ABI_BITS == 32 */
> > +# define TASK_UNMAPPED_BASE  0x40000000
> >   #endif
> >   abi_ulong mmap_next_start = TASK_UNMAPPED_BASE;
>
> This should be a separate change.
>
> While we're at it, we should move this to e.g.
> linux-user/$GUEST/target_mman.h, and make this match each guest
> kernel's TASK_UNMAPPED_BASE.  It really shouldn't depend on the host
> at all.

I think it matters if you want to run a 32-bit guest on 32-bit host?
Those TASK_UNMAPPED_BASE are actually TARGET_TASK_UNMAPPED_BASE values.

Helge