RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]

[Thomas Stuefe]

On Wed, 9 Mar 2022 08:35:41 GMT, Johannes Bechberger <duke at openjdk.java.net> wrote:

>> The WXMode for the current thread (on MacOS aarch64) is currently stored in the thread class which is unnecessary as the WXMode is bound to the current OS thread, not the current instance of the thread class.
>> This pull request moves the storage of the current WXMode into a thread local global variable in `os` and changes all related code. SafeFetch depended on the existence of a thread object only because of the WXMode. This pull request therefore removes the dependency, making SafeFetch usable in more contexts.
>
> Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
> 
>   current_thread_wx -> ThreadWX

Hi Anton,

thanks a lot for your explanations. You made some things clearer to me. My answers are inline.

> > Why don't we just switch it on once, for a thread that conceivably may call into generated code, and be done with? Why is this fine granular switching even needed? I find it difficult to imagine an attack vector that exploits having this always enabled for a thread. After all, we have to mark code cache with MAP_JIT already, so it is not like we can execute arbitrary memory ranges.
> 
> A java thread executes the code (interpreter, JIT) and changes the code (e.g. it could make a nmethod nonentrant, change inline cache). Code modifications are done in VM (runtime) call. So WX state is tied to the java thread state. The WX management is done more to satisfy the platform requirement than to make the system more secure.

Okay, that was the piece I was missing. I was assuming that we have either executing or modifying threads and that a thread was either one or the other (compiler threads compile, java threads run). You are saying that Java Threads may write too. And CompilerThreads, in the case of SafeFetch at least, may run generated code. So switching has to be done as a stack mark. Have I gotten this right?

> 
> > Related to that, how much does this call cost? Is this a runtime call into the pthread library or does it get inlined somehow? Because things like SafeFetch are trimmed to be super cheap if the memory can be accessed. Doing a pthread call on every invocation may throw off the cost-benefit ratio.
> 
> SafeFetch is much more expensive compared the direct memory access. So I assume it's used only when the real chance exists the access may fail, and the average cost of SafeFetch is much higher than the best case.

Yes, we only do this when necessary, but it is supposed to be reasonably cheap if memory is accessible. Its Load (the safefetch blob) -> unconditional jump to the blob -> load target memory -> jump back. Depending on what the pthread library call does, and if it's a real function call into a library, it would be more expensive than that.

> 
> Yes, WX management is offered via a pthread function call. I haven't investigated if the native compiler can inline the call. The WX management itself considerably cheap [#2200 (comment)](https://github.com/openjdk/jdk/pull/2200#issuecomment-773382787).
> 
> > Why and where do we need nesting? This would be so much easier if we could just not care.
> 
> We swtich the state to WXWrite at the entry in VM call, but a VM call may do another VM call. E.g. a runtime VM calls the JNI GetLongField. So GetLongField could be called from a code executing in Java (WXExec) and VM (WXWrite) states, the WX state should be restored back on leaving JNI function. The original state is tracked in Thread.

Okay, thanks again for clarifying.

> > Arguably we should restore, upon leaving the VM, the state that has been present before. Because a native thread may already have modified the wx state and overruling it is just rude. But I offhand don't see a way to do this since we have no way (?) to query the current state.
> 
> How in general safe to call SafeFetch on a native thread that has no Thread object?

SafeFetch is safe to call if the stub routine exists. So it is safe after VM initialization. And that can be tested too. Callers, when in doubt, are encouraged to use `CanUseSafeFetch` to check if VM is still in pre-initialization time. `CanUseSafeFetch` + `SafeFetch` should never crash, regardless of when and by whom it was called. We also have `os::is_readable_pointer()`, which wraps these two calls for convenience.

> The JVM has not initialized the thread, so there could be no JVM signal handler installed. Or using libjsig is mandatory for this case?

As David wrote, the Signal handler is per process. It is set as part of VM initialization before SafeFetch blob is generated. Foreign threads still enter the signal handler. So crashes in foreign threads still generate hs-err reports. Depending on how your support is organized that's either a bug or a feature :)

> 
> I also don't know a good way to query the WX state.
> 
> > It also would be slightly faster. Using Thread, we'd access TLS to get Thread::current, then dereference that to read the wx state . OTOH using OS TLS, we access TLS to get the wx state directly. We save one dereference.
> 
> If we compare approaches in general (not only SafeFetch), the Thread is already in hands, so we should to compare a read of the field from a C++ object, and the read of a TLS variable. The former could not be slower than the latter.

You lost me here. To me most of the invocations of `ThreadWXEnable` seem to use `Thread::current()`. Only those who retrieve the thread from the JNI environment don't.

IIRC, TLS, at least on Linux, lives at the front of the thread stack, so accessing it should be quite cheap.

I see the performance point of an option to pass in Thread* in case one already has it. I dislike it a bit because it gives the illusion that you could pass in arbitrary threads when in fact you must pass in Thread::current. But an assertion could help clarifying here.

> 
> Is it possible to change SafeFetch only? Switch to WXExec before calling the stub and switch WXWrite back unconditionally? We won't need to provide assert in ThreadWXEnable. But SafeFetch can check the assumption with assert via Thread, if it exists.

But SafeFetch could be used from outside code as well as VM code. In case of the latter, prior state can either be WXWrite or WXExec. It needs to restore the prior state after the call.

---

To summarize the different proposals:

- you propose to use Thread* when available and assume WXWrite as prior state when not. You argue that if there is no Thread::current, we are not a VM thread and we should need no nesting, so a simple switchback to wxwrite should suffice after leaving SafeFetch, right?
- Johannes proposes to use TLS, and just always support nesting, regardless of who calls.

What I like about Johannes proposal is that its simple. It has fewer dependencies on VM infrastructure and we can mostly just hide it in the platform tree.

Cheers, Thomas

-------------

PR: https://git.openjdk.java.net/jdk/pull/7727