New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read barrier verification #4037

Merged
merged 1 commit into from Jan 10, 2019

Conversation

Projects
None yet
3 participants
@VermaSh
Copy link
Contributor

VermaSh commented Dec 13, 2018

This PR provides implementation for ReadBarrierVerifier. This read barrier is designed to catch direct memory read sites. Memory slots are poisoned after GC and only way to read the correct address is through ReadBarrierVerifier. Any read access made outside of this barrier will result in inaccessible address error due to shadow heap address range protection.

Use cases:
The test can be toggled individually or all together, by setting fvtest_enableReadBarrierVerification to a combination of five 1s. Testing for JNI global reference can be toggled by passing 01000. Testing for monitor table object slots can be toggled by passing 00100. Testing for class statics can be toggled by passing 00010. Testing for heap slots can be toggled by passing 00001. Finally, testing for all of the above can be toggled by passing 11111.

Scope of this instrumentation is limited to standard gc policies using non compressed pointers on 64 bit machines. Heap can only be tested on optthruput gc policy. Options, -alwaysCallReadBarrier, -Xgcpolicy:optthruput and -Xint, are needed when testing for heap slots. Root slot testing, however, doesn't need either of those and can be tested on gencon gc policy as well.

Cmd to enable instrumentation for monitor slots and class statics:
./java -Xnocompressedrefs -XXgc:fvtest_enableReadBarrierVerification=00110

Cmd to enable instrumentation for heap slots:
./java -Xnocompressedrefs -Xgcpolicy:optthruput -XXgc:fvtest_enableReadBarrierVerification=00001 -Xint -Xgc:alwaysCallReadBarrier

@amicic amicic requested a review from dmitripivkine Dec 13, 2018

@amicic

This comment has been minimized.

Copy link
Contributor

amicic commented Dec 13, 2018

dependent on eclipse/omr#3297

