Re: [PATCH 02/13] elf: switch _dl_map_segment() to anonymous mapping

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

On 18/03/23 13:50, Stas Sergeev via Libc-alpha wrote:
> _dl_map_segment() was mapping entire file image and then was skipping
> the load of the first segment. Switch _dl_map_segment() to anonymous
> mapping and do not skip the map of the first segment.
> 
> Use PROT_READ|PROT_WRITE as a protection. _dl_map_segments() later
> sets the proper protection for both file-mapped and anonymous parts.
> 
> The test-suite was run on x86_64/64 and showed no regressions.
> 
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> ---
>  elf/dl-map-segments.h | 73 +++++++++++++++++++++++--------------------
>  1 file changed, 39 insertions(+), 34 deletions(-)
> 
> diff --git a/elf/dl-map-segments.h b/elf/dl-map-segments.h
> index 504cfc0a41..9af8cae188 100644
> --- a/elf/dl-map-segments.h
> +++ b/elf/dl-map-segments.h
> @@ -22,18 +22,26 @@
>  /* Map a segment and align it properly.  */
>  
>  static __always_inline ElfW(Addr)
> -_dl_map_segment (const struct loadcmd *c, ElfW(Addr) mappref,
> -		 const size_t maplength, int fd)
> +_dl_map_segment (ElfW(Addr) mappref, size_t maplength, size_t mapalign)
>  {
> -  if (__glibc_likely (c->mapalign <= GLRO(dl_pagesize)))
> -    return (ElfW(Addr)) __mmap ((void *) mappref, maplength, c->prot,
> -				MAP_COPY|MAP_FILE, fd, c->mapoff);
> +  int err;
> +  unsigned map_flags = MAP_ANONYMOUS | MAP_PRIVATE;
> +  unsigned prot = PROT_READ | PROT_WRITE;

glibc code guidelines [1] suggest to explicit define the types, so
'unsigned int' here.

[1] https://sourceware.org/glibc/wiki/Style_and_Conventions

> +
> +#ifdef MAP_DENYWRITE
> +  /* Tell mmap() that we are mapping solib. This flag enables things
> +     like LD_PREFER_MAP_32BIT_EXEC. */
> +  map_flags |= MAP_DENYWRITE;
> +#endif

Why do you need o add MAP_DENYWRITE? They are complete ignored by Linux,
as stated in:

include/linux/mman.h
 31 /*
 32  * The historical set of flags that all mmap implementations implicitly
 33  * support when a ->mmap_validate() op is not provided in file_operations.
 34  *
 35  * MAP_EXECUTABLE and MAP_DENYWRITE are completely ignored throughout the
 36  * kernel.
 37  */
 38 #define LEGACY_MAP_MASK (MAP_SHARED \

(if you grep in Linux source code you will see there are only defined for
historical/compatibility reasons, there is indeed no code that actually uses
it).

> +  if (__glibc_likely (mapalign <= GLRO(dl_pagesize)))
> +    return (ElfW(Addr)) __mmap ((void *) mappref, maplength, prot,
> +				map_flags, -1, 0);
>  
>    /* If the segment alignment > the page size, alocate enough space to
>       ensure that the segment can be properly aligned.  */
> -  ElfW(Addr) maplen = (maplength >= c->mapalign
> -		       ? (maplength + c->mapalign)
> -		       : (2 * c->mapalign));
> +  ElfW(Addr) maplen = (maplength >= mapalign
> +		       ? (maplength + mapalign)
> +		       : (2 * mapalign));
>    ElfW(Addr) map_start = (ElfW(Addr)) __mmap ((void *) mappref, maplen,
>  					      PROT_NONE,
>  					      MAP_ANONYMOUS|MAP_PRIVATE,
> @@ -41,26 +49,24 @@ _dl_map_segment (const struct loadcmd *c, ElfW(Addr) mappref,
>    if (__glibc_unlikely ((void *) map_start == MAP_FAILED))
>      return map_start;
>  
> -  ElfW(Addr) map_start_aligned = ALIGN_UP (map_start, c->mapalign);
> -  map_start_aligned = (ElfW(Addr)) __mmap ((void *) map_start_aligned,
> -					   maplength, c->prot,
> -					   MAP_COPY|MAP_FILE|MAP_FIXED,
> -					   fd, c->mapoff);
> -  if (__glibc_unlikely ((void *) map_start_aligned == MAP_FAILED))
> -    __munmap ((void *) map_start, maplen);
> -  else
> +  ElfW(Addr) map_start_aligned = ALIGN_UP (map_start, mapalign);
> +  err = __mprotect ((void *) map_start_aligned, maplength, prot);
> +  if (__glibc_unlikely (err))
>      {
> -      /* Unmap the unused regions.  */
> -      ElfW(Addr) delta = map_start_aligned - map_start;
> -      if (delta)
> -	__munmap ((void *) map_start, delta);
> -      ElfW(Addr) map_end = map_start_aligned + maplength;
> -      map_end = ALIGN_UP (map_end, GLRO(dl_pagesize));
> -      delta = map_start + maplen - map_end;
> -      if (delta)
> -	__munmap ((void *) map_end, delta);
> +      __munmap ((void *) map_start, maplen);
> +      return (ElfW(Addr)) MAP_FAILED;
>      }
>  
> +  /* Unmap the unused regions.  */
> +  ElfW(Addr) delta = map_start_aligned - map_start;
> +  if (delta)
> +    __munmap ((void *) map_start, delta);
> +  ElfW(Addr) map_end = map_start_aligned + maplength;
> +  map_end = ALIGN_UP (map_end, GLRO(dl_pagesize));
> +  delta = map_start + maplen - map_end;
> +  if (delta)
> +    __munmap ((void *) map_end, delta);
> +
>    return map_start_aligned;
>  }
>  

So basically it would add another mmap on program loading.  For instance, loading
a simple empty main programs:

* Before:

  openat(AT_FDCWD, "./main", O_RDONLY|O_CLOEXEC) = 3
  mmap(NULL, 16408, PROT_READ, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7f9d922a9000
  mmap(0x7f9d922aa000, 4096, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1000) = 0x7f9d922aa000
  mmap(0x7f9d922ab000, 4096, PROT_READ, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x2000) = 0x7f9d922ab000
  mmap(0x7f9d922ac000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x2000) = 0x7f9d922ac000
  [...]
  openat(AT_FDCWD, "/home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
  mmap(NULL, 2080624, PROT_READ, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7f9d920ab000
  mprotect(0x7f9d920d1000, 1847296, PROT_NONE) = 0
  mmap(0x7f9d920d1000, 1490944, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x26000) = 0x7f9d920d1000
  mmap(0x7f9d9223d000, 352256, PROT_READ, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x192000) = 0x7f9d9223d000
  mmap(0x7f9d92294000, 24576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1e8000) = 0x7f9d92294000
  mmap(0x7f9d9229a000, 53104, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7f9d9229a000

* With this patch:

  openat(AT_FDCWD, "./main", O_RDONLY|O_CLOEXEC) = 3
  mmap(NULL, 16408, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_DENYWRITE, -1, 0) = 0x7f705da76000
  mmap(0x7f705da76000, 4096, PROT_READ, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0) = 0x7f705da76000
  mmap(0x7f705da77000, 4096, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1000) = 0x7f705da77000
  mmap(0x7f705da78000, 4096, PROT_READ, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x2000) = 0x7f705da78000
  mmap(0x7f705da79000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x2000) = 0x7f705da79000
  [...]
  openat(AT_FDCWD, "/home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
  mmap(NULL, 2080624, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_DENYWRITE, -1, 0) = 0x7f705d878000
  mprotect(0x7f705d89e000, 1847296, PROT_NONE) = 0
  mmap(0x7f705d878000, 155648, PROT_READ, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0) = 0x7f705d878000
  mmap(0x7f705d89e000, 1490944, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x26000) = 0x7f705d89e000
  mmap(0x7f705da0a000, 352256, PROT_READ, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x192000) = 0x7f705da0a000
  mmap(0x7f705da61000, 24576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1e8000) = 0x7f705da61000
  mmap(0x7f705da67000, 53104, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7f705da67000

And it also slight change the mapping, using the same program:

* Before:

      0x7ffff7dc2000     0x7ffff7de8000    0x26000        0x0  r--p   /home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so
      0x7ffff7de8000     0x7ffff7f54000   0x16c000    0x26000  r-xp   /home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so
      0x7ffff7f54000     0x7ffff7faa000    0x56000   0x192000  r--p   /home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so
      0x7ffff7faa000     0x7ffff7fab000     0x1000   0x1e8000  ---p   /home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so
      0x7ffff7fab000     0x7ffff7faf000     0x4000   0x1e8000  r--p   /home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so
      0x7ffff7faf000     0x7ffff7fb1000     0x2000   0x1ec000  rw-p   /home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so

* With this patch:

      0x7ffff7dc1000     0x7ffff7de7000    0x26000        0x0  r--p   /home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so
      0x7ffff7de7000     0x7ffff7f53000   0x16c000    0x26000  r-xp   /home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so
      0x7ffff7f53000     0x7ffff7fa9000    0x56000   0x192000  r--p   /home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so
      0x7ffff7fa9000     0x7ffff7faa000     0x1000        0x0  ---p
      0x7ffff7faa000     0x7ffff7fae000     0x4000   0x1e8000  r--p   /home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so
      0x7ffff7fae000     0x7ffff7fb0000     0x2000   0x1ec000  rw-p   /home/azanella/Projects/glibc/build/x86_64-linux-gnu/libc.so

So I am not seeing any advantage of this refactor: it slight increase the
number of syscalls for library loading and changes the 'debuggability' of
resulting shared library maps.

> @@ -98,7 +104,7 @@ _dl_map_segments (struct link_map *l, int fd,
>             - MAP_BASE_ADDR (l));
>  
>        /* Remember which part of the address space this object uses.  */
> -      l->l_map_start = _dl_map_segment (c, mappref, maplength, fd);
> +      l->l_map_start = _dl_map_segment (mappref, maplength, c->mapalign);
>        if (__glibc_unlikely ((void *) l->l_map_start == MAP_FAILED))
>          return DL_MAP_SEGMENTS_ERROR_MAP_SEGMENT;
>  
> @@ -123,14 +129,14 @@ _dl_map_segments (struct link_map *l, int fd,
>          }
>  
>        l->l_contiguous = 1;
> -
> -      goto postmap;
>      }
> -
> -  /* Remember which part of the address space this object uses.  */
> -  l->l_map_start = c->mapstart + l->l_addr;
> -  l->l_map_end = l->l_map_start + maplength;
> -  l->l_contiguous = !has_holes;
> +  else
> +    {
> +      /* Remember which part of the address space this object uses.  */
> +      l->l_map_start = c->mapstart + l->l_addr;
> +      l->l_map_end = l->l_map_start + maplength;
> +      l->l_contiguous = !has_holes;
> +    }
>  
>    while (c < &loadcmds[nloadcmds])
>      {
> @@ -143,7 +149,6 @@ _dl_map_segments (struct link_map *l, int fd,
>                == MAP_FAILED))
>          return DL_MAP_SEGMENTS_ERROR_MAP_SEGMENT;
>  
> -    postmap:
>        _dl_postprocess_loadcmd (l, header, c);
>  
>        if (c->allocend > c->dataend)