This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Support dynamic premapped R/W range in primary allocator.
ClosedPublic

Authored by morehouse on Mar 9 2021, 2:38 PM.

Details

Summary

The main use case for this change is HWASan aliasing mode, which premaps
the alias space adjacent to the dynamic shadow. With this change, the
primary allocator can allocate from the alias space instead of a
separate region.

Diff Detail

Event Timeline

morehouse requested review of this revision.Mar 9 2021, 2:38 PM
morehouse created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2021, 2:38 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
eugenis accepted this revision.Mar 9 2021, 4:12 PM

LGTM

compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
108–109

I wonder if MADVISE_DONTNEED works correctly on aliased memory? You probably need to run it on each alias for it to have an effect on RSS.

This revision is now accepted and ready to land.Mar 9 2021, 4:12 PM
morehouse added inline comments.Mar 10 2021, 10:20 AM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
108–109

Yes, just ran an experiment. Each alias occupies RSS independently. So we can fault in one alias and the others won't take up RSS until we touch them. If we touch multiple aliases, they each take up a chunk of RSS.

Probably we need a callback that hwasan_allocator.cpp can register in order to release alias pages as well.

pcc added a subscriber: pcc.Mar 10 2021, 2:35 PM
pcc added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
108–109

I don't think it would. Since the original mapping was created with MAP_SHARED, MADV_DONTNEED will not affect the contents of the pages. I confirmed this by running this program:

#define _GNU_SOURCE
#include <sys/mman.h>
#include <stdio.h>

int main() {
  void *p = mmap(0, 4096, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0); 
  void *p2 = mremap(p, 0, 4096, MREMAP_MAYMOVE);
  *(char *)p = 1;
  printf("%d\n", *(volatile char *)p2);
  madvise(p, 4096, MADV_DONTNEED);
  printf("%d\n", *(volatile char *)p2);
  madvise(p2, 4096, MADV_DONTNEED);
  printf("%d\n", *(volatile char *)p2);
}

The output is

1
1
1
morehouse added inline comments.Mar 10 2021, 2:44 PM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
108–109

Right, the memory is not cleared, but RSS does go down by the expected amount after the MADV_DONTNEED.

So it seems worth adding a callback?

pcc added inline comments.Mar 10 2021, 4:42 PM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
108–109

The goal isn't just to make RSS go down though, right? Presumably if the data is still accessible then it's still in memory. I'm not sure if there's a syscall sequence that we can use to release this memory.

This is also a bit more concerning:

#define _GNU_SOURCE
#include <sys/mman.h>
#include <stdio.h>

int main() {
  void *p = mmap(0, 4096, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0);
  void *p2 = mremap(p, 0, 4096, MREMAP_MAYMOVE);
  *(char *)p = 1;
  printf("%d\n", *(volatile char *)p2);
  madvise(p, 4096, MADV_DONTNEED);
  printf("%d\n", *(volatile char *)p2);
  madvise(p2, 4096, MADV_DONTNEED);
  printf("%d\n", *(volatile char *)p2);
  if (fork() == 0) {
    *(char *)p = 2;
    exit(0);
  }
  sleep(1);
  printf("%d\n", *(volatile char *)p);
}

The output is

1
1
1
2
vitalybuka accepted this revision.Mar 10 2021, 10:33 PM

LGTM if "release" will be addressed in another patch

compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
678

do we need to check that beg/size are in mapped range?

morehouse updated this revision to Diff 329983.Mar 11 2021, 9:05 AM
morehouse marked an inline comment as done.
  • Check bounds on mmaps.

LGTM if "release" will be addressed in another patch

Yes, I will address it in a different patch.

compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
108–109

I think memory sharing across fork is the price we pay for MAP_SHARED. At least within Google I expect it to not be much problem since uses of fork are 99%+ fork+exec. Maybe we can duplicate the alias region with a fork interceptor, but this is complexity I'd like to avoid unless necessary.

For actually releasing the physical memory, the only thing I think *might* work is tmpfs + fallocate(FALLOCATE_FL_PUNCH_HOLE). I'd need to experiment to be sure. But that also involves more complexity that I'd like to avoid (at least initially). Let's only deal with it if it becomes an issue?

morehouse added inline comments.Mar 12 2021, 11:34 AM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
108–109

Unfortunately some of the build infra (python, flex) within Google calls fork. I tried changing TCMalloc to use MAP_SHARED, and got crashes when building things:

Fatal Python error: GC object already tracked
(Segmentation fault) flex failed: error executing command...

So I am less confident we can deploy this now...

With a fork interceptor we might be able to duplicate and recreate aliases in the child, but copying TBs of memory will be very slow.

eugenis added inline comments.Mar 12 2021, 1:36 PM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
108–109

Ouch. Even in fork + exec code there may be a heap memory write between fork and exec that would affect the parent process.

We do not need to copy TBs of memory - only the actual physical pages. We could do this by scanning /proc/$PID/pagemap. Or maybe mlock + mincore? Even such limited copy might be prohibitively expensive.

  • Add TODO for release callback support.
eugenis accepted this revision.Mar 17 2021, 2:53 PM

LGTM

This revision was landed with ongoing or failed builds.Mar 22 2021, 2:45 PM
This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Mar 22 2021, 6:53 PM
morehouse updated this revision to Diff 332700.Mar 23 2021, 9:29 AM
  • Disable tests on Android and Windows due to OOM.
This revision was landed with ongoing or failed builds.Mar 23 2021, 10:00 AM
This revision was automatically updated to reflect the committed changes.