Hello,
On 2018-09-10 01:27, Magnus Ihse Bursie wrote:
On 2018-09-07 19:30, Erik Joelsson wrote:
Hello again,
Resurrecting this review. It is now part of proposed JEP and some
details have changed. The alternate jvm variant is now called
"nonspeculative", and so is activated with the command line flag
"-nonspeculative". This is all in line with the proposed JEP.
Differences from last webrev are:
* The name of the new JVM variant
* Removed an unused AC_SUBST
Webrev: http://cr.openjdk.java.net/~erikj/8202384/webrev.07/index.html
Looks good to me. But what about the change in GensrcJfr.gmk? (I might
have asked about this before...)
It's a leftover but still needed IMO. This patch initially contained the
fix for separating output dirs for the jfr java gensrc tool. That fix
was then applied separately, but as I see it incompletely. If we compile
the tool in the GensrcX.gmk file, then we must also filter the classes
being compiled so we don't recompile every tool in every GensrcX.gmk
file. Right now, there are no other tools in that src root, but there
may well be in the future, especially if we start moving all build tools
into a common dir.
/Erik
/Magnus
/Erik
On 2018-06-11 23:29, Magnus Ihse Bursie wrote:
On 2018-06-11 22:42, Erik Joelsson wrote:
Hello,
Based on the discussion here, I have reverted back to something
more similar to webrev.02, but with a few changes. Mainly fixing a
bug that caused JVM_FEATURES_hardened to not actually be the same
as for server (if you have custom additions in configure). I also
added a check so that configure fails if you try to enable either
variant hardened or feature no-speculative-cti and the flags aren't
available.
Webrev: http://cr.openjdk.java.net/~erikj/8202384/webrev.05/index.html
Looks good to me. Thanks for all the effort!
/Magnus
/Erik
On 2018-06-11 00:10, Magnus Ihse Bursie wrote:
On 2018-06-08 23:50, Erik Joelsson wrote:
On 2018-06-07 17:30, David Holmes wrote:
On 8/06/2018 6:11 AM, Erik Joelsson wrote:
I just don't think the extra work is warranted or should be
prioritized at this point. I also cannot think of a combination
of options required for what you are suggesting that wouldn't
be confusing to the user. If someone truly feels like these
flags are forced on them and can't live with them, we or
preferably that person can fix it then. I don't think that's
dictatorship. OpenJDK is still open source and anyone can
contribute.
I don't see why --enable-hardened-jdk and
--enable-hardened-hotspot to add to the right flags would be
either complicated or confusing.
For me the confusion surrounds the difference between
--enable-hardened-hotspot and --with-jvm-variants=server,
hardened and making the user understand it. But sure, it is
doable. Here is a new webrev with those two options as I
interpret them. Here is the help text:
--enable-hardened-jdk enable hardenening compiler flags for
all jdk
libraries (except the JVM), typically
disabling
speculative cti. [disabled]
--enable-hardened-hotspot
enable hardenening compiler flags for
hotspot (all
jvm variants), typically disabling
speculative cti.
To make hardening of hotspot a runtime
choice,
consider the "hardened" jvm variant
instead of this
option. [disabled]
Note that this changes the default for jdk libraries to not
enable hardening unless the user requests it.
Webrev: http://cr.openjdk.java.net/~erikj/8202384/webrev.04/
Hold it, hold it! I'm not sure how we ended up here, but I don't
like it at all. :-(
I think Eriks initial patch is much better than this. Some
arguments in random order to defend this position:
1) Why should we have a configure option to disable security
relevant flags for the JDK, if there has been no measured negative
effect? We don't do this for any other compiler flags, especially
not security relevant ones!
I've re-read the entire thread to see if I could understand what
could possibly motivate this, but the only thing I can find is
David Holmes vague fear that these flags would not be well-tested
enough. Let me counter with my own vague guesses: I believe the
spectre mitigation methods to have been fully and properly tested,
since they are rolled-out massively on all products. And let me
complement with my own fear: the PR catastrophe if OpenJDK were
*not* built with spectre mitigations, and someone were to exploit
that!
In fact, I could even argue that "server" should be hardened *by
default*, and that we should instead introduce a non-hardened JVM
named something akin to "quick-but-dangerous-server" instead. But
I realize that a 25% performance hit is hard to swallow, so I
won't push this agenda.
2) It is by no means clear that "--enable-hardened-jdk" does not
harden all aspects of the JDK! If we should keep the option (which
I definitely do not think we should!) it should be renamed to
"--enable-hardened-libraries", or something like that. And it
should be on by default, so it should be a
"--disabled-hardened-jdk-libraries".
Also, the general-sounding name "hardened" sounds like it might
encompass more things than it does. What if I disabled a hardened
jdk build, should I still get stack banging protection? If so, you
need to move a lot more security-related flags to this option.
(And, just to be absolutely clear: I don't think you should do that.)
3) Having two completely different ways of turning on Spectre
protection for hotspot is just utterly confusing! This was a
perfect example of how to use the JVM features, just as in the
original patch.
If you want to have spectre mitigation enabled for both server and
client, by default, you would just need to run "configure
--with-jvm-variants=server,client
--with-jvm-features=no-speculative-cti", which will enable that
feature for all variants. That's not really hard *at all* for
anyone building OpenJDK. And it's way clearer what will happen,
than a --enable-hardened-hotspot.
4) If you are a downstream provider building OpenJDK and you are
dead set on not including Spectre mitigations in the JDK
libraries, despite being shown to have no negative effects, then
you can do just as any other downstream user with highly
specialized requirements, and patch the source. I have no
sympathies for this; I can't stop it but I don't think there's any
reason for us to complicate the code to support this unlikely case.
So, to recap, I think the webrev as published in
http://cr.openjdk.java.net/~erikj/8202384/webrev.02/ (with
"altserver" renamed to "hardened") is the way to go.
/Magnus
/Erik
