RFR(M): 8139864: Improve handling of stack protection zones.
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Dec 18 22:26:29 UTC 2015
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>; 'Coleen Phillimore'
>> <coleen.phillimore 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.
>>
>> 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-
>> 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>; 'Coleen
>> Phillimore'
>>>> <coleen.phillimore 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.
>>>> 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-
>>>> 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-
>>>> 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>; Lindenmaier, Goetz
>>>>>> <goetz.lindenmaier at sap.com>; hotspot-runtime-
>> 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-
>>>>>> 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-
>>>>>> StackZones/webrev.00-basic/
>>>>>>>> http://cr.openjdk.java.net/~goetz/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] On Behalf Of Lindenmaier, Goetz
>>>>>>>>> Sent: Freitag, 27. November 2015 10:28
>>>>>>>>> To: David Holmes <david.holmes at oracle.com>; Coleen Phillimore
>>>>>>>>> (coleen.phillimore at oracle.com) <coleen.phillimore at oracle.com>;
>>>>>> hotspot-
>>>>>>>>> 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>; Coleen
>>>>>> Phillimore
>>>>>>>>>> (coleen.phillimore at oracle.com)
>> <coleen.phillimore at oracle.com>;
>>>>>>>>> hotspot-
>>>>>>>>>> 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-
>>>>>>>>>> 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-
>>>>>>>>>> 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
>> --
>> Frederic Parain - Oracle
>> Grenoble Engineering Center - France
>> Phone: +33 4 76 18 81 17
>> Email: Frederic.Parain at oracle.com
More information about the hotspot-runtime-dev
mailing list