RFR(M): 8139864: Improve handling of stack protection zones.
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Dec 18 14:59:32 UTC 2015
Thanks Fred for looking at this in detail. Goetz, I'll review this
today and can sponsor it.
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