Oilpan: Improve address space randomization for the Oilpan heap.

Oilpan: Improve address space randomization for the Oilpan heap.

Allocate oilpan heap pages in chunks of 10 pages at a random address.
Allocating all individual pages at random addresses blows out the TLB
and leads to poor performance so we group the allocations slightly.

Using a random aligned address for our allocations has the additional
advantage that we usually do not have to unmap surrounding unaligned
bits on all page allocations which turns out to be surprisingly
expensive on Linux.

The address space randomization code has been extracted from
PartitionAlloc. No need to reinvent the wheel. :-)

R=cevans@chromium.org, erik.corry@gmail.com, oilpan-reviews@chromium.org, tsepez@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177085

[Mads Ager (chromium), 2014-06-26 15:24:59]

Tom and Chris, does the extraction of the address space randomization bits into
its own header make sense to you or would you prefer another arrangement?

[Tom Sepez, 2014-06-26 16:31:01]

I like the idea of splitting it off, but why is the code in a .h rather than a
.cpp file?

[Mads Ager (chromium), 2014-06-26 16:57:18]

On 2014/06/26 16:31:01, Tom Sepez wrote:
> I like the idea of splitting it off, but why is the code in a .h rather than a
> .cpp file?

Good point, thanks. I did it initially to make sure that it could get inlined
the way it probably was before. I doubt that it will regress having it in the
cpp file so we should do that and deal with it if it causes regressions. 

Also, I didn't have the right header guards. Fixed that too.

[Tom Sepez, 2014-06-26 17:06:54]

LGTM, but get cevans to look as well.  I only reviewed the ASR portion, not the
oilpan-specific code.

https://codereview.chromium.org/343753004/diff/1/Source/wtf/AddressSpaceRando...
File Source/wtf/AddressSpaceRandomization.h (right):

https://codereview.chromium.org/343753004/diff/1/Source/wtf/AddressSpaceRando...
Source/wtf/AddressSpaceRandomization.h:2: * Copyright (C) 2013 Google Inc. All
rights reserved.
Nit: no (C), 2014.

https://codereview.chromium.org/343753004/diff/20001/Source/wtf/AddressSpaceR...
File Source/wtf/AddressSpaceRandomization.cpp (right):

https://codereview.chromium.org/343753004/diff/20001/Source/wtf/AddressSpaceR...
Source/wtf/AddressSpaceRandomization.cpp:2: * Copyright (C) 2013 Google Inc. All
rights reserved.
nit: no (C), year 2014.

https://codereview.chromium.org/343753004/diff/20001/Source/wtf/AddressSpaceR...
Source/wtf/AddressSpaceRandomization.cpp:53: uint32_t ranvalInternal(ranctx* x)
nit: can this be static or empty namespace'd? I know we didn't do it that way,
but I don't see any sense in exposing it.

https://codereview.chromium.org/343753004/diff/20001/Source/wtf/AddressSpaceR...
Source/wtf/AddressSpaceRandomization.cpp:65: uint32_t ranval(ranctx* x)
ditto

https://codereview.chromium.org/343753004/diff/20001/Source/wtf/AddressSpaceR...
File Source/wtf/AddressSpaceRandomization.h (right):

https://codereview.chromium.org/343753004/diff/20001/Source/wtf/AddressSpaceR...
Source/wtf/AddressSpaceRandomization.h:2: * Copyright (C) 2013 Google Inc. All
rights reserved.
nit: no (C), year 2014.

[zerny-chromium, 2014-06-27 04:33:18]

lgtm

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:333: static PageMemory* allocate(size_t
payloadSize)
This appears unused now that we chunk allocate in ThreadHeap::allocatePage.

[haraken, 2014-06-27 05:36:33]

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:177: // space containing a number of blink heap
pages. On Window, reserved

Window => Windows

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:179: // whole. The PageMemoryRegion allows us to
do that be keeping track

be keeping => by keeping

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:224: // If the allocate memory was not blink page
aligned release

allocate => allocated

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:794: // Allocate a memory region for
blinkPagesPerRegion pages that

We might want to encapsulate this logic in PageMemory's method. For example, you
can keep PageMemory::allocate and do the below logic in the
PageMemory::allocate.

