Quantcast

RFR: 8177845: Need a mechanism to load Graal

classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle
(adding hotspot-compiler-dev)

Please review these changes that make jdk.internal.vm.compiler an upgradable compiler.
The primary requirement for this is to remove all usage of JDK internals from Graal.
While this most involves changes in Graal, there remain a few capabilities that must
be exposed via JVMCI. Namely:

1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal initialize
  can use system properties that are guaranteed not to have been modified by application
  code (Graal initialization is lazy and may occur after application code has been run).

2. Truffle needs to be able to trigger JVMCI initialization so that it can select the Graal
  optimized implementation of the Truffle runtime.

These capabilities have been added to jdk.vm.ci.services.Services. A comment has
also been added to this class to denote the current methods to be removed
in a subsequent bug to update the JDK copy of Graal with the upstream version which
no longer uses the methods. The methods destined for removal are:

exportJVMCITo(Class<?> requestor)
load(Class<S> service)
loadSingle(Class<S> service, boolean required)

The first one is no longer needed as JVMCI exports itself to Graal service providers
it loads via private API and Graal re-exports[1] JVMCI to any module the extends Graal.
The latter 2 are no longer required as Graal uses the standard ServiceLoader. This API
is only useful for JVMCI-8 where a hidden JVMCI class loader is used.

These changes have been tested against upstream Graal. To make the Graal tests pass, 2 extra
minor changes were required:

1. In JDK-8177673, HotSpotMemoryAccessProviderImpl::checkRead was added and throws IllegalArgumentException
  on all error paths except one which throws AssertionError. The latter was a mistake and has been
  fixed to also throw IllegalArgumentException.
2. An invalid assertion has been removed from HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
  Up to JDK 8, it was true that all classes in java.* packages were loaded by the boot class loader.
  This is no longer true in JDK 9 with classes like java.sql being loaded by platform class loader.

As hinted above, a subsequent bug will be required to pull in the latest upstream Graal and
remove use of JDK internal API from the jdk.aot module. The qualified exports to
jdk.vm.internal.compiler and jdk.aot can then be removed.

-Doug

https://bugs.openjdk.java.net/browse/JDK-8177845
http://cr.openjdk.java.net/~dnsimon/8177845/

[1] http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle
(adding hotspot-compiler-dev)

> On 19 Apr 2017, at 02:02, Mandy Chung <[hidden email]> wrote:
>
>
>> On Apr 18, 2017, at 3:13 PM, Doug Simon <[hidden email]> wrote:
>>
>> Please review these changes that make jdk.internal.vm.compiler an upgradable compiler.
>> :
>> http://cr.openjdk.java.net/~dnsimon/8177845/
>
> Thanks for making this change.  This would simplify the way to replace JDK’s graal with the lab graal.  A couple of comments:
>
> jdk/internal/misc/VM.java
>
> 161      * Note that the saved system properties do not include
> 162      * the ones set by sun.misc.Version.init().
>
> sun.misc.Version is no longer present in JDK 9.  Renamed to java.lang.VersionProps

Sorry, the webrev was out of date. I've updated it and also fixed the comment for VM::getSavedProperty.

