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

[Helge Deller <deller@gmx.de>]

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