RFR(M): 8139864: Improve handling of stack protection zones.
Frederic Parain
frederic.parain at oracle.com
Wed Dec 16 09:42:26 UTC 2015
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
More information about the hotspot-runtime-dev
mailing list