> jdk/vm/ci/services/Services.java
> 67             Class.forName("jdk.vm.ci.runtime.JVMCI”);
>
> JVMCI class is local in jdk.internal.vm.ci module.  An alternative may be
> to provide a static initialize method rather than Class::forName.

JVMCI is broken into fine grained "projects" and the jdk.vm.ci.runtime[1] project depends on the jdk.vm.ci.services project[2] so I cannot make a direct reference without introducing a circular dependency. I don't expect the reflection cost to be significant in practice.

> jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java
> 211         for (HotSpotJVMCIBackendFactory factory : ServiceLoader.load(HotSpotJVMCIBackendFactory.class)) {
>
> This uses the thread context class loader to load providers.  Services::load uses
> the system class loader to load providers.  I suspect you want this to use the
> system class loader here too.
>
> jdk/vm/ci/services/JVMCIServiceLocator.java
> 78         for (JVMCIServiceLocator access : ServiceLoader.load(JVMCIServiceLocator.class)) {
>
> Same comment as above. I think you want to use system class loader to load providers.

Yes, I think you're right. I've updated the webrev with that change.

Thanks for the review.

-Doug

[1] http://hg.openjdk.java.net/jdk9/dev/hotspot/file/560d7aa083a2/.mx.jvmci/suite.py#l94
[2] http://hg.openjdk.java.net/jdk9/dev/hotspot/file/560d7aa083a2/.mx.jvmci/suite.py#l45

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle
In reply to this post by Doug Simon @ Oracle

> On 19 Apr 2017, at 16:04, Alan Bateman <[hidden email]> wrote:
>
> On 18/04/2017 23:13, Doug Simon wrote:
>
>> :
>>
>> https://bugs.openjdk.java.net/browse/JDK-8177845
>> http://cr.openjdk.java.net/~dnsimon/8177845/
>>
> I'm happy to see jdk.internal.vm.compiler changing to be an upgradable module and the patching foo going away.

Yes, I'm delighted to see the command line required for using upstream Graal shrinking!

> If I read the changes correctly then I can extend JVMCIServiceLocator and the construction of my sub-class will open up all packages in jdk.internal.vm.ci to me. It is there any to tie this to -XX:+EnableJVMCI?

We could but I'm not sure what it would buy you. The service lookup only originates from the JVMCI runtime and the initialization of JVMCI already checks EnableJVMCI[1]

-Doug

[1] http://hg.openjdk.java.net/jdk9/dev/hotspot/file/4368832d1991/src/share/vm/jvmci/jvmciRuntime.cpp#l634
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Christian Thalinger-4
In reply to this post by Doug Simon @ Oracle
I lost track a bit but why are we exporting jdk.vm.ci.services to the world now?

 module jdk.internal.vm.ci {
-    exports jdk.vm.ci.services to jdk.internal.vm.compiler;
+    exports jdk.vm.ci.services;

On Apr 18, 2017, at 9:16 PM, Doug Simon <[hidden email]> wrote:

(adding hotspot-compiler-dev)

Please review these changes that make jdk.internal.vm.compiler an upgradable compiler.
The primary requirement for this is to remove all usage of JDK internals from Graal.
While this most involves changes in Graal, there remain a few capabilities that must
be exposed via JVMCI. Namely:

1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal initialize
 can use system properties that are guaranteed not to have been modified by application
 code (Graal initialization is lazy and may occur after application code has been run).

2. Truffle needs to be able to trigger JVMCI initialization so that it can select the Graal
 optimized implementation of the Truffle runtime.

These capabilities have been added to jdk.vm.ci.services.Services. A comment has
also been added to this class to denote the current methods to be removed
in a subsequent bug to update the JDK copy of Graal with the upstream version which
no longer uses the methods. The methods destined for removal are:

exportJVMCITo(Class<?> requestor)
load(Class<S> service)
loadSingle(Class<S> service, boolean required)

The first one is no longer needed as JVMCI exports itself to Graal service providers
it loads via private API and Graal re-exports[1] JVMCI to any module the extends Graal.
The latter 2 are no longer required as Graal uses the standard ServiceLoader. This API
is only useful for JVMCI-8 where a hidden JVMCI class loader is used.

These changes have been tested against upstream Graal. To make the Graal tests pass, 2 extra
minor changes were required:

1. In JDK-8177673, HotSpotMemoryAccessProviderImpl::checkRead was added and throws IllegalArgumentException
 on all error paths except one which throws AssertionError. The latter was a mistake and has been
 fixed to also throw IllegalArgumentException.
2. An invalid assertion has been removed from HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
 Up to JDK 8, it was true that all classes in java.* packages were loaded by the boot class loader.
 This is no longer true in JDK 9 with classes like java.sql being loaded by platform class loader.

As hinted above, a subsequent bug will be required to pull in the latest upstream Graal and
remove use of JDK internal API from the jdk.aot module. The qualified exports to
jdk.vm.internal.compiler and jdk.aot can then be removed.

-Doug

https://bugs.openjdk.java.net/browse/JDK-8177845
http://cr.openjdk.java.net/~dnsimon/8177845/

[1] http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Mandy Chung
Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check.  Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports.

The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules.

Mandy

> On Apr 19, 2017, at 11:24 AM, Christian Thalinger <[hidden email]> wrote:
>
> I lost track a bit but why are we exporting jdk.vm.ci.services to the world now?
>
> module jdk.internal.vm.ci {
> -    exports jdk.vm.ci.services to jdk.internal.vm.compiler;
> +    exports jdk.vm.ci.services;
>
>> On Apr 18, 2017, at 9:16 PM, Doug Simon <[hidden email]> wrote:
>>
>> (adding hotspot-compiler-dev)
>>
>> Please review these changes that make jdk.internal.vm.compiler an upgradable compiler.
>> The primary requirement for this is to remove all usage of JDK internals from Graal.
>> While this most involves changes in Graal, there remain a few capabilities that must
>> be exposed via JVMCI. Namely:
>>
>> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal initialize
>> can use system properties that are guaranteed not to have been modified by application
>> code (Graal initialization is lazy and may occur after application code has been run).
>>
>> 2. Truffle needs to be able to trigger JVMCI initialization so that it can select the Graal
>> optimized implementation of the Truffle runtime.
>>
>> These capabilities have been added to jdk.vm.ci.services.Services. A comment has
>> also been added to this class to denote the current methods to be removed
>> in a subsequent bug to update the JDK copy of Graal with the upstream version which
>> no longer uses the methods. The methods destined for removal are:
>>
>> exportJVMCITo(Class<?> requestor)
>> load(Class<S> service)
>> loadSingle(Class<S> service, boolean required)
>>
>> The first one is no longer needed as JVMCI exports itself to Graal service providers
>> it loads via private API and Graal re-exports[1] JVMCI to any module the extends Graal.
>> The latter 2 are no longer required as Graal uses the standard ServiceLoader. This API
>> is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
>>
>> These changes have been tested against upstream Graal. To make the Graal tests pass, 2 extra
>> minor changes were required:
>>
>> 1. In JDK-8177673, HotSpotMemoryAccessProviderImpl::checkRead was added and throws IllegalArgumentException
>> on all error paths except one which throws AssertionError. The latter was a mistake and has been
>> fixed to also throw IllegalArgumentException.
>> 2. An invalid assertion has been removed from HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
>> Up to JDK 8, it was true that all classes in java.* packages were loaded by the boot class loader.
>> This is no longer true in JDK 9 with classes like java.sql being loaded by platform class loader.
>>
>> As hinted above, a subsequent bug will be required to pull in the latest upstream Graal and
>> remove use of JDK internal API from the jdk.aot module. The qualified exports to
>> jdk.vm.internal.compiler and jdk.aot can then be removed.
>>
>> -Doug
>>
>> https://bugs.openjdk.java.net/browse/JDK-8177845
>> http://cr.openjdk.java.net/~dnsimon/8177845/
>>
>> [1] http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Christian Thalinger-4

> On Apr 19, 2017, at 8:38 AM, Mandy Chung <[hidden email]> wrote:
>
> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check.  Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports.
>
> The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules.

This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services?  After all, this is a security issue.

>
> Mandy
>
>> On Apr 19, 2017, at 11:24 AM, Christian Thalinger <[hidden email]> wrote:
>>
>> I lost track a bit but why are we exporting jdk.vm.ci.services to the world now?
>>
>> module jdk.internal.vm.ci {
>> -    exports jdk.vm.ci.services to jdk.internal.vm.compiler;
>> +    exports jdk.vm.ci.services;
>>
>>> On Apr 18, 2017, at 9:16 PM, Doug Simon <[hidden email]> wrote:
>>>
>>> (adding hotspot-compiler-dev)
>>>
>>> Please review these changes that make jdk.internal.vm.compiler an upgradable compiler.
>>> The primary requirement for this is to remove all usage of JDK internals from Graal.
>>> While this most involves changes in Graal, there remain a few capabilities that must
>>> be exposed via JVMCI. Namely:
>>>
>>> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal initialize
>>> can use system properties that are guaranteed not to have been modified by application
>>> code (Graal initialization is lazy and may occur after application code has been run).
>>>
>>> 2. Truffle needs to be able to trigger JVMCI initialization so that it can select the Graal
>>> optimized implementation of the Truffle runtime.
>>>
>>> These capabilities have been added to jdk.vm.ci.services.Services. A comment has
>>> also been added to this class to denote the current methods to be removed
>>> in a subsequent bug to update the JDK copy of Graal with the upstream version which
>>> no longer uses the methods. The methods destined for removal are:
>>>
>>> exportJVMCITo(Class<?> requestor)
>>> load(Class<S> service)
>>> loadSingle(Class<S> service, boolean required)
>>>
>>> The first one is no longer needed as JVMCI exports itself to Graal service providers
>>> it loads via private API and Graal re-exports[1] JVMCI to any module the extends Graal.
>>> The latter 2 are no longer required as Graal uses the standard ServiceLoader. This API
>>> is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
>>>
>>> These changes have been tested against upstream Graal. To make the Graal tests pass, 2 extra
>>> minor changes were required:
>>>
>>> 1. In JDK-8177673, HotSpotMemoryAccessProviderImpl::checkRead was added and throws IllegalArgumentException
>>> on all error paths except one which throws AssertionError. The latter was a mistake and has been
>>> fixed to also throw IllegalArgumentException.
>>> 2. An invalid assertion has been removed from HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
>>> Up to JDK 8, it was true that all classes in java.* packages were loaded by the boot class loader.
>>> This is no longer true in JDK 9 with classes like java.sql being loaded by platform class loader.
>>>
>>> As hinted above, a subsequent bug will be required to pull in the latest upstream Graal and
>>> remove use of JDK internal API from the jdk.aot module. The qualified exports to
>>> jdk.vm.internal.compiler and jdk.aot can then be removed.
>>>
>>> -Doug
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8177845
>>> http://cr.openjdk.java.net/~dnsimon/8177845/
>>>
>>> [1] http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
>>>
>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Mandy Chung

> On Apr 19, 2017, at 11:55 AM, Christian Thalinger <[hidden email]> wrote:
>
>
>> On Apr 19, 2017, at 8:38 AM, Mandy Chung <[hidden email]> wrote:
>>
>> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check.  Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports.
>>
>> The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules.
>
> This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services?  

It's unqualified and no restriction in this change.

Mandy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle

> On 19 Apr 2017, at 21:04, Mandy Chung <[hidden email]> wrote:
>
>
>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger <[hidden email]> wrote:
>>
>>
>>> On Apr 19, 2017, at 8:38 AM, Mandy Chung <[hidden email]> wrote:
>>>
>>> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check.  Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports.
>>>
>>> The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules.
>>
>> This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services?  
>
> It's unqualified and no restriction in this change.

The public methods currently in jdk.vm.ci.services are:

1. JVMCIServiceLocator.getProvider(Class<S>)
2. JVMCIServiceLocator.getProviders(Class<S>)
3. Services.initializeJVMCI()
4. Services.getSavedProperties()
5. Services.exportJVMCITo(Class<?>)
6. Services.load(Class<S>)
7. Services.loadSingle(Class<S>, boolean)

1 should be made protected. I'll update the webrev with this change.

2 should check for JVMCIPermission. I'll update the webrev with this change.

3 is harmless from a security perspective in my opinion.

4 checks for JVMCIPermission.

5, 6 and 7 will be removed in a follow bug that updates Graal from upstream (and removes its usage of these methods).

Does this address the security concerns?

-Doug
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Christian Thalinger-4

On Apr 19, 2017, at 9:27 AM, Doug Simon <[hidden email]> wrote:


On 19 Apr 2017, at 21:04, Mandy Chung <[hidden email]> wrote:


On Apr 19, 2017, at 11:55 AM, Christian Thalinger <[hidden email]> wrote:


On Apr 19, 2017, at 8:38 AM, Mandy Chung <[hidden email]> wrote:

Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check.  Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports.

The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules.

This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services?  

It's unqualified and no restriction in this change.

The public methods currently in jdk.vm.ci.services are:

1. JVMCIServiceLocator.getProvider(Class<S>)
2. JVMCIServiceLocator.getProviders(Class<S>)
3. Services.initializeJVMCI()
4. Services.getSavedProperties()
5. Services.exportJVMCITo(Class<?>)
6. Services.load(Class<S>)
7. Services.loadSingle(Class<S>, boolean)

1 should be made protected. I'll update the webrev with this change.

Good.


2 should check for JVMCIPermission. I'll update the webrev with this change.

Good.


3 is harmless from a security perspective in my opinion.

Would be good if one of Oracle’s security engineers could take a quick look just to be sure.


4 checks for JVMCIPermission.

Ok.


5, 6 and 7 will be removed in a follow bug that updates Graal from upstream (and removes its usage of these methods).

About this, will this Graal update happen for JDK 9?  It’s awfully late in the cycle...


Does this address the security concerns?

Mostly, yes.  Thanks.


-Doug

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle

> On 19 Apr 2017, at 21:40, Christian Thalinger <[hidden email]> wrote:
>
>>
>> On Apr 19, 2017, at 9:27 AM, Doug Simon <[hidden email]> wrote:
>>
>>>
>>> On 19 Apr 2017, at 21:04, Mandy Chung <[hidden email]> wrote:
>>>
>>>
>>>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger <[hidden email]> wrote:
>>>>
>>>>
>>>>> On Apr 19, 2017, at 8:38 AM, Mandy Chung <[hidden email]> wrote:
>>>>>
>>>>> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check.  Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports.
>>>>>
>>>>> The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules.
>>>>
>>>> This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services?  
>>>
>>> It's unqualified and no restriction in this change.
>>
>> The public methods currently in jdk.vm.ci.services are:
>>
>> 1. JVMCIServiceLocator.getProvider(Class<S>)
>> 2. JVMCIServiceLocator.getProviders(Class<S>)
>> 3. Services.initializeJVMCI()
>> 4. Services.getSavedProperties()
>> 5. Services.exportJVMCITo(Class<?>)
>> 6. Services.load(Class<S>)
>> 7. Services.loadSingle(Class<S>, boolean)
>>
>> 1 should be made protected. I'll update the webrev with this change.
>
> Good.
>
>>
>> 2 should check for JVMCIPermission. I'll update the webrev with this change.
>
> Good.
>
>>
>> 3 is harmless from a security perspective in my opinion.
>
> Would be good if one of Oracle’s security engineers could take a quick look just to be sure.

Vladimir, can you please bring this to the attention of the relevant engineer.

>>
>> 4 checks for JVMCIPermission.
>
> Ok.
>
>>
>> 5, 6 and 7 will be removed in a follow bug that updates Graal from upstream (and removes its usage of these methods).
>
> About this, will this Graal update happen for JDK 9?

Yes.

>  It’s awfully late in the cycle...

These are jigsaw related changes and I've been told jigsaw has an FC exception (although I don't exactly know what that is).

-Doug
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle
I've updated http://cr.openjdk.java.net/~dnsimon/8177845/hotspot/ with these changes:

1. JVMCIServiceLocator.getProvider(Class<S>) is now protected
2. JVMCIServiceLocator.getProviders(Class<S>) now checks JVMCIPermission
3. Rename: jdk.vm.ci.services.internal.JDK9 -> jdk.vm.ci.services.internal.ReflectionAccessJDK

-Doug

> On 19 Apr 2017, at 23:12, Doug Simon <[hidden email]> wrote:
>
>>
>> On 19 Apr 2017, at 21:40, Christian Thalinger <[hidden email]> wrote:
>>
>>>
>>> On Apr 19, 2017, at 9:27 AM, Doug Simon <[hidden email]> wrote:
>>>
>>>>
>>>> On 19 Apr 2017, at 21:04, Mandy Chung <[hidden email]> wrote:
>>>>
>>>>
>>>>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger <[hidden email]> wrote:
>>>>>
>>>>>
>>>>>> On Apr 19, 2017, at 8:38 AM, Mandy Chung <[hidden email]> wrote:
>>>>>>
>>>>>> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check.  Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports.
>>>>>>
>>>>>> The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules.
>>>>>
>>>>> This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services?  
>>>>
>>>> It's unqualified and no restriction in this change.
>>>
>>> The public methods currently in jdk.vm.ci.services are:
>>>
>>> 1. JVMCIServiceLocator.getProvider(Class<S>)
>>> 2. JVMCIServiceLocator.getProviders(Class<S>)
>>> 3. Services.initializeJVMCI()
>>> 4. Services.getSavedProperties()
>>> 5. Services.exportJVMCITo(Class<?>)
>>> 6. Services.load(Class<S>)
>>> 7. Services.loadSingle(Class<S>, boolean)
>>>
>>> 1 should be made protected. I'll update the webrev with this change.
>>
>> Good.
>>
>>>
>>> 2 should check for JVMCIPermission. I'll update the webrev with this change.
>>
>> Good.
>>
>>>
>>> 3 is harmless from a security perspective in my opinion.
>>
>> Would be good if one of Oracle’s security engineers could take a quick look just to be sure.
>
> Vladimir, can you please bring this to the attention of the relevant engineer.
>
>>>
>>> 4 checks for JVMCIPermission.
>>
>> Ok.
>>
>>>
>>> 5, 6 and 7 will be removed in a follow bug that updates Graal from upstream (and removes its usage of these methods).
>>
>> About this, will this Graal update happen for JDK 9?
>
> Yes.
>
>> It’s awfully late in the cycle...
>
> These are jigsaw related changes and I've been told jigsaw has an FC exception (although I don't exactly know what that is).
>
> -Doug

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Vladimir Kozlov
Hotspot changes looks good to me.

Thanks,
Vladimir

On 4/19/17 2:26 PM, Doug Simon wrote:

> I've updated http://cr.openjdk.java.net/~dnsimon/8177845/hotspot/ with these changes:
>
> 1. JVMCIServiceLocator.getProvider(Class<S>) is now protected
> 2. JVMCIServiceLocator.getProviders(Class<S>) now checks JVMCIPermission
> 3. Rename: jdk.vm.ci.services.internal.JDK9 -> jdk.vm.ci.services.internal.ReflectionAccessJDK
>
> -Doug
>
>> On 19 Apr 2017, at 23:12, Doug Simon <[hidden email]> wrote:
>>
>>>
>>> On 19 Apr 2017, at 21:40, Christian Thalinger <[hidden email]> wrote:
>>>
>>>>
>>>> On Apr 19, 2017, at 9:27 AM, Doug Simon <[hidden email]> wrote:
>>>>
>>>>>
>>>>> On 19 Apr 2017, at 21:04, Mandy Chung <[hidden email]> wrote:
>>>>>
>>>>>
>>>>>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger <[hidden email]> wrote:
>>>>>>
>>>>>>
>>>>>>> On Apr 19, 2017, at 8:38 AM, Mandy Chung <[hidden email]> wrote:
>>>>>>>
>>>>>>> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check.  Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports.
>>>>>>>
>>>>>>> The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules.
>>>>>>
>>>>>> This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services?
>>>>>
>>>>> It's unqualified and no restriction in this change.
>>>>
>>>> The public methods currently in jdk.vm.ci.services are:
>>>>
>>>> 1. JVMCIServiceLocator.getProvider(Class<S>)
>>>> 2. JVMCIServiceLocator.getProviders(Class<S>)
>>>> 3. Services.initializeJVMCI()
>>>> 4. Services.getSavedProperties()
>>>> 5. Services.exportJVMCITo(Class<?>)
>>>> 6. Services.load(Class<S>)
>>>> 7. Services.loadSingle(Class<S>, boolean)
>>>>
>>>> 1 should be made protected. I'll update the webrev with this change.
>>>
>>> Good.
>>>
>>>>
>>>> 2 should check for JVMCIPermission. I'll update the webrev with this change.
>>>
>>> Good.
>>>
>>>>
>>>> 3 is harmless from a security perspective in my opinion.
>>>
>>> Would be good if one of Oracle’s security engineers could take a quick look just to be sure.
>>
>> Vladimir, can you please bring this to the attention of the relevant engineer.
>>
>>>>
>>>> 4 checks for JVMCIPermission.
>>>
>>> Ok.
>>>
>>>>
>>>> 5, 6 and 7 will be removed in a follow bug that updates Graal from upstream (and removes its usage of these methods).
>>>
>>> About this, will this Graal update happen for JDK 9?
>>
>> Yes.
>>
>>> It’s awfully late in the cycle...
>>
>> These are jigsaw related changes and I've been told jigsaw has an FC exception (although I don't exactly know what that is).
>>
>> -Doug
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Vladimir Kozlov
In reply to this post by Doug Simon @ Oracle
Doug,

Can you point (link) particular code which needs to be reviewed? And
what security issues could be?

Thanks,
Vladimir

On 4/19/17 2:12 PM, Doug Simon wrote:
>> 3. Services.initializeJVMCI()

>>> 3 is harmless from a security perspective in my opinion.
>> Would be good if one of Oracle’s security engineers could take a quick look just to be sure.
> Vladimir, can you please bring this to the attention of the relevant engineer.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Andrew Haley
In reply to this post by Christian Thalinger-4
On 19/04/17 19:55, Christian Thalinger wrote:
> This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services?  After all, this is a security issue.

Does that make any sense?  Surely the *user* of a system can call Truffle,
and therefore can call JMCI.  I thought that was the idea.  If you can call
Truffle from your own code, why shouldn't you be able to call JVMCI from
your own compiler?

Andrew.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle
In reply to this post by Vladimir Kozlov

> On 20 Apr 2017, at 00:31, Vladimir Kozlov <[hidden email]> wrote:
>
> Doug,
>
> Can you point (link) particular code which needs to be reviewed? And what security issues could be?

I believe Chris had possible concerns with Services.initializeJVMCI() (at http://cr.openjdk.java.net/~dnsimon/8177845/hotspot/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.services/src/jdk/vm/ci/services/Services.java.udiff.html). As previously stated, I can't see any security issues with this method which is why it doesn't check JVMCIPermission.

-Doug

>
> Thanks,
> Vladimir
>
> On 4/19/17 2:12 PM, Doug Simon wrote:
>>> 3. Services.initializeJVMCI()
>
>>>> 3 is harmless from a security perspective in my opinion.
>>> Would be good if one of Oracle’s security engineers could take a quick look just to be sure.
>> Vladimir, can you please bring this to the attention of the relevant engineer.
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle
In reply to this post by Andrew Haley

> On 20 Apr 2017, at 09:46, Andrew Haley <[hidden email]> wrote:
>
> On 19/04/17 19:55, Christian Thalinger wrote:
>> This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services?  After all, this is a security issue.
>
> Does that make any sense?  Surely the *user* of a system can call Truffle,
> and therefore can call JMCI.

Truffle is completely separate from JVMCI. Only the Graal implementation of TruffleRuntime knows about JVMCI.

>  I thought that was the idea.  If you can call
> Truffle from your own code, why shouldn't you be able to call JVMCI from
> your own compiler?

We may need to add some doPrivileged sections to Graal at some point ensure everything works when running Truffle under a security manager.

-Doug
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Andrew Haley
On 20/04/17 08:56, Doug Simon wrote:

>
>> On 20 Apr 2017, at 09:46, Andrew Haley <[hidden email]> wrote:
>>
>> On 19/04/17 19:55, Christian Thalinger wrote:
>>> This all makes sense but where is the restriction that only
>>> jdk.internal.vm.compiler can use jdk.vm.ci.services?  After all,
>>> this is a security issue.
>>
>> Does that make any sense?  Surely the *user* of a system can call Truffle,
>> and therefore can call JMCI.
>
> Truffle is completely separate from JVMCI. Only the Graal
> implementation of TruffleRuntime knows about JVMCI.

Yes, I am aware of that.  :-)

>> I thought that was the idea.  If you can call Truffle from your own
>> code, why shouldn't you be able to call JVMCI from your own
>> compiler?
>
> We may need to add some doPrivileged sections to Graal at some point
> ensure everything works when running Truffle under a security
> manager.

That would be courageous.  But what Christian was saying, I believe,
was that JVMCI could be restricted to only being available from
jdk.internal.vm.compiler.

Andrew.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle

> On 20 Apr 2017, at 10:01, Andrew Haley <[hidden email]> wrote:
>
> On 20/04/17 08:56, Doug Simon wrote:
>>
>>> On 20 Apr 2017, at 09:46, Andrew Haley <[hidden email]> wrote:
>>>
>>> On 19/04/17 19:55, Christian Thalinger wrote:
>>>> This all makes sense but where is the restriction that only
>>>> jdk.internal.vm.compiler can use jdk.vm.ci.services?  After all,
>>>> this is a security issue.
>>>
>>> Does that make any sense?  Surely the *user* of a system can call Truffle,
>>> and therefore can call JMCI.
>>
>> Truffle is completely separate from JVMCI. Only the Graal
>> implementation of TruffleRuntime knows about JVMCI.
>
> Yes, I am aware of that.  :-)

Ok, sorry for being confused by your statement ;-)

>>> I thought that was the idea.  If you can call Truffle from your own
>>> code, why shouldn't you be able to call JVMCI from your own
>>> compiler?
>>
>> We may need to add some doPrivileged sections to Graal at some point
>> ensure everything works when running Truffle under a security
>> manager.
>
> That would be courageous.  But what Christian was saying, I believe,
> was that JVMCI could be restricted to only being available from
> jdk.internal.vm.compiler.

Ah, ok. That would go against the move to make jdk.internal.vm.compiler an upgradeable module as there should not be any qualified exports to an upgradeable module. For this reason, the security of JVMCI relies on exporting as little of the API as possible and having JVMCIPermission checks on the exported methods where necessary.

-Doug
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Christian Thalinger-4

On Apr 19, 2017, at 10:34 PM, Doug Simon <[hidden email]> wrote:


On 20 Apr 2017, at 10:01, Andrew Haley <[hidden email]> wrote:

On 20/04/17 08:56, Doug Simon wrote:

On 20 Apr 2017, at 09:46, Andrew Haley <[hidden email]> wrote:

On 19/04/17 19:55, Christian Thalinger wrote:
This all makes sense but where is the restriction that only
jdk.internal.vm.compiler can use jdk.vm.ci.services?  After all,
this is a security issue.

Does that make any sense?  Surely the *user* of a system can call Truffle,
and therefore can call JMCI.

Truffle is completely separate from JVMCI. Only the Graal
implementation of TruffleRuntime knows about JVMCI.

Yes, I am aware of that.  :-)

