Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v6]
On Mon, 9 Jan 2023 08:08:28 GMT, Thomas Stuefe wrote: > Metaspace changes mostly good. Do the gtests pass? Especially the Metaspace > fence tests? You need to execute them via the jtreg wrapper > (test/hotspot/jtreg/gtest) - these wrappers execute the googletest suite with > a number of different settings, among others also metaspace fences enabled. They do now. Had to poison/unpoison in the test. The other failures are from print_hex_dump being instrumented and just general incompatibility with ASan. They should probably be fixed in general. Filed JDK-8299826 to take a look at some point. - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v6]
On Mon, 9 Jan 2023 07:39:03 GMT, Thomas Stuefe wrote:
>> Justin King has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Use -fno-common as suggested by ASan docs
>>
>> Signed-off-by: Justin King
>
> src/hotspot/share/memory/metaspace/chunkManager.cpp line 137:
>
>> 135: // - Or, if the necessary space cannot be committed because we hit a
>> commit limit.
>> 136: // This may be either the GC threshold or MaxMetaspaceSize.
>> 137: Metachunk* ChunkManager::get_chunk_locked(chunklevel_t preferred_level,
>> chunklevel_t max_level, size_t min_committed_words) {
>
> Please add `assert_lock_strong(Metaspace_lock);`
Done.
> src/hotspot/share/memory/metaspace/chunkManager.cpp line 247:
>
>> 245: //this function returns !!
>> 246: void ChunkManager::return_chunk(Metachunk* c) {
>> 247: ASAN_POISON_MEMORY_REGION(c->base(), c->word_size() * BytesPerWord);
>
> Please add comment:
>
> // It is valid to poison the chunk payload area at this point since its
> physically separated from the chunk meta info.
Done.
> src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp line 239:
>
>> 237: assert_is_aligned(_word_size, chunklevel::MAX_CHUNK_WORD_SIZE);
>> 238:
>> 239: // Poison the memory region. It will be unpoisoned later by
>> MetaspaceArena.
>
> Can you be a bit more specific? Proposal:
> "It will be unpoisened on a per-chunk base for chunks that are handed to
> arenas."
Done.
> src/hotspot/share/runtime/os.cpp line 951:
>
>> 949: NO_SANITIZE_ADDRESS void os::print_hex_dump(outputStream* st, address
>> start, address end,
>> 950: int unitsize, int
>> bytes_per_line,
>> 951: address logical_start) {
>
> Out of scope for this bug; can this be moved to a different RFE?
>
> While looking at this, I found that os::print_hex_dump has a little error. It
> is not supposed to access memory directly but only via SafeFetch (in order to
> print '?' for unreadable locations). SafeFetch would be the natural point for
> asan exclusion. I filed a bug for this:
> https://bugs.openjdk.org/browse/JDK-8299790
Sure, reverted the changes to this file. I think SafeFetch is fine already as
its assembly and instrumentation won't happen, but we can double check later
once its fixed. I think there are some non-assembly versions of SafeFetch and
that would need to be fixed.
-
PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v6]
On Thu, 5 Jan 2023 16:29:13 GMT, Justin King wrote:
>> This change instruments Metaspace for ASan. Metaspace allocates memory using
>> `mmap`/`munmap` which ASan is not aware of. Fortunately ASan supports
>> applications [manually poisoning/unpoisoning
>> memory](https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning).
>> ASan is able to detect poisoned memory, similar to `use-after-free`, and
>> will raise an error similarly called `use-after-poison`. This provides and
>> extra layer of defense and confidence.
>>
>> The header `sanitizers/address.h` defines macros for poisoning/unpoisoning
>> memory regions. These macros can be used regardless of build mode. When ASan
>> is not available, they are implemented using a NOOP approach which still
>> compiles the arguments but does so such that they will be stripped out by
>> the compiler due to being unreachable. This helps with maintenance.
>>
>> This also has the added benefit of making
>> [LSan](https://bugs.openjdk.org/browse/JDK-8298445) more accurate and
>> deterministic, as LSan will not look for pointers to malloc memory in
>> poisoned memory regions.
>>
>> IMO the benefit of doing this greatly outweighs the cost.
>
> Justin King has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use -fno-common as suggested by ASan docs
>
> Signed-off-by: Justin King
Metaspace changes mostly good. Do the gtests pass? Especially the Metaspace
fence tests? You need to execute them via the jtreg wrapper
(test/hotspot/jtreg/gtest) - these wrappers execute the googletest suite with a
number of different settings, among others also metaspace fences enabled.
src/hotspot/share/memory/metaspace/chunkManager.cpp line 137:
> 135: // - Or, if the necessary space cannot be committed because we hit a
> commit limit.
> 136: // This may be either the GC threshold or MaxMetaspaceSize.
> 137: Metachunk* ChunkManager::get_chunk_locked(chunklevel_t preferred_level,
> chunklevel_t max_level, size_t min_committed_words) {
Please add `assert_lock_strong(Metaspace_lock);`
src/hotspot/share/memory/metaspace/chunkManager.cpp line 247:
> 245: //this function returns !!
> 246: void ChunkManager::return_chunk(Metachunk* c) {
> 247: ASAN_POISON_MEMORY_REGION(c->base(), c->word_size() * BytesPerWord);
Please add comment:
// It is valid to poison the chunk payload area at this point since its
physically separated from the chunk meta info.
src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp line 239:
> 237: assert_is_aligned(_word_size, chunklevel::MAX_CHUNK_WORD_SIZE);
> 238:
> 239: // Poison the memory region. It will be unpoisoned later by
> MetaspaceArena.
Can you be a bit more specific? Proposal:
"It will be unpoisened on a per-chunk base for chunks that are handed to
arenas."
src/hotspot/share/runtime/os.cpp line 951:
> 949: NO_SANITIZE_ADDRESS void os::print_hex_dump(outputStream* st, address
> start, address end,
> 950: int unitsize, int
> bytes_per_line,
> 951: address logical_start) {
Out of scope for this bug; can this be moved to a different RFE?
While looking at this, I found that os::print_hex_dump has a little error. It
is not supposed to access memory directly but only via SafeFetch (in order to
print '?' for unreadable locations). SafeFetch would be the natural point for
asan exclusion. I filed a bug for this:
https://bugs.openjdk.org/browse/JDK-8299790
-
PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v6]
On Thu, 5 Jan 2023 16:29:13 GMT, Justin King wrote: >> This change instruments Metaspace for ASan. Metaspace allocates memory using >> `mmap`/`munmap` which ASan is not aware of. Fortunately ASan supports >> applications [manually poisoning/unpoisoning >> memory](https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning). >> ASan is able to detect poisoned memory, similar to `use-after-free`, and >> will raise an error similarly called `use-after-poison`. This provides and >> extra layer of defense and confidence. >> >> The header `sanitizers/address.h` defines macros for poisoning/unpoisoning >> memory regions. These macros can be used regardless of build mode. When ASan >> is not available, they are implemented using a NOOP approach which still >> compiles the arguments but does so such that they will be stripped out by >> the compiler due to being unreachable. This helps with maintenance. >> >> This also has the added benefit of making >> [LSan](https://bugs.openjdk.org/browse/JDK-8298445) more accurate and >> deterministic, as LSan will not look for pointers to malloc memory in >> poisoned memory regions. >> >> IMO the benefit of doing this greatly outweighs the cost. > > Justin King has updated the pull request incrementally with one additional > commit since the last revision: > > Use -fno-common as suggested by ASan docs > > Signed-off-by: Justin King Yeah, I was originally going for the most pedantic implementation. But I like your proposed approach and it strikes a happy middle ground. > That explains (I think) another question I had. Which is how are overlapped > ranges handled. Given four subsequent addresses a b c d, I should be able to > poison [b,c) and then poison [a,d), right? Or do I have to take care to only > pass non-overlapping mappings? ASAN is fine with overlapping. It doesn't check the current state of poisoning/unpoisoning when doing the operations. Though there are variants that do such as __sanitizer_annotate_contiguous_container, but that is for instrumenting contiguous containers similar to std::vector. That will come at a later time for applicable Hotspot containers, but out of scope for this change and unrelated to Metaspace. > Great. I'll take a look next week since I still have vacation. No problem. - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v6]
> This change instruments Metaspace for ASan. Metaspace allocates memory using > `mmap`/`munmap` which ASan is not aware of. Fortunately ASan supports > applications [manually poisoning/unpoisoning > memory](https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning). > ASan is able to detect poisoned memory, similar to `use-after-free`, and > will raise an error similarly called `use-after-poison`. This provides and > extra layer of defense and confidence. > > The header `sanitizers/address.h` defines macros for poisoning/unpoisoning > memory regions. These macros can be used regardless of build mode. When ASan > is not available, they are implemented using a NOOP approach which still > compiles the arguments but does so such that they will be stripped out by the > compiler due to being unreachable. This helps with maintenance. > > This also has the added benefit of making > [LSan](https://bugs.openjdk.org/browse/JDK-8298445) more accurate and > deterministic, as LSan will not look for pointers to malloc memory in > poisoned memory regions. > > IMO the benefit of doing this greatly outweighs the cost. Justin King has updated the pull request incrementally with one additional commit since the last revision: Use -fno-common as suggested by ASan docs Signed-off-by: Justin King - Changes: - all: https://git.openjdk.org/jdk/pull/11702/files - new: https://git.openjdk.org/jdk/pull/11702/files/a4b5da42..00cf2efc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11702&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11702&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11702.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11702/head:pull/11702 PR: https://git.openjdk.org/jdk/pull/11702
