RFR(M): 8139864: Improve handling of stack protection zones.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Mon Dec 21 07:42:22 UTC 2015
Hi Coleen,
sorry for the trouble!
I didn't do a solaris x86_64 build, but windows should
have worked! I did a windows 64 bit build. Maybe some
difference in compilers.
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