Ok, sorry for being confused by your statement ;-)

I thought that was the idea.  If you can call Truffle from your own
code, why shouldn't you be able to call JVMCI from your own
compiler?

We may need to add some doPrivileged sections to Graal at some point
ensure everything works when running Truffle under a security
manager.

That would be courageous.  But what Christian was saying, I believe,
was that JVMCI could be restricted to only being available from
jdk.internal.vm.compiler.

Ah, ok. That would go against the move to make jdk.internal.vm.compiler an upgradeable module as there should not be any qualified exports to an upgradeable module. For this reason, the security of JVMCI relies on exporting as little of the API as possible and having JVMCIPermission checks on the exported methods where necessary.

If that’s the case then I’m fine.  We just have to make sure all JVMCIPermission checks are in place.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle
In reply to this post by Doug Simon @ Oracle

> On 19 Apr 2017, at 22:29, Vladimir Kozlov <[hidden email]> wrote:
>
> ReflectionAccessJDK ? Based on your comment in the file.
>
> Vladimir
>
> On 4/19/17 1:02 PM, Doug Simon wrote:
>> Sure - how about good old Util? ;-) I'm open to other suggestions.
>>
>> Sent from my iPhone
>>
>>> On Apr 19, 2017, at 9:46 PM, Vladimir Kozlov <[hidden email]> wrote:
>>>
>>> Hi Doug,
>>>
>>> Can you consider using other name and not JDK9 for new JVMCI class? It will be used in JDK 10 too:
>>>
>>> jdk.vm.ci.services.internal.JDK9;
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>> On 4/18/17 3:13 PM, Doug Simon wrote:
>>>> Please review these changes that make jdk.internal.vm.compiler an upgradable compiler.
>>>> The primary requirement for this is to remove all usage of JDK internals from Graal.
>>>> While this most involves changes in Graal, there remain a few capabilities that must
>>>> be exposed via JVMCI. Namely:
>>>>
>>>> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal initialize
>>>>  can use system properties that are guaranteed not to have been modified by application
>>>>  code (Graal initialization is lazy and may occur after application code has been run).
>>>>
>>>> 2. Truffle needs to be able to trigger JVMCI initialization so that it can select the Graal
>>>>  optimized implementation of the Truffle runtime.
>>>>
>>>> These capabilities have been added to jdk.vm.ci.services.Services. A comment has
>>>> also been added to this class to denote the current methods to be removed
>>>> in a subsequent bug to update the JDK copy of Graal with the upstream version which
>>>> no longer uses the methods. The methods destined for removal are:
>>>>
>>>> exportJVMCITo(Class<?> requestor)
>>>> load(Class<S> service)
>>>> loadSingle(Class<S> service, boolean required)
>>>>
>>>> The first one is no longer needed as JVMCI exports itself to Graal service providers
>>>> it loads via private API and Graal re-exports[1] JVMCI to any module the extends Graal.
>>>> The latter 2 are no longer required as Graal uses the standard ServiceLoader. This API
>>>> is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
>>>>
>>>> These changes have been tested against upstream Graal. To make the Graal tests pass, 2 extra
>>>> minor changes were required:
>>>>
>>>> 1. In JDK-8177663, HotSpotMemoryAccessProviderImpl::checkRead was added and throws IllegalArgumentException
>>>>  on all error paths except one which throws AssertionError. The latter was a mistake and has been
>>>>  fixed to also throw IllegalArgumentException.
>>>> 2. An invalid assertion has been removed from HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
>>>>  Up to JDK 8, it was true that all classes in java.* packages were loaded by the boot class loader.
>>>>  This is no longer true in JDK 9 with classes like java.sql being loaded by platform class loader.
>>>>
>>>> As hinted above, a subsequent bug will be required to pull in the latest upstream Graal and
>>>> remove use of JDK internal API from the jdk.aot module. The qualified exports to
>>>> jdk.vm.internal.compiler and jdk.aot can then be removed.
>>>>
>>>> -Doug
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8177845
>>>> http://cr.openjdk.java.net/~dnsimon/8177845/
>>>>
>>>> [1] http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
>>>>
>>>>
>>>>
>>

12
Loading...