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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h | ||
---|---|---|
108 | 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. |
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h | ||
---|---|---|
108 | 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. |
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h | ||
---|---|---|
108 | 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 |
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h | ||
---|---|---|
108 | 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? |
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h | ||
---|---|---|
108 | 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 |
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? |
Yes, I will address it in a different patch.
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h | ||
---|---|---|
108 | 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? |
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h | ||
---|---|---|
108 | 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. |
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h | ||
---|---|---|
108 | 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. |
Reverted, it fails on Windows https://lab.llvm.org/buildbot/#/builders/127/builds/7999 and Android https://lab.llvm.org/buildbot/#/builders/77/builds/4839
clang-tidy: error: This file must be included inside sanitizer_allocator.h [clang-diagnostic-error]
not useful