Release the shadow memory used by the mmap range at munmap

[stephan.yichao.zhao]

Summary

When an application does a lot of pairs of mmap and munmap, if we did
not release shadoe memory used by mmap addresses, this would increase
memory usage.

[morehouse]

Can we add a test that ensures RSS doesn't increase after a mmap + munmap pair?

compiler-rt/lib/dfsan/dfsan_interceptors.cpp

53

Nit: GetPageSizeCached

[stephan.yichao.zhao]

addressed comments
added a test case

[morehouse]

compiler-rt/test/dfsan/munmap_release_shadow.c

12

Nit: get_rss_kb seems simpler

18

Why ssize_t instead of size_t or long?

39

Rather than returning errors from this function, can we simplify by asserting inline?

43

I think the Linux kernel avoids reusing address space by default, so we don't need to give mmap hints.

I think I'd prefer removing the hints to simplify the test.

69

I'm not sure, but 4GB RSS might be too much on the buildbots. Can we simplify the test and reduce this by doing a single large mmap + munmap instead of many loops?

[stephan.yichao.zhao]

addressed comments

[stephan.yichao.zhao]

compiler-rt/lib/dfsan/dfsan_interceptors.cpp

37

With madvise by DONTNEED OR FREE (SANITIZER_MADVISE_DONTNEED) at munmap, is it possible to also remove these dfsan_set_labels ? Before the next access OS will zero-fill data.

https://www.man7.org/linux/man-pages/man2/madvise.2.html says
"""
MADV_DONTNEED

...
After a successful MADV_DONTNEED operation, the semantics of
memory access in the specified region are changed: subsequent
accesses of pages in the range will succeed, but will result
in either ... or zero-
fill-on-demand pages for anonymous private mappings.

MADV_FREE (since Linux 4.5)

...
Once pages in the range have been freed, the caller will see
zero-fill-on-demand pages upon subsequent page references.

"""

compiler-rt/test/dfsan/munmap_release_shadow.c

43

In our system, for some reason, the mmap likes to reuse the last address. Sometimes all returned addresses are the same from hundreds of runs.
With the hint, it is able to return different addresses.

69

reduced to 50 loops. This reduced the max RSS below 500M. This also reduces its test time from 20s to 2-3s.

[morehouse]

compiler-rt/lib/dfsan/dfsan_interceptors.cpp

37

Yes, it should work as an optimization. Happy to take a separate patch for it.

compiler-rt/test/dfsan/munmap_release_shadow.c

43

Ok. Then maybe let's make hint a static variable in mmap_track_and_munmap to simplify the logic.

69

Do we need *any* loops at all? Isn't a single mmap + touch the memory + munmap enough to test that RSS increases then goes back down?

The test seems overly complicated if it could be tested with something as simple as:

const size_t map_size = 100 << 20;
size_t before = get_rss_kb();

void *p = mmap(NULL, map_size, ...);
memset(p, 0xff, map_size);
size_t after_mmap = get_rss_kb();

munmap(p, map_size);
size_t after_munmap = get_rss_kb();

assert(after_mmap >= before + 3 * map_size);
assert(after_munmap <= after_mmap - 3 * map_size);

[stephan.yichao.zhao]

addressed comments

[stephan.yichao.zhao]

updated

[stephan.yichao.zhao]

compiler-rt/test/dfsan/munmap_release_shadow.c

43

removed hint after simplifying the test.

69

Still called dfsan_set_label after memset because the dfsan_set_label in mmap may not touch pages if initial data are zero-filled.

[morehouse]

compiler-rt/test/dfsan/munmap_release_shadow.c

28–31

[stephan.yichao.zhao]

addressed comment

compiler-rt/test/dfsan/munmap_release_shadow.c

28–31

Thank you!

[glider]

I might be missing something, but if you release one of the shadow pages to the OS, couldn't it then accidentally use that page for one of the user anonymous mappings, thus breaking DFSan logic?

[stephan.yichao.zhao]

In D88686#2311152, @glider wrote:

I might be missing something, but if you release one of the shadow pages to the OS, couldn't it then accidentally use that page for one of the user anonymous mappings, thus breaking DFSan logic?

