Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On 12/03/2022 2:37 am, Anton Kozlov wrote: On Thu, 10 Mar 2022 18:04:50 GMT, Thomas Stuefe wrote: blocking SIGSEGV and SIGBUS - or other synchronous error signals like SIGFPE - and then triggering said signal is UB. What happens is OS-dependent. I saw processes vanishing, or hang, or core. It makes sense, since what is the kernel supposed to do. It cannot deliver the signal, and deferring it would require returning to the faulting instruction, that would just re-fault. For some more details see e.g. https://bugs.openjdk.java.net/browse/JDK-8252533 This UB looks reasonable. My point is that a native thread would run fine with SIGSEGV blocked. But then JVM decides it can do SafeFetch, and things gets nasty. Is there a crash that is fixed by the change? I just spotted it is an enhancement, not a bug. Just trying to understand the problem. Yes, this issue is a breakout from https://bugs.openjdk.java.net/browse/JDK-8282306, where we'd like to use SafeFetch to make stack walking in AsyncGetCallTrace more robust. AGCT is called from the signal handler, and it may run in any number of situations (e.g. in foreign threads, or threads which are in the process of getting dismantled, etc). I mean, some way to verify the issue is fixed, e.g. a test that does not fail anymore. I see AsyncGetCallTrace to assume the JavaThread very soon, or do I look at the wrong place? https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/forte.cpp#L569 It is up to the agent setting things up for AGCT to only actually call it for JavaThreads. David - Another situation is error handling itself. When writing an hs-err file, we use SafeFetch to do carefully tiptoe around the possibly corrupt VM state. If the original crash happened in a foreign thread, we still want some of these reports to work (e.g. dumping register content or printing stacks). So SafeFetch should be as robust as possible. OK, thanks. I think we also handle recursive segfaults recover after interpretation of the corrupted VM state. Otherwise, implementing the printing functions would be too tedious and hard with SafeFetch alone. But I see it's used in printing register content, at least. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Sat, 12 Mar 2022 12:30:39 GMT, Andrew Haley wrote: > 1ns Incidentally, there must be a lot of speculation and bypassing going on there. I can see 15 cycles of latency, probably 20, so that'd be more like 5ns start to finish. M1 is a remarkable thing. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Fri, 11 Mar 2022 12:18:36 GMT, Florian Weimer wrote: > > According to > > https://forums.swift.org/t/concurrencys-use-of-thread-local-variables/48654: > > "these accesses are just a move from a system register plus a load/store > > at a constant offset." > > Ideally you'd still benchmark that. Some AArch64 implementations have really, > really slow moves from the system register used as the thread pointer. > Hopefully Apple's implementation isn't in that category. In a tight loop, loads from __thread variables take 1ns. It's this: 0x18ea1c530 <+0>: ldrx16, [x0, #0x8] 0x18ea1c534 <+4>: mrsx17, TPIDRRO_EL0 0x18ea1c538 <+8>: andx17, x17, #0xfff8 0x18ea1c53c <+12>: ldrx17, [x17, x16, lsl #3] 0x18ea1c540 <+16>: cbzx17, 0x18ea1c550 ; only executed first time 0x18ea1c544 <+20>: ldrx16, [x0, #0x10] 0x18ea1c548 <+24>: addx0, x17, x16 0x18ea1c54c <+28>: ret ... which looks the same as what glibc does. Not bad, but quite a lot more to do than a simple load. I'd still use a static SafeFetch, with no W^X fiddling. It just seems to me much more reasonable. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Fri, 11 Mar 2022 16:34:29 GMT, Anton Kozlov wrote: > > blocking SIGSEGV and SIGBUS - or other synchronous error signals like > > SIGFPE - and then triggering said signal is UB. What happens is > > OS-dependent. I saw processes vanishing, or hang, or core. It makes sense, > > since what is the kernel supposed to do. It cannot deliver the signal, and > > deferring it would require returning to the faulting instruction, that > > would just re-fault. > > For some more details see e.g. > > https://bugs.openjdk.java.net/browse/JDK-8252533 > > This UB looks reasonable. My point is that a native thread would run fine > with SIGSEGV blocked. But then JVM decides it can do SafeFetch, and things > gets nasty. Blocking synchronous error signals makes zero sense even for normal programs, since you lose the ability to get cores. For the JVM in particular, it also blocks facilities like polling pages, or dynamically querying CPU abilities. So a JVM would not even start with synchronous error signals blocked. > > > > Is there a crash that is fixed by the change? I just spotted it is an > > > enhancement, not a bug. Just trying to understand the problem. > > > > > > Yes, this issue is a breakout from > > https://bugs.openjdk.java.net/browse/JDK-8282306, where we'd like to use > > SafeFetch to make stack walking in AsyncGetCallTrace more robust. AGCT is > > called from the signal handler, and it may run in any number of situations > > (e.g. in foreign threads, or threads that are in the process of getting > > dismantled, etc). > > I mean, some way to verify the issue is fixed, e.g. a test that does not fail > anymore. No, tests do not exist. Unfortunately, otherwise this regression would have been detected right away and we would not need this PR. We have a test though that tests SafeFetch during error handling. That test can be tweaked for this purpose. So, test does not exist yet, but can be easily written. > > I see AsyncGetCallTrace to assume the JavaThread very soon, or do I look at > the wrong place? > https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/forte.cpp#L569 > > > Another situation is error handling itself. When writing an hs-err file, we > > use SafeFetch to do carefully tiptoe around the possibly corrupt VM state. > > If the original crash happened in a foreign thread, we still want some of > > these reports to work (e.g. dumping register content or printing stacks). > > So SafeFetch should be as robust as possible. > > OK, thanks. I think we also handle recursive segfaults recover after > interpretation of the corrupted VM state. Otherwise, implementing the > printing functions would be too tedious and hard with SafeFetch alone. But I > see it's used in printing register content, at least. Secondary error handling is a very coarse-grained tool. If an error reporting step crashes out, we continue with the next step. Has disadvantages though. The total number of retries is very limited. And a faulting error reporting step still hurts, because its report is compromised. E.g. if the call stack printing crashes out, we have no call stack. This is not an abstract problem. Its a very concrete and typical problem. I spend a large part of my work with hs-err reports. They are of very high importance to us. We (SAP) have invested a lot of time and effort in hardening out OpenJDK error reporting, and SafeFetch is an important part of that. For example, we provided the facility that made SafeFetch usable in signal handling. It would be nice if our work was not compromised. Please let us find a way forward here. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Thu, 10 Mar 2022 18:04:50 GMT, Thomas Stuefe wrote: > blocking SIGSEGV and SIGBUS - or other synchronous error signals like SIGFPE > - and then triggering said signal is UB. What happens is OS-dependent. I saw > processes vanishing, or hang, or core. It makes sense, since what is the > kernel supposed to do. It cannot deliver the signal, and deferring it would > require returning to the faulting instruction, that would just re-fault. > For some more details see e.g. > https://bugs.openjdk.java.net/browse/JDK-8252533 This UB looks reasonable. My point is that a native thread would run fine with SIGSEGV blocked. But then JVM decides it can do SafeFetch, and things gets nasty. > > Is there a crash that is fixed by the change? I just spotted it is an > > enhancement, not a bug. Just trying to understand the problem. > > Yes, this issue is a breakout from > https://bugs.openjdk.java.net/browse/JDK-8282306, where we'd like to use > SafeFetch to make stack walking in AsyncGetCallTrace more robust. AGCT is > called from the signal handler, and it may run in any number of situations > (e.g. in foreign threads, or threads which are in the process of getting > dismantled, etc). I mean, some way to verify the issue is fixed, e.g. a test that does not fail anymore. I see AsyncGetCallTrace to assume the JavaThread very soon, or do I look at the wrong place? https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/forte.cpp#L569 > Another situation is error handling itself. When writing an hs-err file, we > use SafeFetch to do carefully tiptoe around the possibly corrupt VM state. If > the original crash happened in a foreign thread, we still want some of these > reports to work (e.g. dumping register content or printing stacks). So > SafeFetch should be as robust as possible. OK, thanks. I think we also handle recursive segfaults recover after interpretation of the corrupted VM state. Otherwise, implementing the printing functions would be too tedious and hard with SafeFetch alone. But I see it's used in printing register content, at least. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Thu, 10 Mar 2022 18:07:37 GMT, Thomas Stuefe wrote: > > > > 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. > > > > > > I'm not sure I understand what is the "outside code". The SafeFetch is the > > private hotspot function, it cannot be linked with non-JVM code, isn't it? > > Sorry for being imprecise. I meant SafeFetch is triggered from within a > signal handler that runs on a foreign thread. E.g. AGCT or error handling. Then the OS TLS way is not better since when the signal handler and SafeFetch start, the state is unknown and is only assumed to be Write (in initialization of TLS variable). - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Fri, 11 Mar 2022 09:50:22 GMT, Johannes Bechberger wrote: > According to > https://forums.swift.org/t/concurrencys-use-of-thread-local-variables/48654: > "these accesses are just a move from a system register plus a load/store at a > constant offset." Ideally you'd still benchmark that. Some AArch64 implementations have really, really slow moves from the system register used as the thread pointer. Hopefully Apple's implementation isn't in that category. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Fri, 11 Mar 2022 09:33:40 GMT, Andrew Haley wrote: > We could also redefine SafeFetch on MacOS/AArch64 to not need WX. We could do > this by statically generating SafeFetch on that platform, and it wouldn't be > in the JIT region at all. Why not just do that? Do you mean using inline assembly? - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Fri, 11 Mar 2022 09:33:40 GMT, Andrew Haley wrote: > But we don't need to speculate. If thread-local variables are cheap on MacOS, > and there is no reason why they should be expensive, then we can stop > worrying and just use a thread-local variable for WX state. We can measure > how long it takes, and we only have to care about one platform, MacOS/AArch64. According to https://forums.swift.org/t/concurrencys-use-of-thread-local-variables/48654: "these accesses are just a move from a system register plus a load/store at a constant offset." - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Thu, 10 Mar 2022 14:09:24 GMT, Anton Kozlov wrote: > > 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, unfortunately we need something like this. But we don't need to speculate. If thread-local variables are cheap on MacOS, and there is no reason why they should be expensive, then we can stop worrying and just use a thread-local variable for WX state. We can measure how long it takes, and we only have to care about one platform, MacOS/AArch64. We could also redefine SafeFetch on MacOS/AArch64 to not need WX. We could do this by statically generating SafeFetch on that platform, and it wouldn't be in the JIT region at all. Why not just do that? - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Wed, 9 Mar 2022 08:35:41 GMT, Johannes Bechberger 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 I'm also personally in favour of packaging it in the OS specific header. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Wed, 9 Mar 2022 08:35:41 GMT, Johannes Bechberger 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 The refactoring is a consequence of the initial TLS change as it pollutes the shared os "namespace" unnecessarily. So I'd want to see this neatly packaged where it belongs please. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Thu, 10 Mar 2022 14:09:24 GMT, Anton Kozlov wrote: > > > 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. > > I'm not sure I understand what is the "outside code". The SafeFetch is the > private hotspot function, it cannot be linked with non-JVM code, isn't it? Sorry for being imprecise. I meant SafeFetch is triggered from within a signal handler that runs on a foreign thread. E.g. AGCT or error handling. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Thu, 10 Mar 2022 12:31:06 GMT, Anton Kozlov wrote: > > Signal handlers are per-process not per-thread, so a thread need not be > > attached to the VM for our signal handlers to get involved - that is why > > they call Thread::current_or_null_safe(). > > Oh, right, thanks. I was too concentrated on thinking another platforms like > windows, that missed the example will work for *nix. > > A general question to the bug. The signal mask is per-thread, and a native > thread may block the JVM signal. I think safefetch will fail, if somehow we > manage to call it on this thread (without jsig). So I'm not sure the > safefetch is really safe on all platforms and in all contexts, that is, it > always recovers from the read failure if called on a random thread. To expand on @dholmes-ora answer: blocking SIGSEGV and SIGBUS - or other synchronous error signals like SIGFPE - and then triggering said signal is UB. What happens is OS-dependent. I saw processes vanishing, or hang, or core. It makes sense, since what is the kernel supposed to do. It cannot deliver the signal, and deferring it would require returning to the faulting instruction, that would just re-fault. For some more details see e.g. https://bugs.openjdk.java.net/browse/JDK-8252533 > > Is there a crash that is fixed by the change? I just spotted it is an > enhancement, not a bug. Just trying to understand the problem. Yes, this issue is a breakout from https://bugs.openjdk.java.net/browse/JDK-8282306, where we'd like to use SafeFetch to make stack walking in AsyncGetCallTrace more robust. AGCT is called from the signal handler, and it may run in any number of situations (e.g. in foreign threads, or threads which are in the process of getting dismantled, etc). Another situation is error handling itself. When writing an hs-err file, we use SafeFetch to do carefully tiptoe around the possibly corrupt VM state. If the original crash happened in a foreign thread, we still want some of these reports to work (e.g. dumping register content or printing stacks). So SafeFetch should be as robust as possible. > I'm not opposing the refactoring, it has some advantages, but I'd want to > separate functional change from the refactoring. This would aid backporting, > at least. I agree that a minimal patch would be good. I feel partly guilty for this expanding discussion. I'm fine with a minimal change, without any refactoring, in whatever form @parttimenerd choses - be it OS thread local or the approach proposed by you, @AntonKozlov. Cheers Thomas - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Thu, 10 Mar 2022 12:31:06 GMT, Anton Kozlov wrote: > The signal mask is per-thread, and a native thread may block the JVM signal. @AntonKozlov the signal from safefetch (if it is not safe) is a SIGSEGV or SIGBUS. If these signals happen to be blocked and we raise the signal synchronously then we are in undefined behaviour territory. So I guess in that sense yes safefetch is not guaranteed to be safe. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Thu, 10 Mar 2022 12:41:11 GMT, Thomas Stuefe wrote: > 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? Right. > 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, unfortunately we need something like this. > > 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. > > 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. Right, JNI env is used e.g. in interfaceSupport.hpp where the most VM entries are defined. I found only few instances of ThreadWXEnable to receive Thread::current() as the argument immediately. In the rest, the Thread is there somewhere in the context. > > 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. There is the assert in Thread::enable_wx, where the implementation actually is unable to handle anything except the current threads. > > 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. I'm not sure I understand what is the "outside code". The SafeFetch is the private hotspot function, it cannot be linked with non-JVM code, isn't it? > 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? So far I like the another approach more, that is, always assume WXWrite and use the Thread only to assert the state. But I did not understand your concern above. > * 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. I'm not opposing the refactoring, it has some advantages, but I'd want to separate functional change from the refactoring. This would aid backporting, at least. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Wed, 9 Mar 2022 08:35:41 GMT, Johannes Bechberger 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
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Thu, 10 Mar 2022 10:17:31 GMT, David Holmes wrote: > Signal handlers are per-process not per-thread, so a thread need not be > attached to the VM for our signal handlers to get involved - that is why they > call Thread::current_or_null_safe(). Oh, right, thanks. I was too concentrated on thinking another platforms like windows, that missed the example will work for *nix. A general question to the bug. The signal mask is per-thread, and a native thread may block the JVM signal. I think safefetch will fail, if somehow we manage to call it on this thread (without jsig). So I'm not sure the safefetch is really safe on all platforms and in all contexts, that is, it always recovers from the read failure if called on a random thread. Is there a crash that is fixed by the change? I just spotted it is an enhancement, not a bug. Just trying to understand the problem. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Thu, 10 Mar 2022 07:31:47 GMT, Anton Kozlov wrote: > How in general safe to call SafeFetch on a native thread that has no Thread > object? The JVM has not initialized the thread, so there could be no JVM > signal handler installed. @AntonKozlov Signal handlers are per-process not per-thread, so a thread need not be attached to the VM for our signal handlers to get involved - that is why they call `Thread::current_or_null_safe()`. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Thu, 10 Mar 2022 07:09:27 GMT, Anton Kozlov wrote: > > Interesting. But I would nonetheless create an assertion that checks that > > there is no nesting in the case without a Thread object. I would this using > > a thread local nesting counter in the ThreadWXEnable class (incremented in > > the constructor and decremented in the destructor). > > Hmm, yes. And the assert would need to check the native thread still don't > have a Thread object, i.e. the either Thread or TLS is used for tracking. > This looks rather complicated, I agree. 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. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Thu, 10 Mar 2022 06:00:42 GMT, Thomas Stuefe wrote: > 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? The JVM has not initialized the thread, so there could be no JVM signal handler installed. Or using libjsig is mandatory for this case? 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. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Wed, 9 Mar 2022 20:25:02 GMT, Johannes Bechberger wrote: > Interesting. But I would nonetheless create an assertion that checks that > there is no nesting in the case without a Thread object. I would this using a > thread local nesting counter in the ThreadWXEnable class (incremented in the > constructor and decremented in the destructor). Hmm, yes. And the assert would need to check the native thread still don't have a Thread object, i.e. the either Thread or TLS is used for tracking. This looks rather complicated, I agree. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Thu, 10 Mar 2022 05:52:48 GMT, Thomas Stuefe wrote: > 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 non entrant, change inline cache). Code modifications are done in VM (runtime) call. So WX state is tied to java thread state. The WX management is done more to satisfy the platform requirement, than to make the system more secure. > 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, 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 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. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Wed, 9 Mar 2022 19:03:16 GMT, Anton Kozlov wrote:
>> Johannes Bechberger has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> current_thread_wx -> ThreadWX
>
> https://github.com/openjdk/jdk/compare/master...478ec1a7ca2c72e5916b28613a4875aa2ee1a793
> touches more places than a targeted change in ThreadWXEnable... I'm not sure
> the real nesting is required for a thread that is not registered properly
> with VM. The initial state is always assumed for the NULL Thread. The
> SafeFetch assembly does not do up-calls to VM. I don't see why we'd need
> runtime tracking of WX state. The state is either WXExec for SafeFetch
> assembly, or unknown -- which we assume to be WXWrite regardless of approach
> taken.
>
> Nesting was implemented to reduce the amount of changes in JVM (yes, WX code
> scattered around the VM less than it could be :)), but it is possible to
> avoid runtime WX tracking if you always know the state, like we do if Thread
> == NULL.
Hi @AntonKozlov,
> [master...478ec1a](https://github.com/openjdk/jdk/compare/master...478ec1a7ca2c72e5916b28613a4875aa2ee1a793)
> touches more places than a targeted change in ThreadWXEnable... I'm not sure
> the real nesting is required for a thread that is not registered properly
> with VM.
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.
> The change proposes to assume WXWrite as the initial state. Have you
> considered to extend ThreadWXEnable to fix the assert failure? Something like
> below (I have not tried to compile though). The refactoring looks OK, but it
> makes sense to separate it from functional change.
>
> ```
> class ThreadWXEnable {
> Thread* _thread;
> WXMode _old_mode;
>
> public:
> ThreadWXEnable(WXMode new_mode, Thread* thread) :
> _thread(thread)
> {
> if (_thread) {
> _old_mode = _thread->enable_wx(new_mode);
> } else {
> os::current_thread_enable_wx(new_mode);
> _old_mode = WXWrite;
> }
> }
> ~ThreadWXEnable() {
> if (_thread) {
> _thread->enable_wx(_old_mode);
> } else {
> os::current_thread_enable_wx(_old_mode);
> }
> }
> };
> ```
I honestly don't find this easier than the solution @parttimenerd proposes,
using a OS thread local. Using an OS thread local makes this whole system
independent from Thread, so you don't need to know about Thread and don't rely
on Thread::current being present.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Wed, 9 Mar 2022 19:03:16 GMT, Anton Kozlov wrote: >> Johannes Bechberger has updated the pull request incrementally with one >> additional commit since the last revision: >> >> current_thread_wx -> ThreadWX > > https://github.com/openjdk/jdk/compare/master...478ec1a7ca2c72e5916b28613a4875aa2ee1a793 > touches more places than a targeted change in ThreadWXEnable... I'm not sure > the real nesting is required for a thread that is not registered properly > with VM. The initial state is always assumed for the NULL Thread. The > SafeFetch assembly does not do up-calls to VM. I don't see why we'd need > runtime tracking of WX state. The state is either WXExec for SafeFetch > assembly, or unknown -- which we assume to be WXWrite regardless of approach > taken. > > Nesting was implemented to reduce the amount of changes in JVM (yes, WX code > scattered around the VM less than it could be :)), but it is possible to > avoid runtime WX tracking if you always know the state, like we do if Thread > == NULL. @AntonKozlov can you give us please a bit more background about the wx state stuff? - 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. - 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. - Why and where do we need nesting? This would be so much easier if we could just not care. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Wed, 9 Mar 2022 08:35:41 GMT, Johannes Bechberger 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 Interesting. But I would nonetheless create an assertion that checks that there is no nesting in the case without a Thread object. I would this using a thread local nesting counter in the ThreadWXEnable class (incremented in the constructor and decremented in the destructor). - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Wed, 9 Mar 2022 08:35:41 GMT, Johannes Bechberger 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 https://github.com/openjdk/jdk/compare/master...478ec1a7ca2c72e5916b28613a4875aa2ee1a793 touches more places than a targeted change in ThreadWXEnable... I'm not sure the real nesting is required for a thread that is not registered properly with VM. The initial state is always assumed for the NULL Thread. The SafeFetch assembly does not do up-calls to VM. I don't see why we'd need runtime tracking of WX state. The state is either WXExec for SafeFetch assembly, or unknown -- which we assume to be WXWrite regardless of approach taken. Nesting was implemented to reduce the amount of changes in JVM (yes, WX code scattered around the VM less than it could be :)), but it is possible to avoid runtime WX tracking if you always know the state, like we do if Thread == NULL. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Wed, 9 Mar 2022 08:35:41 GMT, Johannes Bechberger 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 This probably does not work as intended, as it would prevent nesting of ThreadWXEnable before a thread exists. I already proposed a version without any refactoring (just moving the current WXMode from Thread into os near the related methods): https://github.com/openjdk/jdk/pull/7727/commits/478ec1a7ca2c72e5916b28613a4875aa2ee1a793 (state at this commit). - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Wed, 9 Mar 2022 08:35:41 GMT, Johannes Bechberger
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
The change proposes to assume WXWrite as the initial state. Have you considered
to extend ThreadWXEnable to fix the assert failure? Something like below (I
have not tried to compile though). The refactoring looks OK, but it makes sense
to separate it from functional change.
class ThreadWXEnable {
Thread* _thread;
WXMode _old_mode;
public:
ThreadWXEnable(WXMode new_mode, Thread* thread) :
_thread(thread)
{
if (_thread) {
_old_mode = _thread->enable_wx(new_mode);
} else {
os::current_thread_enable_wx(new_mode);
_old_mode = WXWrite;
}
}
~ThreadWXEnable() {
if (_thread) {
_thread->enable_wx(_old_mode);
} else {
os::current_thread_enable_wx(_old_mode);
}
}
};
-
PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Wed, 9 Mar 2022 11:14:09 GMT, David Holmes wrote: >> Johannes Bechberger has updated the pull request incrementally with one >> additional commit since the last revision: >> >> current_thread_wx -> ThreadWX > > src/hotspot/cpu/aarch64/jniFastGetField_aarch64.cpp line 35: > >> 33: #include "prims/jvmtiExport.hpp" >> 34: #include "runtime/safepoint.hpp" >> 35: #include "runtime/thread.inline.hpp" > > I still don't see why this is needed. I must have forgotten this line during refactoring. - PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
On Wed, 9 Mar 2022 08:35:41 GMT, Johannes Bechberger
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
A couple of minor nits but otherwise I'm okay with this version.
Thanks,
David
src/hotspot/cpu/aarch64/jniFastGetField_aarch64.cpp line 35:
> 33: #include "prims/jvmtiExport.hpp"
> 34: #include "runtime/safepoint.hpp"
> 35: #include "runtime/thread.inline.hpp"
I still don't see why this is needed.
src/hotspot/cpu/aarch64/jniFastGetField_aarch64.cpp line 77:
> 75: template decltype(BasicTypeToJni::jni_type)>
> 76: JniType static_fast_get_field_wrapper(JNIEnv *env, jobject obj, jfieldID
> fieldID) {
> 77: JavaThread* thread = JavaThread::thread_from_jni_environment(env);
This line is no longer needed.
-
PR: https://git.openjdk.java.net/jdk/pull/7727
Re: RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7727/files - new: https://git.openjdk.java.net/jdk/pull/7727/files/21dd0046..f206e6d2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7727=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7727=04-05 Stats: 61 lines in 28 files changed: 0 ins; 0 del; 61 mod Patch: https://git.openjdk.java.net/jdk/pull/7727.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7727/head:pull/7727 PR: https://git.openjdk.java.net/jdk/pull/7727
