This is an archive of the discontinued LLVM Phabricator instance.

[GWP-ASan] Refactor memory mapping functions
ClosedPublic

Authored by cryptoad on Oct 22 2020, 3:54 PM.

Details

Summary

In preparation for Fuchsia support, this CL refactors the memory
mapping functions.

The new functions are as follows:

  • for Freeslots and Metadata: void *map(size_t Size, const char *Name) const; void unmap(void *Ptr, size_t Size) const;
  • for the Pool: void *reservePool(size_t Size); void commitPool(void *Ptr, size_t Size) const; void decommitPool(void *Ptr, size_t Size) const; void unreservePool(); Note that those don't need a Name parameter as those are fixed per function. {reserve,unreserve}Pool are not const because they will modify platform specific class member on Fuchsia.

I added a plethora of assert() as the initial code was not enforcing
page alignment for sizes and addresses, which caused problem in the
initial Fuchsia draft. All sizes should now be properly rounded up to
a page.

Diff Detail

Event Timeline

cryptoad created this revision.Oct 22 2020, 3:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2020, 3:54 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
cryptoad requested review of this revision.Oct 22 2020, 3:54 PM
cryptoad updated this revision to Diff 300120.Oct 22 2020, 4:11 PM

Correcting a potential conversion warning.

hctim accepted this revision.Oct 23 2020, 10:32 AM
hctim added inline comments.
compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
128

nits just to be really clear/descriptive with the function names.

mapWithRW()

reserveGuardedPool()
commitGuardedPoolRegionWithRW()
decommitGuardedPoolRegion()
unreserveGuardedPool()

Also can you add a comment above why the pool is special? Something like:

The pool is managed separately, as some platforms (particularly Fuchsia) manage virtual memory regions as a chunk where individual pages can still have separate permissions. These platforms maintain metadata about the region in order to perform operations. The pool is unique as it's the only thing in GWP-ASan that treats pages in a single VM region on an individual basis for page protection.
This revision is now accepted and ready to land.Oct 23 2020, 10:32 AM
mcgrathr accepted this revision.Oct 23 2020, 2:26 PM

LGTM. Different names and more comments would be preferable.

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
128

I'm not in favor of the "decommit" name because the semantics on Fuchsia are to unmap the region so that later accesses will fault. "decommit" usually means "restore to lazy zero-fill state", not "make potentially inaccessible".

I'm certainly in favor of any clarifying comments that anyone thinks help.

135

I'd like to see some explicit comments here about the API contract: the returned ptr is the reserved address range of (at least) Size bytes; a Ptr/Size in commitPool must be a subrange of that range; a Ptr/Size in decommitPool must be an exact pair previously passed to commitPool.

136

How about allocateInPool and deallocateInPool?

compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_posix.cpp
29

Google style is to prefer anonymous namespace, I don't know about LLVM style.

49

could use the name in the error

cryptoad updated this revision to Diff 300679.Oct 26 2020, 8:11 AM
cryptoad marked 5 inline comments as done.

Addressing review comments:

  • tried to strike a middle ground between name suggestions, settled on: `void *reserveGuardedPool(size_t Size);

void allocateInGuardedPool(void *Ptr, size_t Size) const;
void deallocateInGuardedPool(void *Ptr, size_t Size) const;
void unreserveGuardedPool();`

  • added comments and clarifications about API contracts
  • used anonymous namespace instead of static
cryptoad updated this revision to Diff 300701.Oct 26 2020, 9:35 AM

clang-format a file that was missed.

hctim accepted this revision.Oct 26 2020, 11:38 AM

LGTM

mcgrathr accepted this revision.Oct 26 2020, 1:06 PM

Thanks!

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
130–131

Fuchsia supports it too and doesn't have this constraint.

This revision was landed with ongoing or failed builds.Oct 26 2020, 1:32 PM
This revision was automatically updated to reflect the committed changes.