[PATCH v6] malloc: Mark pages with MADV_DONTNEED to shrink and grow rather than mremap()

[William Hunt]

Hello Wilco, 

> +static __always_inline bool
> +_int_realloc_madvise (mchunkptr oldp, size_t difference)
> +{
> +  /* Only MADV_DONTNEED if allowing overcommit memory, otherwise there
> +     is a risk of exhausting the process' pre-determined VMA fraction.  */
> +  if (__glibc_unlikely (!check_can_keep_vma ()))
> +    return false;
> 
> I don't believe we need this check - it's never wrong to use madvise.
> 
> +  char *madv_start = (char *) mmap_base (oldp) + mmap_size (oldp) - difference;
> +  return __madvise (madv_start, difference, MADV_DONTNEED) != -1;
> +}
> 
> Only this part needs to check !check_may_shrink_heap():
> 
> +      difference = ALIGN_DOWN (difference, GLRO (dl_pagesize));
> +      /* Don't shrink more than the relative threshold for the chunk.  */
> +      if (difference <= max_madvise (mmap_size (oldp)) &&
> +          _int_realloc_madvise (oldp, difference))
> +          return oldmem;
> +    }
> 
> Then if mremap fails/doesn't exist, we still use madvise since that does reduce
> actual memory usage.
This works for Linux, since mremap() is supported and it's very unlikely even
if overcommitting is disabled that mremap() will fail enough times to run out 
of pages. For non-Linux systems this would cause madvise() to be called if we 
use it as a backup since mremap() is Linux-specific. Solaris disables 
overcommit memory by default, and FreeBSD and its derivates allow disabling. 
However, I assume we are not developing support for them since they use their 
own C libraries and I'm not sure if glibc can even be used on these OSs?
 
> +/* When disabling overcommit behavior, we want to make sure that we are
> +   judicious with our heap usage as well, and explicitly give away the freed
> +   top of the heap to reduce our commit charge.  */
> +static inline bool
> +check_may_shrink_heap (void)
> +{
> +  static int may_shrink_heap = -1;
> +
> +  return may_shrink_heap >= 0 || check_can_keep_vma ();
>  }
> 
> This removes the caching of the expensive systemcall... It may be worth renaming
> to something more useful if we can think of a better name.
I'll fix this so that may_shrink_heap is check for being non-negative, then 
assigned to the function call. A better name for check_can_keep_vma() would be 
check_cannot_overcommit(). 
 
> Note +  return !overcommitted; incorrectly flips the existing meaning of true/false.
I got confused by the check_may_shrink_heap(), since this implies the opposite 
of what it does. I'll change the name to check_cannot_madvise_arena(). 


> Florian: I am not sure what the __libc_enable_secure has to do with overcommit - so
> I am wondering whether we could remove that check altogether?
I think the intention is to remove access to the pages, so a process running 
with extra privileges doesn't expose data after using a secure exec to a less 
privileged process. MADV_DONTNEED uses zero-fill-on-demand pages for anonymous 
private mappings, so an madvise() works for mmap'd chunks and non-main arenas. 
The main arena uses MORECORE which by default is sbrk, and the process' heap 
is anonymous and private so this should also give zero-fill-on-demand pages. 
However, if MORECORE is defined to something like shm_open() then the memory 
contents would be repopulated from the underlying mapped file. I'm not sure if 
something like that could be supported by malloc though? 
 
> Note there is an existing bug in that it is not thread-safe:
> 
> static inline bool
> check_may_shrink_heap (void)
> {
>   static int may_shrink_heap = -1;
> 
>   if (__builtin_expect (may_shrink_heap >= 0, 1))        // should use __glibc_likely
>     return may_shrink_heap;
> 
>   may_shrink_heap = __libc_enable_secure;        // first write
> 
>   if (__builtin_expect (may_shrink_heap == 0, 1))  // spurious read (may not return same value as __libc_enable_secure)
>     {
>       int fd = __open_nocancel ("/proc/sys/vm/overcommit_memory",
>                                 O_RDONLY | O_CLOEXEC);
>       if (fd >= 0)
>         {
>           char val;
>           ssize_t n = __read_nocancel (fd, &val, 1);
>           may_shrink_heap = n > 0 && val == '2';    // 2nd write
>           __close_nocancel_nostatus (fd);
>         }
>     }
> 
>   return may_shrink_heap;      // spurious read (may return different value from last write)
> }
> 
> Basically one can do one read of the global and one write if the read says it is not
> initialized - otherwise you end up with glitches and incorrect values returned.
Good catch, I'll fix that with an atomic_load_relaxed().

Kind regards,
William