Re: RFR: 8330969: scalability issue with loaded JVMTI agent
On Wed, 24 Apr 2024 16:04:30 GMT, Serguei Spitsyn wrote: > This is a fix of the following JVMTI scalability issue. A closed benchmark > with millions of virtual threads shows 3X-4X overhead when a JVMTI agent has > been loaded. For instance, this is observable when an app is executed under > control of the Oracle Studio `collect` utility. > For performance analysis, experiments and numbers, please, see the comment > below this description. > > The fix is to replace the global counter `_VTMS_transition_count` with the > mark bit `_VTMS_transition_mark` in each `JavaThread`'. > > Testing: > - Tested with mach5 tiers 1-6 The benchmark takes a little bit more than 3 sec without any JVMTI agent: Total: in 3045 ms The benchmark takes more than ~3.2X of the above when executed with the `collect` utility: Creating experiment database test.1.er (Process ID: 25262) ... Picked up JAVA_TOOL_OPTIONS: -agentlib:collector Total: in 9864 ms With the fix in place the overhead of a JVMTI agent is around 1.2X: Creating experiment database test.1.er (Process ID: 26442) ... Picked up JAVA_TOOL_OPTIONS: -agentlib:collector Total: in 3765 ms The most of the overhead is taken by two functions: - `JvmtiVTMSTransitionDisabler::start_VTMS_transition()` - `JvmtiVTMSTransitionDisabler::finish_VTMS_transition()` Oracle Studio Performance Analyzer `err_print` utility shows the following performance data for these functions: ``` % er_print -viewmode expert -metrics ie.%totalcpu -csingle JvmtiVTMSTransitionDisabler::start_VTMS_transition test.1.er Attr. Total Name CPU sec. % === Callers 42.930 50.06 SharedRuntime::notify_jvmti_vthread_mount(oopDesc*, unsigned char, JavaThread*) 21.505 25.08 JvmtiVTMSTransitionDisabler::VTMS_vthread_end(_jobject*) 21.315 24.86 JvmtiVTMSTransitionDisabler::VTMS_vthread_unmount(_jobject*, bool) === Stack Fragment 81.407 94.94 JvmtiVTMSTransitionDisabler::start_VTMS_transition(_jobject*, bool) === Callees 4.083 4.76 java_lang_Thread::set_is_in_VTMS_transition(oopDesc*, bool) 0.140 0.16 __tls_get_addr 0.120 0.14 JNIHandles::resolve_external_guard(_jobject*) % er_print -viewmode expert -metrics ie.%totalcpu -csingle JvmtiVTMSTransitionDisabler::finish_VTMS_transition test.1.er Attr. Total Name CPU sec. % === Callers 47.363 52.59 SharedRuntime::notify_jvmti_vthread_unmount(oopDesc*, unsigned char, JavaThread*) 21.355 23.71 JvmtiVTMSTransitionDisabler::VTMS_vthread_mount(_jobject*, bool) 21.345 23.70 JvmtiVTMSTransitionDisabler::VTMS_vthread_start(_jobject*) === Stack Fragment 64.145 71.22 JvmtiVTMSTransitionDisabler::finish_VTMS_transition(_jobject*, bool) === Callees 25.288 28.08 java_lang_Thread::set_is_in_VTMS_transition(oopDesc*, bool) 0.240 0.27 __tls_get_addr 0.200 0.22 JavaThread::set_is_in_VTMS_transition(bool) 0.190 0.21 JNIHandles::resolve_external_guard(_jobject*) The main source of this overhead (~90% of overhead) is atomic increment and decrement of the global counter `VTMS_transition_count`: - `Atomic::inc(&_VTMS_transition_count)`; - `Atomic::dec(&_VTMS_transition_count)`; The fix is to replace this global counter with mark bits `_VTMS_transition_mark` distributed over all `JavaThread`'s. If these lines are commented out or replaced with the distributed thread-local marks the main performance overhead is gone: % er_print -viewmode expert -metrics ie.%totalcpu -csingle JvmtiVTMSTransitionDisabler::start_VTMS_transition test.2.er Attr. TotalName CPU sec. % == Callers 1.801 64.29 SharedRuntime::notify_jvmti_vthread_mount(oopDesc*, unsigned char, JavaThread*) 0.580 20.71 JvmtiVTMSTransitionDisabler::VTMS_vthread_unmount(_jobject*, bool) 0.420 15.00 JvmtiVTMSTransitionDisabler::VTMS_vthread_end(_jobject*) == Stack Fragment 0.630 22.50 JvmtiVTMSTransitionDisabler::start_VTMS_transition(_jobject*, bool) == Callees 1.931 68.93 java_lang_Thread::set_is_in_VTMS_transition(oopDesc*, bool) 0.220 7.86 __tls_get_addr 0.020 0.71 JNIHandles::resolve_external_guard(_jobject*) % er_print -viewmode expert -metrics ie.%totalcpu -csingle JvmtiVTMSTransitionDisabler::finish_VTMS_transition test.2.er Attr. TotalName CPU sec. % == Callers 1.661 39.15 JvmtiVTMSTransitionDisabler::VTMS_vthread_mount(_jobject*, bool) 1.351 31.84 JvmtiVTMSTransitionDisabler::VTMS_vthread_start(_jobject*) 1.231 29.01 SharedRuntime::notify_jvmti_vthread_unmount(oopDesc*, unsigned char, JavaThread*) == Stack Fragment 0.500 11.79 JvmtiVTMSTransitionDisabler::finish_VTMS_transition(_jobject*, bool) == Callees 2.972 70.05 java_lang_Thread::set_is_in_VTMS_transition(oopDesc*, bool) 0.350 8.25 JavaThread::set_is_in_VTMS_transition(bool) 0.340 8.02
RFR: 8330969: scalability issue with loaded JVMTI agent
This is a fix of the following JVMTI scalability issue. A closed benchmark with millions of virtual threads shows 4X overhead when a JVMTI agent has been loaded. For instance, this is observable when an app is executed under control of the Oracle Studio `collect` utility. The benchmark takes a little bit more than 3 sec without any JVMTI agent: Total: in 3045 ms The benchmark takes more than ~3.2X of the above when executed with the `collect` utility: Creating experiment database test.1.er (Process ID: 25262) ... Picked up JAVA_TOOL_OPTIONS: -agentlib:collector Total: in 9864 ms With the fix in place the overhead of a JVMTI agent is around 1.2X: Creating experiment database test.1.er (Process ID: 26442) ... Picked up JAVA_TOOL_OPTIONS: -agentlib:collector Total: in 3765 ms The most of the overhead is taken by two functions: - `JvmtiVTMSTransitionDisabler::start_VTMS_transition()` - `JvmtiVTMSTransitionDisabler::finish_VTMS_transition()` Oracle Studio Performance Analyzer `err_print utility shows the following performance data for these functions: ``` % er_print -viewmode expert -metrics ie.%totalcpu -csingle JvmtiVTMSTransitionDisabler::start_VTMS_transition test.1.er Attr. Total Name CPU sec. % === Callers 42.930 50.06 SharedRuntime::notify_jvmti_vthread_mount(oopDesc*, unsigned char, JavaThread*) 21.505 25.08 JvmtiVTMSTransitionDisabler::VTMS_vthread_end(_jobject*) 21.315 24.86 JvmtiVTMSTransitionDisabler::VTMS_vthread_unmount(_jobject*, bool) === Stack Fragment 81.407 94.94 JvmtiVTMSTransitionDisabler::start_VTMS_transition(_jobject*, bool) === Callees 4.083 4.76 java_lang_Thread::set_is_in_VTMS_transition(oopDesc*, bool) 0.140 0.16 __tls_get_addr 0.120 0.14 JNIHandles::resolve_external_guard(_jobject*) % er_print -viewmode expert -metrics ie.%totalcpu -csingle JvmtiVTMSTransitionDisabler::finish_VTMS_transition test.1.er Attr. Total Name CPU sec. % === Callers 47.363 52.59 SharedRuntime::notify_jvmti_vthread_unmount(oopDesc*, unsigned char, JavaThread*) 21.355 23.71 JvmtiVTMSTransitionDisabler::VTMS_vthread_mount(_jobject*, bool) 21.345 23.70 JvmtiVTMSTransitionDisabler::VTMS_vthread_start(_jobject*) === Stack Fragment 64.145 71.22 JvmtiVTMSTransitionDisabler::finish_VTMS_transition(_jobject*, bool) === Callees 25.288 28.08 java_lang_Thread::set_is_in_VTMS_transition(oopDesc*, bool) 0.240 0.27 __tls_get_addr 0.200 0.22 JavaThread::set_is_in_VTMS_transition(bool) 0.190 0.21 JNIHandles::resolve_external_guard(_jobject*) The main source of this overhead (~90% of overhead) is atomic increment and decrement of the global counter `VTMS_transition_count`: - `Atomic::inc(&_VTMS_transition_count)`; - `Atomic::dec(&_VTMS_transition_count)`; The fix is to replace this global counter with mark bits `_VTMS_transition_mark` distributed over all `JavaThread`'s. If these lines are commented out or replaced with the distributed thread-local marks the main performance overhead is gone: % er_print -viewmode expert -metrics ie.%totalcpu -csingle JvmtiVTMSTransitionDisabler::start_VTMS_transition test.2.er Attr. TotalName CPU sec. % == Callers 1.801 64.29 SharedRuntime::notify_jvmti_vthread_mount(oopDesc*, unsigned char, JavaThread*) 0.580 20.71 JvmtiVTMSTransitionDisabler::VTMS_vthread_unmount(_jobject*, bool) 0.420 15.00 JvmtiVTMSTransitionDisabler::VTMS_vthread_end(_jobject*) == Stack Fragment 0.630 22.50 JvmtiVTMSTransitionDisabler::start_VTMS_transition(_jobject*, bool) == Callees 1.931 68.93 java_lang_Thread::set_is_in_VTMS_transition(oopDesc*, bool) 0.220 7.86 __tls_get_addr 0.020 0.71 JNIHandles::resolve_external_guard(_jobject*) % er_print -viewmode expert -metrics ie.%totalcpu -csingle JvmtiVTMSTransitionDisabler::finish_VTMS_transition test.2.er Attr. TotalName CPU sec. % == Callers 1.661 39.15 JvmtiVTMSTransitionDisabler::VTMS_vthread_mount(_jobject*, bool) 1.351 31.84 JvmtiVTMSTransitionDisabler::VTMS_vthread_start(_jobject*) 1.231 29.01 SharedRuntime::notify_jvmti_vthread_unmount(oopDesc*, unsigned char, JavaThread*) == Stack Fragment 0.500 11.79 JvmtiVTMSTransitionDisabler::finish_VTMS_transition(_jobject*, bool) == Callees 2.972 70.05 java_lang_Thread::set_is_in_VTMS_transition(oopDesc*, bool) 0.350 8.25 JavaThread::set_is_in_VTMS_transition(bool) 0.340 8.02 __tls_get_addr 0.080 1.89 JNIHandles::resolve_external_guard(_jobject*) The rest of the overhead (~10% of total overhead) is taken by calls to the function `java_lang_Thread::set_is_in_VTMS_transition()`. The plan is to address this in a separate fix. But it is expected to be a little bit more tricky. Testing: - Tested with mach5 tiers 1-6
Integrated: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed
On Wed, 17 Apr 2024 00:29:52 GMT, Serguei Spitsyn wrote:
> This is a simple fix of three similar asserts.
> The `_target_jt->jvmti_vthread()` has to be used instead of
> `_target_jt->vthread()`.
> The `_target_jt->vthread()` can be outdated in some specific contexts as
> shown in the `hs_err` stack trace.
>
> I've seen similar issue and already fixed it in this fragment of code:
>
> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
> . . .
> void do_vthread(Handle target_h) {
> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
> check");
> // use jvmti_vthread() as vthread() can be outdated
> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
> target_h(), "sanity check");
> . . .
>
> The issue above was fixed by replacing `_target_jt->vthread()` with
> `_target_jt->jvmti_vthread()`.
>
> There are three places which need to be fixed the same way:
> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>
> Testing:
> - Run mach5 tiers 1-6
This pull request has now been integrated.
Changeset: 15190816
Author:Serguei Spitsyn
URL:
https://git.openjdk.org/jdk/commit/15190816f704f2e8681bc3e2d74832828a574106
Stats: 11 lines in 3 files changed: 7 ins; 0 del; 4 mod
8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() ==
target_h()) failed
Reviewed-by: pchilanomate, cjplummer, lmesnik
-
PR: https://git.openjdk.org/jdk/pull/18806
Re: RFR: 8330155: Serial: Remove TenuredSpace [v3]
On Tue, 23 Apr 2024 17:22:41 GMT, Guoxiong Li wrote: >> Hi all, >> >> This patch removes the class `TenuredSpace` and adjusts its usages. After >> removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove >> this file and change the included header file to `space.hpp`. >> >> The test `make test-tier1_gc` passed locally. Thanks for taking the time to >> review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li has updated the pull request incrementally with one additional > commit since the last revision: > > Fix included header file error after merging master. Thanks for the reviews. Integrating. - PR Comment: https://git.openjdk.org/jdk/pull/18894#issuecomment-2074744514
Integrated: 8330155: Serial: Remove TenuredSpace
On Mon, 22 Apr 2024 16:24:06 GMT, Guoxiong Li wrote: > Hi all, > > This patch removes the class `TenuredSpace` and adjusts its usages. After > removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove > this file and change the included header file to `space.hpp`. > > The test `make test-tier1_gc` passed locally. Thanks for taking the time to > review. > > Best Regards, > -- Guoxiong This pull request has now been integrated. Changeset: 2bb5cf5f Author:Guoxiong Li URL: https://git.openjdk.org/jdk/commit/2bb5cf5f7b2cc40aca3bdd36400dc4af5723 Stats: 162 lines in 21 files changed: 11 ins; 127 del; 24 mod 8330155: Serial: Remove TenuredSpace Reviewed-by: ayang, cjplummer, tschatzl - PR: https://git.openjdk.org/jdk/pull/18894
Re: RFO: a tool to analyze HotSpot fatal error logs
I would find this tool particularly useful. I came across various `hs_err_pid` files, while one can read them this can speed analyzing up significantly. Even for simple cases, like a bug in the FFM API usage (it's still possible to dereference a bad address). Looking forward to this plugin ! Thanks for proposing the idea. -- Brice On Fri, Apr 12, 2024 at 5:35 PM Laurence Cable wrote: > Hi Maxim, a great idea, the JDK serviceability team here at Oracle would > like to assist you in any way we can. > > I think also we should (in the future) consider the format of the error > file and associated jcmd and perhaps render the content > in a format that is better suited to programmatic parsing even a > jq-esque formatter that took the human-readable format and > re-formatted would be useful (IMO) > > Rgds > > - Larry Cable > > > On 4/11/24 7:05 AM, Maxim Kartashev wrote: > > Hello, > > > > I am writing to inquire about the potential interest of the people > > involved in inspecting HotSpot crashes in a tool aimed at facilitating > > that inspection. > > > > We at JetBrains have developed an internal plugin that helps both with > > filtering through dozens of reports quickly in order to find a pattern > > and for diving deep into a particular crash. In addition to the > > "standard" features such as syntax highlighting, folding, and > > structural navigation, it will > > * highlight potential problems such as overloaded CPU, low physical > > memory, the presence of OOME in the recent exceptions, LD_LIBRARY_PATH > > being set, etc, > > * generate an "executive summary" for a high-level overview, for > > example, by front-line support, > > * pop up a tooltip for any recognized address describing its origin > > (for example, if it belongs to some thread's stack, the Java heap, a > > register, or a memory-mapped region), > > * provide the ability to highlight all addresses "near" the selected > > address, including registers, threads, and memory-mapped regions. > > > > If there is sufficient interest in creating a public and/or > > open-source variant of this internal plugin, I will pitch the idea to > > my employer. It shouldn't be too much work to create a public version. > > > > Kind regards, > > Maxim. > > > > References: > > * https://docs.oracle.com/javase/10/troubleshoot/fatal-error-log.htm > > > >
Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed [v3]
On Wed, 24 Apr 2024 05:44:42 GMT, Chris Plummer wrote:
>> Serguei Spitsyn has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - “Merge”
>> - review: updated same clarifying comment in several spots
>> - add comments explaining that the vthread() can return outdated oop
>> - 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() ==
>> target_h()) failed
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 2079:
>
>> 2077: void
>> 2078: GetSingleStackTraceClosure::do_vthread(Handle target_h) {
>> 2079: // Use jvmti_vthread() instead of vthread() as target could have
>> temporary changed
>
> Suggestion:
>
> // Use jvmti_vthread() instead of vthread() as target could have
> temporarily changed
Good catch, fixed now.
> src/hotspot/share/prims/jvmtiEnvBase.hpp line 509:
>
>> 507: void do_vthread(Handle target_h) {
>> 508: assert(_target_jt != nullptr, "sanity check");
>> 509: // Use jvmti_vthread() instead of vthread() as target could have
>> temporary changed
>
> Suggestion:
>
> // Use jvmti_vthread() instead of vthread() as target could have
> temporarily changed
Good catch, thanks. Fixed now.
> src/hotspot/share/prims/jvmtiEnvBase.hpp line 531:
>
>> 529: void do_vthread(Handle target_h) {
>> 530: assert(_target_jt != nullptr, "sanity check");
>> 531: // Use jvmti_vthread() instead of vthread() as target could have
>> temporary changed
>
> Suggestion:
>
> // Use jvmti_vthread() instead of vthread() as target could have
> temporarily changed
Good catch, thanks. Fixed now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18806#discussion_r1577621820
PR Review Comment: https://git.openjdk.org/jdk/pull/18806#discussion_r1577623055
PR Review Comment: https://git.openjdk.org/jdk/pull/18806#discussion_r1577622556
Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed [v4]
> This is a simple fix of three similar asserts.
> The `_target_jt->jvmti_vthread()` has to be used instead of
> `_target_jt->vthread()`.
> The `_target_jt->vthread()` can be outdated in some specific contexts as
> shown in the `hs_err` stack trace.
>
> I've seen similar issue and already fixed it in this fragment of code:
>
> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
> . . .
> void do_vthread(Handle target_h) {
> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
> check");
> // use jvmti_vthread() as vthread() can be outdated
> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
> target_h(), "sanity check");
> . . .
>
> The issue above was fixed by replacing `_target_jt->vthread()` with
> `_target_jt->jvmti_vthread()`.
>
> There are three places which need to be fixed the same way:
> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>
> Testing:
> - Run mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: fixed typo in same comment in several spots
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18806/files
- new: https://git.openjdk.org/jdk/pull/18806/files/c5ad80d0..643c3046
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18806=03
- incr: https://webrevs.openjdk.org/?repo=jdk=18806=02-03
Stats: 4 lines in 3 files changed: 0 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/18806.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18806/head:pull/18806
PR: https://git.openjdk.org/jdk/pull/18806
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Tue, 23 Apr 2024 13:56:32 GMT, Julian Waters wrote: > WIP > > This changeset contains hsdis for Windows/gcc Port. It supports both the > binutils and capstone backends, though the LLVM backend is left out due to > compatibility issues encountered during the build. Currently, which gcc > distributions are supported is still to be clarified, as several, ranging > from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, > the build system hack in place at the moment to compile the binutils backend > on Windows is still left in place, for now. Please mark the PR as draft it is not intended for review. - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2074481887
Integrated: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner
On Mon, 15 Apr 2024 06:47:24 GMT, Serguei Spitsyn wrote: > This is the test issue. The `WaitingPT3` thread posted the `MonitorWait` > event but has not released the `lockCheck` monitor yet. It has been fixed to > wait for each `WaitingTask` thread to really reach the `WAITING` state. The > same approach is used for `EnteringTask` threads. It has been fixed to wait > for each `EnteringTask` thread to reach the `BLOCKED_ON_MONITOR` state. > > Testing: > - Reran the test > `serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java` locally > - TBD: submit the mach5 tiers 1-6 as well This pull request has now been integrated. Changeset: 174d6265 Author:Serguei Spitsyn URL: https://git.openjdk.org/jdk/commit/174d62652c69e811cf44ab64db575b13a848a728 Stats: 114 lines in 4 files changed: 20 ins; 80 del; 14 mod 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner Reviewed-by: lmesnik, cjplummer - PR: https://git.openjdk.org/jdk/pull/18778
Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v5]
On Wed, 24 Apr 2024 00:01:45 GMT, Serguei Spitsyn wrote: >> This is the test issue. The `WaitingPT3` thread posted the `MonitorWait` >> event but has not released the `lockCheck` monitor yet. It has been fixed to >> wait for each `WaitingTask` thread to really reach the `WAITING` state. The >> same approach is used for `EnteringTask` threads. It has been fixed to wait >> for each `EnteringTask` thread to reach the `BLOCKED_ON_MONITOR` state. >> >> Testing: >> - Reran the test >> `serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java` locally >> - TBD: submit the mach5 tiers 1-6 as well > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: rename ensureWaitsToBeNotified to ensureWaitingToBeNotified Thank you for review, Leonid and Chris! - PR Comment: https://git.openjdk.org/jdk/pull/18778#issuecomment-2074450724
Re: RFR: 8330155: Serial: Remove TenuredSpace [v3]
On Tue, 23 Apr 2024 17:22:41 GMT, Guoxiong Li wrote: >> Hi all, >> >> This patch removes the class `TenuredSpace` and adjusts its usages. After >> removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove >> this file and change the included header file to `space.hpp`. >> >> The test `make test-tier1_gc` passed locally. Thanks for taking the time to >> review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li has updated the pull request incrementally with one additional > commit since the last revision: > > Fix included header file error after merging master. Marked as reviewed by ayang (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18894#pullrequestreview-2019079434
Re: RFR: 8330171: Lazy W^X switch implementation
On Tue, 23 Apr 2024 15:11:10 GMT, Andrew Haley wrote: >> What about granting `WXWrite` only if the current thread is in >> `_thread_in_vm`? >> That would be more restrictive and roughly equivalent how it currently >> works. Likely there are some places then that should be granted `WXWrite` >> eagerly because they need `WXWrite` without `_thread_in_vm`. E.g. the JIT >> compiler threads should have `WXWrite` and never `WXExec` (I assume) which >> should be checked in the signal handler. > >> The patch doesn't protect against native agents, as this is obviously >> impossible. The current code doesn't do that either. For the bytecode, it >> doesn't prevent the attacker from abusing unsafe api to modify code cache. >> However unsafe functions are already considered "safe" and we proactively >> enable WXWrite as well as move thread to `_thread_in_vm` state (@reinrich). >> JITed code can't write to the cache either with or without the patch. >> >> I totally get the sense of loss of security. But is this really the case? > > I think it is. W^X is intended (amongst other things) to protect against the > use of gadgets, from buffer overflow exploits in non-java code to ROP > programming. At present, in order to generate code and execute it, you first > have to be able to make the JIT code writable, then write the code, then make > it executable. then jump to the code. And the exploit writer might have to do > some or all of this by finding gadgets. If we were to merge this patch then > all the attacker would have to do is write code to memory and find a way to > jump to it, and the automatic switch-on-segfault in this patch would do the > all the work the attacker needs. > > It makes far more sense to tag those places that actually need to change W^X > access, and only switch there. > > You could argue that any switching of W^X on a write to code space, then > switching it back on jumping (or returning) to Java code, even what we > already do, is effectively the same thing. Kinda, but it's not on just any > attempt to write to code space or any attempt to jump into code, it's at the > places we choose, and we can be careful to limit those places. > > But surely the JDK is not the most vulnerable part of the stack anyway? I'd > agree with that, of course, but I don't think that's sufficient reason to > decide to bypass an OS security mechanism. > > We are trying to reduce the size of the attack surface. To add a little to @theRealAph 's point, we should avoid painting ourselves into a corner. I don't know how the platform is going to evolve, but I'd be nervous about fighting against the intentions of the protections. - PR Comment: https://git.openjdk.org/jdk/pull/18762#issuecomment-2074244082
