Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1383153002:
Fix PartitionAlloc randomization on 32-bit systems (Closed)
DescriptionFix PartitionAlloc randomization on 32-bit systems
Make PartitionAlloc reduce churn on Windows system allocations, and use less aggressive randomization on 32-bit systems in general.
BUG=538479, 394591
TBR=jchaffraix@chromium.org
Committed: https://crrev.com/794a0e84c6d07da73b687bda35cea145310d2c76
Cr-Commit-Position: refs/heads/master@{#352477}
Patch Set 1 : wip #Patch Set 2 : discard tweak #Patch Set 3 : linux build fix #Patch Set 4 : move discardable memory to a separate CL #Patch Set 5 : slack #
Total comments: 6
Patch Set 6 : action feedback and remove explicit commit #
Total comments: 11
Patch Set 7 : random fix #Patch Set 8 : jfb was half right #
Total comments: 2
Messages
Total messages: 40 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #3 (id:60002) has been deleted
The CQ bit was checked by jschuh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1383153002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1383153002/90001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jschuh@chromium.org changed reviewers: + tsepez@chromium.org
I switched to a reserve-commit approach for allocation probing, to make it more performant on Windows, and dropped the randomization entirely for 32-bit Windows hosts. I also now try a linear probe on all 32-bit platforms, where fragmentation is a serious problem and ASLR is pretty dead anyway.
The CQ bit was checked by jschuh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1383153002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1383153002/130001
https://codereview.chromium.org/1383153002/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/PageAllocator.cpp (right): https://codereview.chromium.org/1383153002/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:116: // Use a linear probe on 32-bit systems, where the address space tends to be cramped. Thought you were going to try 3 random allocations before falling back to probing.
https://codereview.chromium.org/1383153002/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/PageAllocator.cpp (right): https://codereview.chromium.org/1383153002/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:104: if (!(reinterpret_cast<uintptr_t>(ret) & alignOffsetMask)) { nit: maybe reads cleaner if you test !ret and early return above rather than testing inside this block. https://codereview.chromium.org/1383153002/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:124: const size_t tryLen = len + (align - kPageAllocationGranularity); nit: the const doesn't buy you a lot here and isn't really needed.
On 2015/10/05 19:07:30, Tom Sepez wrote: > https://codereview.chromium.org/1383153002/diff/130001/third_party/WebKit/Sou... > File third_party/WebKit/Source/wtf/PageAllocator.cpp (right): > > https://codereview.chromium.org/1383153002/diff/130001/third_party/WebKit/Sou... > third_party/WebKit/Source/wtf/PageAllocator.cpp:116: // Use a linear probe on > 32-bit systems, where the address space tends to be cramped. > Thought you were going to try 3 random allocations before falling back to > probing. Nope. On 32-bit it starts at a random base and then does a linear probe upward from the random base. So, it should still be about as random, but more likely to hit something in the early attempts. On 64-bit it performs a random try each time. The fallback is the larger allocation that gets trimmed, which is still a random base on POSIX, but is not on Windows. The retry loop around the fallback is because it is theoretically possible to have a race on the Windows version, which can't trim. However, a retry should never be needed on POSIX.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
ptal https://codereview.chromium.org/1383153002/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/PageAllocator.cpp (right): https://codereview.chromium.org/1383153002/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:104: if (!(reinterpret_cast<uintptr_t>(ret) & alignOffsetMask)) { On 2015/10/05 19:13:31, Tom Sepez wrote: > nit: maybe reads cleaner if you test !ret and early return above rather than > testing inside this block. Removed the nested block entirely. https://codereview.chromium.org/1383153002/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:116: // Use a linear probe on 32-bit systems, where the address space tends to be cramped. On 2015/10/05 19:07:30, Tom Sepez wrote: > Thought you were going to try 3 random allocations before falling back to > probing. Explained in previous message. https://codereview.chromium.org/1383153002/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:124: const size_t tryLen = len + (align - kPageAllocationGranularity); On 2015/10/05 19:13:31, Tom Sepez wrote: > nit: the const doesn't buy you a lot here and isn't really needed. Done.
lgtm
jfb@chromium.org changed reviewers: + jfb@chromium.org
https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/PageAllocator.cpp (right): https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:124: #if OS(POSIX) && (CPU(X86_64) && CPU(ARM64)) This check is wrong.
https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/PageAllocator.cpp (right): https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:124: #if OS(POSIX) && (CPU(X86_64) && CPU(ARM64)) On 2015/10/05 20:18:09, JF wrote: > This check is wrong. Good catch. Would have lost that extra randomness on the retry.
The CQ bit was checked by jschuh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1383153002/#ps170001 (title: "random fix")
https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/PageAllocator.cpp (right): https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:73: ret = VirtualAlloc(0, len, MEM_RESERVE | MEM_COMMIT, accessFlag); It's not clear what the intent of the fallback is. An address was passed in and allocation failed at that location, so we than ask the OS to allocate wherever it feels. Most of the time that won't respect alignment expectations. At least a comment would be good. In fact, partition alloc sometimes asks for a specific page so addr isn't just random location (PA asks for "next super page") so this code breaks PA's (maybe misguided) expectation. https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:91: size_t alignBaseMask = ~alignOffsetMask; I know this isn't new, but these should be uintptr_t and not size_t since they're masks and not offsets. https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:110: addr = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(addr)& alignBaseMask); Nit clang-format. https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:114: addr = reinterpret_cast<void*>((reinterpret_cast<uintptr_t>(ret) + align) & alignBaseMask); & alignBaseMask isn't required here, I'd assert on it instead. https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:115: #endif Same as below, I'd avoid the preprocessor and just test sizeof(void*). https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:123: addr = nullptr; This drops the explicit address that's passed in to allocPages. I'm guessing that you'd rather remove that parameter outright? Add TODO for this. https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:124: #if OS(POSIX) && (CPU(X86_64) && CPU(ARM64)) You should have a static_assert here that sizeof(void*)==4, and the reverse for 32-bit. Listing the ones we know today to be 64-bit is pretty brittle otherwise. Actually, why not just have: addr = sizeof(void*) == 4 ? 0 : getRandomPageBase(); https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:128: if (!ret) This drops the alignment requirement. https://codereview.chromium.org/1383153002/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:130: size_t preSlack = reinterpret_cast<uintptr_t>(ret) & (align - 1); align - 1 is alignOffsetMask.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1383153002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1383153002/170001
The CQ bit was unchecked by jschuh@chromium.org
The CQ bit was checked by jschuh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1383153002/#ps190001 (title: "jfb was half right")
The CQ bit was unchecked by jschuh@chromium.org
jschuh@chromium.org changed reviewers: + jchaffraix@chromium.org
I figure it's safe to TBR jchafraix@ as a wtf owner since tsepez@ reviewed the CL as one of the PartitionAlloc authors, and really should be an owner for the code.
The CQ bit was checked by jschuh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1383153002/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1383153002/190001
Post-jschuh-shoulder-surf-pair-program lgtm.
Message was sent while issue was closed.
Committed patchset #8 (id:190001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/794a0e84c6d07da73b687bda35cea145310d2c76 Cr-Commit-Position: refs/heads/master@{#352477}
Message was sent while issue was closed.
jchaffraix@chromium.org changed reviewers: + haraken@chromium.org
Message was sent while issue was closed.
Rubber stamp lgtm. Haraken is really a better reviewer for any PartitionAlloc change.
Message was sent while issue was closed.
LGTM with comments. https://codereview.chromium.org/1383153002/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/PageAllocator.cpp (right): https://codereview.chromium.org/1383153002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:124: #if OS(POSIX) && CPU(32BIT) Is this condition correct? Do we want to get a random base in 32bit architecture? https://codereview.chromium.org/1383153002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PageAllocator.cpp:130: size_t preSlack = reinterpret_cast<uintptr_t>(ret) & alignOffsetMask; I'd prefer keeping this trimming logic in a helper method, trimMapping().
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:190001) has been created in https://codereview.chromium.org/1397513003/ by glider@chromium.org. The reason for reverting is: This CL broke TSan bots, see https://crbug.com/539863. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