It's a bit inconsistent that ThreadHeap adds Pages to PageMemoryRegion, but
PageMemory removes Pages from PageMemoryRegion.

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:803: ASSERT(blinkPageSize ==
blinkPagePayloadSize() + 2 * osPageSize());

This assert won't make much sense, since blinkPageloadSize() is calculated as
'blinkPageSize - 2 * osPageSize()'.

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:804: Address payload = region->base() +
osPageSize();

Shall we assert that payload is aligned with blinkPageSize?

[Mads Ager (chromium), 2014-06-27 06:16:36]

Haraken, PTAL

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:177: // space containing a number of blink heap
pages. On Window, reserved
On 2014/06/27 05:36:33, haraken wrote:
> 
> Window => Windows

Done.

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:179: // whole. The PageMemoryRegion allows us to
do that be keeping track
On 2014/06/27 05:36:33, haraken wrote:
> 
> be keeping => by keeping

Done.

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:224: // If the allocate memory was not blink page
aligned release
On 2014/06/27 05:36:33, haraken wrote:
> 
> allocate => allocated

Done.

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:333: static PageMemory* allocate(size_t
payloadSize)
On 2014/06/27 04:33:18, zerny-chromium wrote:
> This appears unused now that we chunk allocate in ThreadHeap::allocatePage.

It is used to allocate PageMemory for just one page which we use for our large
pages where the payload can be larger than blinkPagePayloadSize().

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:794: // Allocate a memory region for
blinkPagesPerRegion pages that
On 2014/06/27 05:36:33, haraken wrote:
> 
> We might want to encapsulate this logic in PageMemory's method. For example,
you
> can keep PageMemory::allocate and do the below logic in the
> PageMemory::allocate.

We still do have PageMemory::allocate for allocating one page. That is currently
used for large objects. That page should not be added to the page pool and
therefore there are two different paths.

> It's a bit inconsistent that ThreadHeap adds Pages to PageMemoryRegion, but
> PageMemory removes Pages from PageMemoryRegion.

PageMemory doesn't know anything about the page pool which is what we have to
add the pages to. I can create a static

PageMemory* PageMemory::setupPage(region, payloadOffset, payloadSize)

method that will just do the 'new PageMemory' call in one place. What do you
think?

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:803: ASSERT(blinkPageSize ==
blinkPagePayloadSize() + 2 * osPageSize());
On 2014/06/27 05:36:33, haraken wrote:
> 
> This assert won't make much sense, since blinkPageloadSize() is calculated as
> 'blinkPageSize - 2 * osPageSize()'.

Removed. I put it in here for my own sanity while coding. It is true by
definition and there is no need to check in debug mode. :)

https://codereview.chromium.org/343753004/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:804: Address payload = region->base() +
osPageSize();
On 2014/06/27 05:36:32, haraken wrote:
> 
> Shall we assert that payload is aligned with blinkPageSize?

No, because it isn't. :-) See the ascii art above. The guard page is blink page
size aligned, they payload isn't.

https://codereview.chromium.org/343753004/diff/20001/Source/wtf/AddressSpaceR...
File Source/wtf/AddressSpaceRandomization.cpp (right):

https://codereview.chromium.org/343753004/diff/20001/Source/wtf/AddressSpaceR...
Source/wtf/AddressSpaceRandomization.cpp:2: * Copyright (C) 2013 Google Inc. All
rights reserved.
On 2014/06/26 17:06:54, Tom Sepez wrote:
> nit: no (C), year 2014.

Done.

https://codereview.chromium.org/343753004/diff/20001/Source/wtf/AddressSpaceR...
Source/wtf/AddressSpaceRandomization.cpp:53: uint32_t ranvalInternal(ranctx* x)
On 2014/06/26 17:06:54, Tom Sepez wrote:
> nit: can this be static or empty namespace'd? I know we didn't do it that way,
> but I don't see any sense in exposing it.

Yes, it definitely can and I agree that there is no sense in exposing it. Put
everything except for the exported function in an anonymous namespace.

https://codereview.chromium.org/343753004/diff/20001/Source/wtf/AddressSpaceR...
Source/wtf/AddressSpaceRandomization.cpp:65: uint32_t ranval(ranctx* x)
On 2014/06/26 17:06:54, Tom Sepez wrote:
> ditto

Done.

https://codereview.chromium.org/343753004/diff/20001/Source/wtf/AddressSpaceR...
File Source/wtf/AddressSpaceRandomization.h (right):

https://codereview.chromium.org/343753004/diff/20001/Source/wtf/AddressSpaceR...
Source/wtf/AddressSpaceRandomization.h:2: * Copyright (C) 2013 Google Inc. All
rights reserved.
On 2014/06/26 17:06:54, Tom Sepez wrote:
> nit: no (C), year 2014.

Thanks. Changed to the new style copyright notice from
http://www.chromium.org/developers/coding-style.

[haraken, 2014-06-27 06:50:17]

LGTM

https://codereview.chromium.org/343753004/diff/60001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/343753004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:803: // will each have the following layout.

I guess "each" is confusing. Also "... payload ..." is confusing since the
"payload" of "... payload ..." is different from the "payload" of
blinkPage"Payload"Size().

Slightly better:

Allocate a memory region for blinkPagesPerRegion pages that have the following
layout.

[guard os page | page payload | page payload | ... | page payload | guard os
page]
^---{ aligned to blink page size }

[Mads Ager (chromium), 2014-06-27 07:01:04]

https://codereview.chromium.org/343753004/diff/60001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/343753004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:803: // will each have the following layout.
On 2014/06/27 06:50:17, haraken wrote:
> 
> I guess "each" is confusing. Also "... payload ..." is confusing since the
> "payload" of "... payload ..." is different from the "payload" of
> blinkPage"Payload"Size().
> 
> Slightly better:
> 
> Allocate a memory region for blinkPagesPerRegion pages that have the following
> layout.
> 
> [guard os page | page payload | page payload | ... | page payload | guard os
> page]
> ^---{ aligned to blink page size }

They layout that we get in a region is:

[guard | payload | guard | guard | payload | guard ]
--------------------------
|       page size        |

^ size aligned           ^ size aligned

So it really is for that many pages that each have the layout of the ascii art.
:)

[Mads Ager (chromium), 2014-06-27 07:02:36]

Thanks for the reviews guys. I'll put this in the commit queue. Chris, when you
have the time, can you have quick look at the extraction of the ASR code? I'll
address any issues you might have in follow-ups. :)

[Mads Ager (chromium), 2014-06-27 07:02:40]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-06-27 07:03:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/343753004/60001

[haraken, 2014-06-27 07:16:43]

https://codereview.chromium.org/343753004/diff/60001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/343753004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:803: // will each have the following layout.
On 2014/06/27 07:01:04, Mads Ager (chromium) wrote:
> On 2014/06/27 06:50:17, haraken wrote:
> > 
> > I guess "each" is confusing. Also "... payload ..." is confusing since the
> > "payload" of "... payload ..." is different from the "payload" of
> > blinkPage"Payload"Size().
> > 
> > Slightly better:
> > 
> > Allocate a memory region for blinkPagesPerRegion pages that have the
following
> > layout.
> > 
> > [guard os page | page payload | page payload | ... | page payload | guard os
> > page]
> > ^---{ aligned to blink page size }
> 
> They layout that we get in a region is:
> 
> [guard | payload | guard | guard | payload | guard ]
> --------------------------
> |       page size        |
> 
> ^ size aligned           ^ size aligned
> 
> So it really is for that many pages that each have the layout of the ascii
art.
> :)

Now I understood!

[commit-bot: I haz the power, 2014-06-27 08:08:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/8700)

[commit-bot: I haz the power, 2014-06-27 08:11:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-27 08:11:59]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/8704)

[Mads Ager (chromium), 2014-06-27 08:15:01]

Erik, could you do the owner review for wtf?

[Erik Corry, 2014-06-27 09:42:26]

LGTM

The comments and the bit masks don't really line up too well.

[Mads Ager (google), 2014-06-27 09:46:53]

The CQ bit was checked by ager@google.com

[commit-bot: I haz the power, 2014-06-27 09:47:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/343753004/60001

[commit-bot: I haz the power, 2014-06-27 09:51:20]

Change committed as 177085