RFR(M): 8139864: Improve handling of stack protection zones.

Sat Dec 19 13:18:50 UTC 2015

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>; 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.

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-

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-

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><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-

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<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-

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<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>