RFR(M): 8139864: Improve handling of stack protection zones.
Coleen Phillimore
coleen.phillimore at oracle.com
Sun Dec 20 15:43:02 UTC 2015
There were two compilation errors in this so I've fixed them and
recommitted. One was thread => jt in os_solaris_x86.hpp and one on
windows:
"C:\jprt\T\P1\230313.cphillim\s\hotspot\src\cpu\x86\vm" /D "_JNI_IMPLEMENTATION_" /Fp"vm.pch" /Yc"precompiled.hpp" /c ../generated/_build_pch_file.cpp
_build_pch_file.cpp
C:\jprt\T\P1\230313.cphillim\s\hotspot\src\os\windows\vm\os_windows.inline.hpp(76) : error C2220: warning treated as error - no 'object' file generated
C:\jprt\T\P1\230313.cphillim\s\hotspot\src\os\windows\vm\os_windows.inline.hpp(76) : warning C4018: '<=' :
Becomes (size_t):
for (size_t pages = 1; pages <= (JavaThread::stack_shadow_zone_size()
/ os::vm_page_size()); pages++) {
Coleen
On 12/19/15 8:18 AM, Lindenmaier, Goetz wrote:
>
> Hi Coleen,
>
> thanks for looking at this.
>
> I also found tons of places where cleanup is useful, e.g. the computation
>
> of os.XXX.min_stack_allowed.
>
> I would volunteer to have a look at this once 8139864 is done.
>
> One thing: there are two kinds of stacks (with and without
> protection), and
>
> code is spread between Thread and JavaThread. For a rework one might
>
> need ThreadStack for some code valid for all threads and subclass
>
> JavaThreadStack that handles stack overflow.
>
> Best regards,
>
> Goetz.
>
> *From:*Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
> *Sent:* Friday, December 18, 2015 11:26 PM
> *To:* Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Frederic Parain
> <frederic.parain at oracle.com>; David Holmes <david.holmes at oracle.com>;
> hotspot-runtime-dev at openjdk.java.net
> *Subject:* Re: RFR(M): 8139864: Improve handling of stack protection
> zones.
>
>
> I think this looks good and I will sponsor it. As I think I said
> before though, this should have some more passes of cleanups. It
> would be nice to have a new file
> src/share/vm/runtime/stackOverflow.hpp to encapsulate the stack
> overflow handling (and new big comment), rather than in the middle of
> thread.hpp
>
> In the stackOverflow.cpp file, we could attempt to consolidate the
> duplicated code for stack overflow handling in the signal handlers,
> the calculation in the
> templateInterpreter::generate_stack_overflow_check (once corrected for
> solaris), the calculation for minimum stack size, and this calculation
> in frame_platform.inline.hpp
>
>
> 60 // consider stack guards when trying to determine "safe" stack pointers
> 61 static size_t stack_guard_size = os::uses_stack_guard_pages() ?
> 62 (JavaThread::stack_red_zone_size() + JavaThread::stack_yellow_zone_size()) : 0;
> 63 size_t usable_stack_size = thread->stack_size() - stack_guard_size;
> 64
> 65 // sp must be within the usable part of the stack (not in guards)
> 66 bool sp_safe = (sp < thread->stack_base()) &&
> 67 (sp >= thread->stack_base() - usable_stack_size);
>
> These same bits of code appear in too many places.
>
> But this change fixes the problem with stack guard page size
> parameters, so it's better.
>
> Thanks,
> Coleen
>
> On 12/18/15 4:42 AM, Lindenmaier, Goetz wrote:
>
> HI,
>
> Coleen, is this fine with you, too?
>
> Could someone please sponsor the change?
>
> Thanks,
>
> Goetz.
>
> -----Original Message-----
>
> From: Frederic Parain [mailto:frederic.parain at oracle.com]
>
> Sent: Freitag, 18. Dezember 2015 10:39
>
> To: Lindenmaier, Goetz<goetz.lindenmaier at sap.com> <mailto:goetz.lindenmaier at sap.com>; 'Coleen Phillimore'
>
> <coleen.phillimore at oracle.com>
> <mailto:coleen.phillimore at oracle.com>; David Holmes
>
> <david.holmes at oracle.com> <mailto:david.holmes at oracle.com>;hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>
>
> Subject: Re: RFR(M): 8139864: Improve handling of stack protection zones.
>
> Still looks good to me.
>
> Thank you,
>
> Fred
>
> On 12/17/2015 09:59 PM, Lindenmaier, Goetz wrote:
>
> Hi,
>
> Our test passed successful after I did two minor
>
> changes: remove the assertion that _stack_reserved_zone_size > 0
>
> and fix a windows build issue. I also did the change proposed
>
> by Frederic.
>
> New webrev:
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864
> <http://cr.openjdk.java.net/%7Egoetz/webrevs/8139864>-
>
> StackZones/webrev.04/
>
> Best regards,
>
> Goetz.
>
> -----Original Message-----
>
> From: Frederic Parain [mailto:frederic.parain at oracle.com]
>
> Sent: Wednesday, December 16, 2015 10:42 AM
>
> To: Lindenmaier, Goetz<goetz.lindenmaier at sap.com>
> <mailto:goetz.lindenmaier at sap.com>; 'Coleen
>
> Phillimore'
>
> <coleen.phillimore at oracle.com>
> <mailto:coleen.phillimore at oracle.com>; David Holmes
>
> <david.holmes at oracle.com> <mailto:david.holmes at oracle.com>;hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>
>
> Subject: Re: RFR(M): 8139864: Improve handling of stack protection
>
> zones.
>
> Hi Goetz,
>
> Thank you for fixing this and thank you for having delayed
>
> your work to let me push JDK-8046936.
>
> On 12/15/2015 11:17 PM, Lindenmaier, Goetz wrote:
>
> Hi,
>
> I reworked my change fixing handling of stack zone sizes with
>
> pages > 4K to fit on top of "8046936 : JEP 270: Reserved Stack Areas for
>
> Critical Sections"
>
> The functional core is still
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864
> <http://cr.openjdk.java.net/%7Egoetz/webrevs/8139864>-
>
> StackZones/webrev.00-basic/,
>
> now extended for StackReservedPages.
>
> All the other changes are replacing local computations by shared utility
>
> functions.
>
> I slightly changed handling StackReservedPages in case of large pages:
>
> I now align to page size. Before, it was set to '1'.
>
> In addition, I did a few cleanups.
>
> 8046936 changed yellow zone accessors to handle both, yellow and
>
> reseved zones.
>
> In some places, I need the pure functions for the yellow zone, so I had
>
> name clashes. Therefore I renamed all functions handling reserved and
>
> yellow
>
> zone together to be named 'yellow_reserved'.
>
> I introduced stack_end(), as I found stack_base() - stack_size() in lots
>
> of places.
>
> I am using on_local_stack(addr) in many places where an address was
>
> checked
>
> to be on the stack (actually, I think in_stack(addr) would be a cleaner
>
> name).
>
> Also, I introduced usage of existing stack_overflow_limit() where that
>
> address
>
> was computed.
>
> I think StackReservedPages is missing in the following places:
>
> frame_aarch64.cpp
>
> frame_x86.cpp
>
> templateInterpreterGenerator_aarch64.cpp:476
>
> templateInterpreterGenerator_sparc.cpp
>
> stack_zero.inline.hpp (fixed)
>
> os_solaris.cpp:4462
>
> os_windows.cpp:4196
>
> The reserved area is not implemented on aarch64 and cannot be
>
> currently implemented on Windows because of JDK-8067946.
>
> In frame_x86.cpp the code is correct, when access to the reserved
>
> zone has been granted, it is legal to have a stack pointer in this
>
> zone.
>
> The two issues on SPARC are real, I'm fixing them.
>
> I did not adapt these places, this should go into an extra change
>
> if it is not left out on purpose.
>
> I left out changes to cppInterpreter_<cpu>.cpp files.
>
> Please review this change. I please need a sponsor.
>
> Tests are planned tomorrow night (I missed our today's deadline).
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864
> <http://cr.openjdk.java.net/%7Egoetz/webrevs/8139864>-
>
> StackZones/webrev.03/
>
> thread.hpp:
>
> lines 1429-1436: commented code
>
> Otherwise, looks good to me.
>
> Regards,
>
> Fred
>
> -----Original Message-----
>
> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
>
> Sent: Friday, December 04, 2015 8:59 PM
>
> To: David Holmes<david.holmes at oracle.com>
> <mailto:david.holmes at oracle.com>; Lindenmaier, Goetz
>
> <goetz.lindenmaier at sap.com>
> <mailto:goetz.lindenmaier at sap.com>; hotspot-runtime-
>
> dev at openjdk.java.net <mailto:dev at openjdk.java.net>
>
> Subject: Re: RFR(M): 8139864: Improve handling of stack protection
>
> zones.
>
> Hi Goetz,
>
> I've reviewed this and I think it looks really good, but it is going to
>
> merge conflict with with Frederic's change for RFR(L): JDK-8046936 : JEP
>
> 270: Reserved Stack Areas for Critical Sections, which he's checking in
>
> early next week.
>
> Fred has added a ReservedStackArea for extra protection for certain
>
> methods, ie these methods can use this extra stack. Fred's change is
>
> almost ready to integrate (sorry we didn't get to this before the
>
> conflict arose). Also, I think the reserved pages will have to be
>
> changed to follow this model too.
>
> Two things about this change also that worry me. This should force
>
> some
>
> sort of compilation error or assertion if new code uses
>
> Stack{Red,Yellow,Shadow}Pages. Maybe the values should be
>
> poisoned
>
> to
>
> be something, although I don't know what.
>
> The second thing is that I think it would be nice to move all this stack
>
> handling stuff out into it's own class (move Thread::_stack_base and
>
> Thread::_stack_size out too) into threadStack.hpp/cpp. That could be a
>
> follow-on cleanup.
>
> This change also has to merge with my interpreter change which you
>
> were
>
> so nice to review so quickly.
>
> Lastly, making StackShadowPages mean 4K pages might affect some
>
> customers that have tuned this or because they have large pages set
>
> the
>
> value to 1. Unfortunately, I think this should have an internal
>
> compatibility request, release note and alert our sustaining team. Most
>
> of our public command line options get this treatment when they
>
> change.
>
> I will take care of this also early next week.
>
> Thanks,
>
> Coleen
>
> On 12/4/15 2:39 AM, David Holmes wrote:
>
> On 2/12/2015 2:10 AM, Lindenmaier, Goetz wrote:
>
> Hi,
>
> I ran the change through our night builds - all jtreg tests are clean.
>
> (and the other tests as well.)
>
> I prepared a complete and - from my side - final webrev:
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864
> <http://cr.openjdk.java.net/%7Egoetz/webrevs/8139864>-
>
> StackZones/webrev.01/
>
> This includes the fixes proposed by David as well as some minor fixes
>
> to
>
> get it compiling on windows.
>
> So to capture the change still the split webrevs might be a good
>
> starting point:
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864
> <http://cr.openjdk.java.net/%7Egoetz/webrevs/8139864>-
>
> StackZones/webrev.00-basic/
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864
> <http://cr.openjdk.java.net/%7Egoetz/webrevs/8139864>-
>
> StackZones/webrev.00-spread/
>
> David, can I consider this reviewed?
>
> I will put a small 'r' review on it. :)
>
> I please need a second reviewer and a sponsor.
>
> Pinging Coleen for Review.
>
> Thanks,
>
> David
>
> Best regards,
>
> Goetz.
>
> -----Original Message-----
>
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
>
> bounces at openjdk.java.net
> <mailto:bounces at openjdk.java.net>] On Behalf Of Lindenmaier, Goetz
>
> Sent: Freitag, 27. November 2015 10:28
>
> To: David Holmes<david.holmes at oracle.com>
> <mailto:david.holmes at oracle.com>; Coleen Phillimore
>
> (coleen.phillimore at oracle.com
> <mailto:coleen.phillimore at oracle.com>)<coleen.phillimore at oracle.com>
> <mailto:coleen.phillimore at oracle.com>;
>
> hotspot-
>
> runtime-dev at openjdk.java.net
> <mailto:runtime-dev at openjdk.java.net>
>
> Subject: RE: RFR(M): 8139864: Improve handling of stack protection
>
> zones.
>
> Hi David,
>
> thanks for looking at this change!
>
> I edited the thinks you remarked, thanks for spotting that.
>
> I'll run the change through our testing to assure I catch all
>
> issues with jtreg. We have some systems with other page sizes.
>
> Best regards,
>
> Goetz.
>
> -----Original Message-----
>
> From: David Holmes [mailto:david.holmes at oracle.com]
>
> Sent: Freitag, 27. November 2015 06:33
>
> To: Lindenmaier, Goetz<goetz.lindenmaier at sap.com>
> <mailto:goetz.lindenmaier at sap.com>; Coleen
>
> Phillimore
>
> (coleen.phillimore at oracle.com
> <mailto:coleen.phillimore at oracle.com>)
>
> <coleen.phillimore at oracle.com>
> <mailto:coleen.phillimore at oracle.com>;
>
> hotspot-
>
> runtime-dev at openjdk.java.net
> <mailto:runtime-dev at openjdk.java.net>
>
> Subject: Re: RFR(M): 8139864: Improve handling of stack
>
> protection
>
> zones.
>
> Hi Goetz,
>
> On 26/11/2015 1:55 AM, Lindenmaier, Goetz wrote:
>
> Hi,
>
> For a description of the problem see
>
> https://bugs.openjdk.java.net/browse/JDK-8139864
>
> This is a first version of my implementation of the proposal '3'
>
> that was
>
> discussed here:
>
> http://mail.openjdk.java.net/pipermail/hotspot-runtime-
>
> dev/2015-
>
> October/016085.html
>
> which is continued here
>
> http://mail.openjdk.java.net/pipermail/hotspot-runtime-
>
> dev/2015-
>
> November/016805.html
>
> Opposed to my original proposal, I did not introduce new flags. I
>
> documented the
>
> old flags to specify 4K pages, and save the ergonomic values in
>
> static
>
> fields
>
> of JavaThread.
>
> This avoids two sets of similar flags.
>
> That seems like a good compromise. Though not sure about
>
> placement
>
> in
>
> JavaThread when everything else stack-related seems to be in os
>
> class.
>
> To simplify reviewing, I made two webrevs for the first round:
>
> The basic changes to determining the sizes of the zones to
>
> protect
>
> are in
>
> this partial webrev:
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864
> <http://cr.openjdk.java.net/%7Egoetz/webrevs/8139864>-
>
> StackZones/webrev.00-basic/
>
> This contains the functional change.
>
> This seems okay to me - though I don't claim expertise in this area.
>
> A couple of minor nits:
>
> src/share/vm/runtime/thread.hpp
>
> 1364 static size_t stack_yellow_zone_size() {
>
> 1365 assert(_stack_red_zone_size > 0, "Don't call this before
>
> the
>
> field is initialized.");
>
> The assert should check yellow size
>
> 1383 static size_t stack_shadow_zone_size() {
>
> 1384 assert(_stack_red_zone_size > 0, "Don't call this before
>
> the
>
> field is initialized.");
>
> The assert should check shadow size.
>
> ---
>
> src/share/vm/runtime/globals.hpp
>
> kB -> KB
>
> I also fixed the bounds of ThreadStackSize and
>
> CompilerThreadStackSize
>
> which
>
> allowed to specify stacks >> max_intx.
>
> Ok - not sure how this was missed given we do the same for
>
> VMThreadStackSize :)
>
> If we agree on this, I need to replace all occurrences of the
>
> three flags by
>
> accessor calls.
>
> This allows to simplify all the platform code which computed the
>
> space
>
> required
>
> over and over again:
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864
> <http://cr.openjdk.java.net/%7Egoetz/webrevs/8139864>-
>
> StackZones/webrev.00-spread/
>
> That all seems okay to me.
>
> Actually I think there is no need that the shadow zone is page
>
> aligned.
>
> There are no
>
> pages to allocate or protect. The size of this zone is only used for
>
> banging.
>
> In most
>
> places only the upper bound of the zone is banged. In few
>
> places,
>
> all
>
> pages
>
> within
>
> this zone are banged. But this code also does not depend on a
>
> page
>
> aligned size.
>
> Removing the rounding to page sizes here could free up stack
>
> space
>
> for
>
> real
>
> usage.
>
> But I think that should be done in an extra change.
>
> I also fixed the bounds of ThreadStackSize and
>
> CompilerThreadStackSize
>
> which
>
> allowed to specify stacks >> max_intx.
>
> If this is agreed on, I will check for jtreg tests that fail on
>
> systems with
>
> stack
>
> pages > 4K and add the fixes to this change.
>
> You may also need to check the tests for valid command-line args
>
> given
>
> the range adjustments on the stack sizes.
>
> Thanks,
>
> David
>
> -----
>
> Best regards,
>
> Goetz.
>
> --
>
> Frederic Parain - Oracle
>
> Grenoble Engineering Center - France
>
> Phone: +33 4 76 18 81 17
>
> Email:Frederic.Parain at oracle.com
> <mailto:Frederic.Parain at oracle.com>
>
> --
>
> Frederic Parain - Oracle
>
> Grenoble Engineering Center - France
>
> Phone: +33 4 76 18 81 17
>
> Email:Frederic.Parain at oracle.com <mailto:Frederic.Parain at oracle.com>
>
More information about the hotspot-runtime-dev
mailing list