@@ -748,6 +748,9 @@ MM_StandardAccessBarrier::preMonitorTableSlotRead(J9VMThread *vmThread, j9object
{
omrobjectptr_t object = (omrobjectptr_t)*srcAddress;

if (NULL == _extensions->scavenger) { /* This check is needed if gc_alwaysCallObjectAccessBarrier build flag is enabled */
return true;
}

This comment has been minimized.

@amicic

amicic Dec 13, 2018

Contributor

remove these changes. they are correct, but we'll consider them as part of another PR

if (shadowHeapTop > (uintptr_t)object && (shadowHeapBase <= (uintptr_t)object)) {
healedAddress = heapBase + ((uintptr_t)object - shadowHeapBase);

// TODO: instead of atomicWriteReference, write back to srcAddress. But maybe we might need it to be atomic

This comment has been minimized.

@amicic

amicic Dec 13, 2018

Contributor

let's play safe and use atomic update here. I belive that there could be a race where another thread can write a different value between the moment this thread read a poissoned value and the moment if wants to update with heal value. so, this thread should fail on update if the field has been modified meanwhile.

update two other spots too

GC_SlotObject slotObject(env->getOmrVM(), srcAddress);

if ((shadowHeapTop > (uintptr_t)object && shadowHeapBase <= (uintptr_t)object)) {
healedAddress = heapBase + ((uintptr_t)object - shadowHeapBase);

This comment has been minimized.

@dmitripivkine

dmitripivkine Dec 13, 2018

Contributor

this variable should be defined here to narrow it's scope

uintptr_t heapBase = (uintptr_t)_extensions->heap->getHeapBase();
uintptr_t shadowHeapBase = (uintptr_t)_extensions->shadowHeapBase;
uintptr_t shadowHeapTop = (uintptr_t)_extensions->shadowHeapTop;
uintptr_t healedAddress;

This comment has been minimized.

@dmitripivkine

dmitripivkine Dec 13, 2018

Contributor

remove this, define it inside if statement

uintptr_t heapTop = (uintptr_t)extensions->heap->getHeapTop();
uintptr_t shadowHeapBase = (uintptr_t)extensions->shadowHeapBase;

uintptr_t poisonedAddress;

This comment has been minimized.

@dmitripivkine

dmitripivkine Dec 13, 2018

Contributor

define this inside if statement

_extensions->accessBarrier = MM_StandardAccessBarrier::newInstance(env);

#if defined(OMR_ENV_DATA64) && !defined(OMR_GC_COMPRESSED_POINTERS)
if (1 == (1 & _extensions->fvtest_enableReadBarrierVerification)) {

This comment has been minimized.

@dmitripivkine

dmitripivkine Dec 13, 2018

Contributor

if this flag has value 1 set only, so there is no need to mask it. It would make sense if this field might have different different bit pattern set but looks like this is not a case

uintptr_t shadowHeapBase = (uintptr_t)extensions->shadowHeapBase;
uintptr_t shadowHeapTop = (uintptr_t)extensions->shadowHeapTop;
uintptr_t healedAddress;
j9object_t object = *srcAddress;

This comment has been minimized.

@dmitripivkine

dmitripivkine Dec 13, 2018

Contributor

move these two definitions inside if statement

uintptr_t shadowHeapBase = (uintptr_t)extensions->shadowHeapBase;
uintptr_t shadowHeapTop = (uintptr_t)extensions->shadowHeapTop;

uintptr_t healedHeapAddress;

This comment has been minimized.

@dmitripivkine

dmitripivkine Dec 13, 2018

Contributor

define this inside if statement

uintptr_t shadowHeapBase = (uintptr_t)_extensions->shadowHeapBase;
uintptr_t shadowHeapTop = (uintptr_t)_extensions->shadowHeapTop;
uintptr_t healedAddress;
j9object_t object = *srcAddress;

This comment has been minimized.

@dmitripivkine

dmitripivkine Dec 13, 2018

Contributor

move these two definitions inside if statement

GC_ClassHeapIterator classHeapIterator(static_cast<J9JavaVM*>(omrVMThread->_vm->_language_vm), segment);
J9Class *clazz = NULL;

while(NULL != (clazz = classHeapIterator.nextClass())) {

This comment has been minimized.

@dmitripivkine

dmitripivkine Dec 13, 2018

Contributor

please add space between while and bracket

OMR_VMThread *omrVMThread = env->getOmrVMThread();
GC_SegmentIterator segmentIterator(static_cast<J9JavaVM*>(omrVMThread->_vm->_language_vm)->classMemorySegments, MEMORY_TYPE_RAM_CLASS);

while(J9MemorySegment *segment = segmentIterator.nextSegment()) {

This comment has been minimized.

@dmitripivkine

dmitripivkine Dec 13, 2018

Contributor

please add space between while and bracket

GC_ClassHeapIterator classHeapIterator(static_cast<J9JavaVM*>(omrVMThread->_vm->_language_vm), segment);
J9Class *clazz = NULL;

while(NULL != (clazz = classHeapIterator.nextClass())) {

This comment has been minimized.

@dmitripivkine

dmitripivkine Dec 13, 2018

Contributor

please add space between while and bracket

OMR_VMThread *omrVMThread = env->getOmrVMThread();
GC_SegmentIterator segmentIterator(static_cast<J9JavaVM*>(omrVMThread->_vm->_language_vm)->classMemorySegments, MEMORY_TYPE_RAM_CLASS);

while(J9MemorySegment *segment = segmentIterator.nextSegment()) {

This comment has been minimized.

@dmitripivkine

dmitripivkine Dec 13, 2018

Contributor

please add space between while and bracket


volatile omrobjectptr_t *slotPtr = NULL;
GC_ClassStaticsIterator classStaticsIterator(env, clazz);
while (NULL != (slotPtr = (omrobject_t*)classStaticsIterator.nextSlot())) {

This comment has been minimized.

@amicic

amicic Dec 13, 2018

Contributor

shouldn't be casting to 'omrobjectptr_t *'?

@@ -853,6 +853,7 @@ bool
MM_StandardAccessBarrier::preObjectRead(J9VMThread *vmThread, J9Object *srcObject, fj9object_t *srcAddress)
{
MM_EnvironmentStandard *env = MM_EnvironmentStandard::getEnvironment(vmThread->omrVMThread);

This comment has been minimized.

@amicic

amicic Dec 13, 2018

Contributor

don't change spacing. this file just simply needs no change

@@ -44,7 +43,7 @@

class MM_StandardAccessBarrier : public MM_ObjectAccessBarrier
{
private:
protected:

This comment has been minimized.

@amicic

amicic Dec 13, 2018

Contributor

this is probably unnecessary, now that Verifier does not have its initialize/teardown

uintptr_t shadowHeapTop = (uintptr_t)extensions->shadowHeapTop;
uintptr_t referenceFromSlot = (uintptr_t)*slot;

if (shadowHeapTop > referenceFromSlot && (shadowHeapBase => referenceFromSlot)) {

This comment has been minimized.

@amicic

amicic Dec 13, 2018

Contributor

extra brackets around first comparisson

This comment has been minimized.

@amicic

amicic Dec 13, 2018

Contributor

does operator '=>' really exist?

uintptr_t shadowHeapTop = (uintptr_t)extensions->shadowHeapTop;
omrobjectptr_t object = convertPointerFromToken(*srcAddress);

if (shadowHeapTop > (uintptr_t)object && (shadowHeapBase <= (uintptr_t)object)) {

This comment has been minimized.

@amicic

amicic Dec 13, 2018

Contributor

extra brackets around first comparison

@VermaSh VermaSh force-pushed the VermaSh:readBarrierVerification branch 3 times, most recently from 010bb3d to f9b77df Dec 13, 2018


bool
MM_ReadBarrierVerifier::preObjectRead(J9VMThread *vmThread, J9Object *srcObject, fj9object_t *srcAddress)
{

This comment has been minimized.

@dmitripivkine

dmitripivkine Dec 14, 2018

Contributor

In general these virtual methods should call parent's functionality as well. It happen to be no need for now because of limited scope. "As is" is ok for now, but we should not forget about it

@dmitripivkine
Copy link
Contributor

dmitripivkine left a comment

I believe it is good enough to submit

@VermaSh VermaSh changed the title WIP: Read barrier verification Read barrier verification Dec 14, 2018

@VermaSh VermaSh force-pushed the VermaSh:readBarrierVerification branch 2 times, most recently from 03d3326 to 1987b32 Dec 14, 2018

Read barrier verification
This PR provides implementation for ReadBarrierVerifier. This read
barrier is designed to catch direct memory read sites. Memory slots
are poisoned after GC and only way to read the correct address is
through ReadBarrierVerifier. Any read access made outside of this
barrier will result in inaccessible address error due to shadow heap
address range protection.

Use cases:
The test can be toggled individually or all together, by setting
fvtest_enableReadBarrierVerification to a combination of five 1s.
Testing for JNI global reference can be toggled by passing 01000.
Testing for monitor table object slots can be toggled by passing
00100. Testing for class statics can be toggled by passing 00010.
Testing for heap slots can be toggled by passing 00001. Finally,
testing for all of the above can be toggled by passing 11111.

Scope of this instrumentation is limited to standard gc policies using
non compressed pointers on 64 bit machines. Heap can only be tested on
optthruput gc policy. Options, -alwaysCallReadBarrier,
-Xgcpolicy:optthruput and -Xint, are needed when testing for heap
slots. Root slot testing, however, doesn't need either of those and
can be tested on gencon gc policy as well.

Cmd to enable instrumentation for monitor slots and class statics:
./java -Xnocompressedrefs
-XXgc:fvtest_enableReadBarrierVerification=00110

Cmd to enable instrumentation for heap slots:
./java -Xnocompressedrefs -Xgcpolicy:optthruput
-XXgc:fvtest_enableReadBarrierVerification=00001 -Xint
-Xgc:alwaysCallReadBarrier

Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>

@VermaSh VermaSh force-pushed the VermaSh:readBarrierVerification branch from 1987b32 to 54f04a8 Dec 14, 2018

@amicic amicic referenced this pull request Dec 17, 2018

Merged

Read barrier verification #3362

@amicic

This comment has been minimized.

Copy link
Contributor

amicic commented Jan 9, 2019

Jenkins test sanity xLinux jdk8

@amicic

This comment has been minimized.

Copy link
Contributor

amicic commented Jan 9, 2019

Jenkins test sanity xLinux jdk8 depends eclipse/omr#3362

@amicic amicic merged commit 066af9b into eclipse:master Jan 10, 2019

5 checks passed

Copyright Check Copyrights appear to be up to date
Details
Line Endings Check Line endings appear to be correct
Details
Sanity - JDK8 - linux_x86-64_cmprssptrs Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment