RFR(M): 8139864: Improve handling of stack protection zones.
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Dec 21 18:41:43 UTC 2015
On 12/21/15 11:08 AM, Lindenmaier, Goetz wrote:
> It's i the repo, thanks for sponsoring!
You're welcome. It's committed now.
Coleen
>
> Best regards,
> Goetz.
>
>> -----Original Message-----
>> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
>> Sent: Sonntag, 20. Dezember 2015 16:43
>> 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.
>>
>>
>> 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.inl
>> ine.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.inl
>> ine.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>
>> <mailto:goetz.lindenmaier at sap.com> ; Frederic Parain
>> <frederic.parain at oracle.com> <mailto:frederic.parain 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.
>>
>>
>>
>>
>> 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