On Wed, 4 Jan 2023 17:05:54 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> Justin King has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - Fix typo >> >> Signed-off-by: Justin King <jck...@google.com> >> - Merge remote-tracking branch 'upstream/master' into jdk-8298908 >> - Exclude more zapping when ASan is in use >> >> Signed-off-by: Justin King <jck...@google.com> >> - Instrument Metaspace for ASan >> >> Signed-off-by: Justin King <jck...@google.com> > > Hi Justin, > > I find this very interesting! Have several questions, though. > > Does Asan assume 8-byte-alignment? I'm hesitant to set this alignment in > stone since Metaspace could be allocated, at least now, with smaller > alignments. We don't do this now, but I'd like to keep the option open. But > see my proposal below, if you follow that, alignment should be no problem. > > How is 32-bit handled? > > Can you please explain the poisoning and unpoisening a bit? How is the > use-after-free instrumentation done (I assume build time instrumentation?) > and how fast is it, e.g. how fine granular can you go - can you have millions > of micro-ranges, is that still feasible and reasonable? Should we poison > before munmap? > > ---- > > Metaspace-wise, I think tracking *blocks* is not the best approach. > Especially *dealloced* blocks, since there, you need to keep the header > unpoisened, but the header is where most overwriters would happen. And block > deallocation is a special rare case anyway. > > Metaspace allocation works like this: > - VSN allocates memory via mmap and hands out *chunks* to arenas. Chunks are > 1k ... 4M in size. > - An arena then owns a series of chunks and hands out parts of these chunks > as individual *blocks* to the user. Blocks are usually 12 bytes.. ~1K, with a > heavy emphasis on tiny sizes. > - Arena is owned by a class loader. Typically individual blocks are not > deallocated. Instead, if the arena is deleted, it releases all *chunks* to a > pool (ChunkManager). Optionally these chunks are also temporarily > uncommitted. These chunks can be reused by Arenas later, being committed > again had they been uncommitted before. > - Block deallocation is a rare case that is only used if Blocks are released > before the arena is released. This happens e.g., if a class is redefined and > its old bytecode is let go, but class and classloader and hence arena live on. > > I think Chunks are a better point for tracking: > - way less ranges to track since granularity is coarser. Less runtime costs? > - Alignment is no problem since chunk start addresses are aligned to chunk > size (it is a buddy allocator), so at least 1k aligned. > - Chunks, in contrast to blocks, are maintained in a way that does not put > metadata into them. Metadata is kept separate. So you can poison freed chunks > at your heart's content with no header to tiptoe around. > - You lose a bit of accuracy, but honestly, not that much. The difference > between "memory in classloader-owned chunk" and "memory handed out to user" > is typically small. > > So I think it would be fine to unpoisen a whole chunk when given to an arena > instead of unpoisening every individual allocation from the arena. And poison > chunks when the Arena dies and puts the chunks back into the pool. So, if you > instrument `ChunkManager::get_chunk()` (poisen) and > `ChunkManager::return_chunk()` (unpoisen) in addition to your existing > pre-poisoning in VSN, it would be sufficient. You could remove the code in > binlist and blocktree, then. > > Cheers, Thomas @tstuefe Okay, I switched to per chunk handling. I also made unpoisoning on destruction of VirtualSpaceNode unconditional, to ensure future memory mapping that happens to reuse the address doesn't trigger use-after-poison unexpectedly. This should be extremely rare, but better safe than sorry. ------------- PR: https://git.openjdk.org/jdk/pull/11702