The mmap interceptions zero-out shadow spaces and anonymous mappings zero-fill mapped pages. So returning shadow pages at munmap equals to the existing behavior.

[eugenis]

In D88686#2311152, @glider wrote:

I might be missing something, but if you release one of the shadow pages to the OS, couldn't it then accidentally use that page for one of the user anonymous mappings, thus breaking DFSan logic?

ReleaseMemoryPagesToOS is not munmap, it's madvise.

[glider]

Fine, thanks for explaining!

compiler-rt/lib/dfsan/dfsan_interceptors.cpp

INTERCEPTOR(void *, mmap, void *addr, SIZE_T length, int prot, int flags,
   // interceptors_initialized is set to true during preinit_array, when we're
   // single-threaded.  So we don't need to worry about accessing it atomically.
   if (!interceptors_initialized)
     res = (void *)syscall(__NR_mmap, addr, length, prot, flags, fd, offset);
   else
     res = REAL(mmap)(addr, length, prot, flags, fd, offset);
 
   if (res != (void *)-1)
     dfsan_set_label(0, res, RoundUpTo(length, GetPageSize()));
   return res;
 }
 
 INTERCEPTOR(void *, mmap64, void *addr, SIZE_T length, int prot, int flags,
             int fd, OFF64_T offset) {
   void *res = REAL(mmap64)(addr, length, prot, flags, fd, offset);
   if (res != (void *)-1)
     dfsan_set_label(0, res, RoundUpTo(length, GetPageSize()));
   return res;
 }
 
+INTERCEPTOR(int, munmap, void *addr, SIZE_T length) {
+  int res = REAL(munmap)(addr, length);
+  if (res != -1) {
+    uptr beg_shadow_addr = (uptr)__dfsan::shadow_for(addr);
+    void *end_addr =
+        (void *)((uptr)addr + RoundUpTo(length, GetPageSizeCached()));
+    uptr end_shadow_addr = (uptr)__dfsan::shadow_for(end_addr);
+    ReleaseMemoryPagesToOS(beg_shadow_addr, end_shadow_addr);
+  }
+  return res;
+}
+
 namespace __dfsan {
 void InitializeInterceptors() {
   CHECK(!interceptors_initialized);
 
   INTERCEPT_FUNCTION(mmap);
   INTERCEPT_FUNCTION(mmap64);
+  INTERCEPT_FUNCTION(munmap);
 
   interceptors_initialized = true;
 }
 }  // namespace __dfsan

compiler-rt/test/dfsan/munmap_release_shadow.c

+// RUN: %clang_dfsan %s -o %t && %run %t
+
+#include <assert.h>
+#include <sanitizer/dfsan_interface.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+size_t get_rss_kb() {
+  long rss = 0L;
+  FILE *f = NULL;
+  assert((f = fopen("/proc/self/statm", "r")));
+  assert(fscanf(f, "%*s%ld", &rss) == 1);
+  fclose(f);
+  return ((size_t)rss * (size_t)sysconf(_SC_PAGESIZE)) >> 10;
+}
+
+int main(int argc, char **argv) {
+  const size_t map_size = 100 << 20;
+  size_t before = get_rss_kb();
+
+  char *p = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
+                 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  const dfsan_label label = dfsan_create_label("l", 0);
+  char val = 0xff;
+  dfsan_set_label(label, &val, sizeof(val));
+  memset(p, val, map_size);
+  size_t after_mmap = get_rss_kb();
+
+  munmap(p, map_size);
+  size_t after_munmap = get_rss_kb();
+
+  fprintf(stderr, "RSS at start: %td, after mmap: %td, after mumap: %td\n",
+          before, after_mmap, after_munmap);
+
+  // The memory after mmap increases 3 times of map_size because the overhead of
+  // shadow memory is 2x.
+  const size_t mmap_cost_kb = 3 * (map_size >> 10);
+  assert(after_mmap >= before + mmap_cost_kb);
+  // OS does not release memory to the same level as the start of the program.
+  // The assert checks the memory after munmap up to a delta.
+  const size_t delta = 5000;
+  assert(after_munmap + mmap_cost_kb <= after_mmap + delta);
+  return 0;
+}