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

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 09/05/2022 13:10, Florian Weimer wrote:
> * 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.

The idea is to allow __mmap64 use the mmap64_call and have the syscall
logic in one place.  Something like:

void *
__mmap64 (void *addr, size_t len, int prot, int flags, int fd, off64_t offset)
{
  MMAP_CHECK_PAGE_UNIT ();

  if (offset & MMAP_OFF_MASK)
    return (void *) INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);

  MMAP_PREPARE (addr, len, prot, flags, fd, offset);
  return mmap64_call (addr, len, prot, flags, fd, offset);
}