Re: [PATCH v4 3/3] csu: Implement and use _dl_early_allocate during static startup

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella:

>> diff --git a/scripts/tst-elf-edit.py b/scripts/tst-elf-edit.py
>> index a514179bbf..0e19ce1e73 100644
>> --- a/scripts/tst-elf-edit.py
>> +++ b/scripts/tst-elf-edit.py

>> @@ -172,24 +181,35 @@ def elf_edit(f, align):
>>  
>>      ehdr = Elf_Ehdr(e_ident)
>>      ehdr.read(f)
>> -    if ehdr.e_type != ET_DYN:
>> -       error('{}: not a shared library'.format(f.name))
>> +    if ehdr.e_type not in (ET_EXEC, ET_DYN):
>> +       error('{}: not an executable or shared library'.format(f.name))
>>  
>>      phdr = Elf_Phdr(e_ident)
>> +    maximize_tls_size_done = False
>>      for i in range(0, ehdr.e_phnum):
>>          f.seek(ehdr.e_phoff + i * phdr.len)
>>          phdr.read(f)
>> -        if phdr.p_type == PT_LOAD:
>> -            elf_edit_align(phdr, align)
>> +        if phdr.p_type == PT_LOAD and opts.align is not None:
>
> I think you can omit the None check.

I think this will raise an exception in elf_edit_align because the -a
argument is now optional.

>> +void *
>> +_dl_early_allocate (size_t size)
>> +{
>> +  void *result;
>> +
>> +  if (__curbrk != NULL)
>> +    /* If the break has been initialized, brk must have run before,
>> +       so just call it once more.  */
>> +    {
>> +      result = __sbrk (size);
>> +      if (result == (void *) -1)
>> +        result = NULL;
>> +    }
>> +  else
>> +    {
>> +      /* If brk has not been invoked, there is no need to update
>> +         __curbrk.  The first call to brk will take care of that.  */
>> +      void *previous = (void *) INTERNAL_SYSCALL_CALL (brk, 0);
>> +      result = (void *) INTERNAL_SYSCALL_CALL (brk, previous + size);
>> +      if (result == previous)
>> +        result = NULL;
>> +      else
>> +        result = previous;
>
> You will need to factor it be arch-specific since alpha return -ENOMEM
> in case of failure and sparc has different calling convention (similar 
> to what it does for clone).  Maybe add a
>
> static inline void *
> brk_call (void *addr)
> {
>   void *r = syscall;
>   r = check_error (r) ? -1 : 0;
> }
>
> And then refactor Linux brk.c versions to use brk_call as well.

Thanks.  I'm testing a new version with this change.

>> +  /* If brk fails, fall back to mmap.  This can happen due to
>> +     unfortunate ASLR layout decisions and kernel bugs, particularly
>> +     for static PIE.  */
>> +  if (result == NULL)
>> +    {
>> +      long int ret;
>> +      int prot = PROT_READ | PROT_WRITE;
>> +      int flags = MAP_PRIVATE | MAP_ANONYMOUS;
>> +#ifdef __NR_mmap2
>> +      ret = MMAP_CALL_INTERNAL (mmap2, 0, size, prot, flags, -1, 0);
>> +#else
>> +      ret = MMAP_CALL_INTERNAL (mmap, 0, size, prot, flags, -1, 0);
>> +#endif
>> +      if (INTERNAL_SYSCALL_ERROR_P (ret))
>
> Maybe move it to mmap_call.h and make it a static inline:
>
> static inline void *
> mmap64_call (void *addr, size_t len, int prot, int flags, int fd, 
> 	   off64_t offset)
> {
>   long int ret;
> #ifdef __NR_mmap2
>   ret = MMAP_CALL_INTERNAL (mmap2, addr, len, prot, flags, fd,
> 			    (off_t) (offset / MMAP2_PAGE_UNIT));
> #else
>   ret = MMAP_CALL_INTERNAL (mmap, addr, len, prot, flags, fd,
> 			    offset);
> #endif
>   return INTERNAL_SYSCALL_ERROR_P (ret) ? NULL : (void *) ret;
> }
>
> static inline void *
> mmap_call_internal (size_t len)
> {
>   int prot = PROT_READ | PROT_WRITE;
>   int flags = MAP_PRIVATE | MAP_ANONYMOUS;
>   return mmap64_call (0, len, PROT_READ | PROT_WRITE,
> 		      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> }

We have several customization points for mmap and mmap2.
MMAP2_PAGE_UNIT can expand to a static global variable page_unit.  This
is difficult to encapsulate properly in an inline function.  Adding
__brk_call was simple enough, but this looks way more complex.

Thanks,
Florian