Introduce v8_enable_virtual_memory_cage [v8/v8 : main]
Samuel Groß (Gerrit)
Attention is currently required from: Samuel Groß.
Set Ready For Review
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Jann Horn (Gerrit)
Attention is currently required from: Michael Lippautz, Samuel Groß.
1 comment:
File src/api/api.cc:
void* Allocate(size_t length) override {
// Page allocator returns zero-initialized pages.
return AllocateUninitialized(length);
}
void* AllocateUninitialized(size_t length) override {
size_t page_size = page_allocator_->AllocatePageSize();
return page_allocator_->AllocatePages(nullptr, RoundUp(length, page_size),
page_size, PageAllocator::kReadWrite);
}Maybe write this the other way around?
"Implement uninitialized memory allocation by allocating zeroed memory"
makes more sense than
"Implement zeroed memory allocation by allocating uninitialized memory"
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Jann Horn (Gerrit)
Attention is currently required from: Michael Lippautz.
1 comment:
File src/base/platform/platform-posix.cc:
Patch Set #7, Line 494:
bool OS::DecommitPages(void* address, size_t size) {Non-actionable comment, just for the record:
On Linux, the differences between the MAP_FIXED approach vs just having a big RW region with MADV_DONTNEED are, as far as I know:
- obviously with a big RW region, accesses to unused addresses
won't fault
- with MADV_DONTNEED, reallocation doesn't require mmap or mprotect
(so the kernel doesn't have to scan through the page tables)
- clobbering the region with MAP_FIXED will delete page tables,
MADV_DONTNEED will leave the page tables intact
- MADV_DONTNEED only takes the mmap semaphore in read mode and
uses locks at the page table level (2 MiB granularity), so it
can run in parallel with page faults; MAP_FIXED holds the
mmap semaphore in write mode while unmapping, so parallel page
faults in the same process will block until the mmap operation
is complete
- currently page fault handling first tries to find the VMA in
the per-thread "vmacache" (which is a 1-way 4-set cache), then
falls back to a binary tree walk on failure.
so if you alternatingly have page faults in two separate VMAs,
there's a 25% chance that each fault will miss in the vmacache
and do a binary tree walk; but if they're the same VMA, they'll
all hit
- for RLIMIT_DATA purposes a huge RW VMA would be accounted as
memory usage (and Chrome limits RLIMIT_DATA for renderers to 8
or 16 GiB)
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Samuel Groß.
7 comments:
Patchset:
Overall the approach sg
I checked now V8's API and will look at the rest next week. The biggest challenge I see which you already started tackling is that we need to keep v8::PageAllocator, the default platform (d8), and Chrome (PartitionAlloc) in sync wrt to the expected behavior of "Decommit".
File include/v8-platform.h:
Patch Set #7, Line 439:
kNoAccessfwiw, Chrome may not do that for performance reasons :/, see [1].
I suspect that a similar issue would apply here.
This is similar to DiscardSystemPages w/ the difference that it guarantees zero initialization when touching memory, right?
Patch Set #7, Line 449:
DecommitPages(I realize you kicked off the discussion with PA folks. We should incorporate their feedback here.)
he embedder is expected to reserve
* the virtual memoryIs that because you want to create this reservation as early as possible and no V8 is running at this point?
To turn it around: Why is it not good enough to just use the provided v8::PageAllocator to allocate the cage ourselves transparently?
Patch Set #7, Line 651:
GetDataCagePageAllocatorCould we expect that `GetPageAllocator()` returns the cage-based allocator in case a VirtualMemoryCage has been passed to v8?
File include/v8.h:
Patch Set #7, Line 5429:
NewCagedAllocatorDo we need to bind this to the term "Cage" here?
Couldn't it just be a "ManagedAllocator" that uses the platform provided PageAllocator (cage-based) internally?
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Igor Sheludko (Gerrit)
Patchset:
First round of comments.
File include/v8-platform.h:
Patch Set #7, Line 445:
* TODO(v8) can this be merged with any of the other similarly-sounding APIs?Sounds like an alias for SetPermissions(address, size, kNoAccess) call. Doesn't it?
Patch Set #7, Line 524:
and pass and exposeMaybe "and pass/expose"?
Patch Set #7, Line 542:
GuardJust an idea.
I wonder if it gives us anything if we "expose" the guard regions existence in the API? Can we just expose the required/recommended guard region size via respective non-constant method and "ask" embedder to reserve those around the usable cage?Patch Set #7, Line 590:
kPointerCageSizeMaybe k[V8]HeapCageSize.
File src/api/api.cc:
Patch Set #7, Line 397:
size_t page_size = page_allocator_->AllocatePageSize();The page size value can be cached in a constant field.
File src/d8/d8.cc:
Spurious change?
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Michael Lippautz, Igor Sheludko.
15 comments:
Patchset:
First round of comments.
Thanks!
Overall the approach sg […]
Cool, thanks!
File include/v8-platform.h:
Patch Set #7, Line 439:
kNoAccessfwiw, Chrome may not do that for performance reasons :/, see [1]. […]
Right, ok. I guess either Chrome will do it when implementing the new API (if we introduce it), e.g. by doing it the same way it is currently implemented in base/platform (i.e. just mmap(MAP_FIXED, PROT_NONE) or VirtualFree) or we just drop that requirement and do a SetPermissions ourselves if we deem it important enough
This is similar to DiscardSystemPages w/ the difference that it guarantees zero initialization when […]
Yes, precisely (and maybe it also guarantees PROT_NONE for the region afterwards, although that is not really important since we could just do a SetPermission ourselves)
Patch Set #7, Line 445:
* TODO(v8) can this be merged with any of the other similarly-sounding APIs?
Sounds like an alias for SetPermissions(address, size, kNoAccess) call. […]
The issue is that, from what I can tell, none of the other APIs guarantee that the page content will be zero on next access. `SetPermissions(address, size, kNoAccess)` doesn't seem to guarantee that, even though it would do the right thing on Windows, and probably on Linux, but not on Apple platforms (See e.g. https://source.chromium.org/chromium/chromium/src/+/main:base/allocator/partition_allocator/page_allocator.h;drc=0ec17e4452120719f6bf5f86f50c781123ee8f5d;bpv=1;bpt=1;l=161 and https://source.chromium.org/chromium/chromium/src/+/main:base/allocator/partition_allocator/page_allocator_internals_posix.h;l=329;drc=0ec17e4452120719f6bf5f86f50c781123ee8f5d;bpv=1;bpt=1). `DiscardSystemPages` seems to be mostly advisory and doesn't guarantee zero initialization either, even though it again probably works correctly on Windows and Linux (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/base/platform/platform-posix.cc;l=466;drc=0ec17e4452120719f6bf5f86f50c781123ee8f5d;bpv=1;bpt=1). So I guess the question is which one is worse: introducing a new API (currently the case) or changing the contract of an existing API.
Patch Set #7, Line 524:
and pass and exposeMaybe "and pass/expose"?
Done
he embedder is expected to reserve
* the virtual memory
Is that because you want to create this reservation as early as possible and no V8 is running at thi […]
Yes, essentially. (I think) the issue is that PartitionAlloc creates its ArrayBuffer partition/pool very early during initialization, but with this cage, that partition needs to be inside the cage. So the way I currently imaging things will work is
1. PartitionAlloc initializes itself, reserving the V8 cage and placing its ArrayBuffer partition inside of that
2. later on, gin initializes V8 and constructs an instance of the VirtualMemoryCage class (as well as the PageAllocators) from some API exposed by PartitionAllocThis is roughly how my ArrayBufferCaging prototypes worked (https://chromium-review.googlesource.com/c/chromium/src/+/2928518 and https://chromium-review.googlesource.com/c/v8/v8/+/2928180)
The other way around might also work, but only if no ArrayBuffers are allocated before V8 is initialized, and it will probably require more significant changes to PartitionAlloc. In any case, I think PartitionAlloc will need to be aware to some degree of V8 internals (i.e., the cage size and its rough layout)...
Patch Set #7, Line 542:
Guard
Just an idea. […]
Right, I think currently, a V8 embedder would just use `VirtualMemoryCage::kOptimalSize` to reserve the memory, then construct the cage instance from that and maybe query `DataCageBase()` and `DataCageSize()` to create the page- and ArrayBuffer allocators without worrying about other internals of the cage. So for that we don't need to expose the guard regions (they are just public now because they are needed for other public constants...).
However, probably PartitionAlloc will need to be aware of the layout of the cage (see discussion in other comments, but basically, it'll probably need to create it's ArrayBuffer partition inside the DataCage part before V8 is initialized), and so maybe we should just expose the entire layout to the embedder?
Patch Set #7, Line 590:
kPointerCageSizeMaybe k[V8]HeapCageSize.
Ack
I guess this is part of the overall TODO of "figure out how to properly name these things". I'm fine with doing s/PointerCage/HeapCage/.
Patch Set #7, Line 651:
GetDataCagePageAllocator
Could we expect that `GetPageAllocator()` returns the cage-based allocator in case a VirtualMemoryCa […]
Unfortunately, I think we can't, and we'll continue to need two page allocators: one that always allocates inside the cage and one that always allocates outside the cage. For example, there is the Code region and maybe other sensitive memory areas that V8 creates which must not be corrupted by an attacker (which they easily could if they were inside the cage).
I guess we'll need to make it clear that the other allocator must *not* allocate pages inside the cage though, good point. Or maybe we should even rename the other PageAllocator if the virtual memory cage is enabled, to something like "GetUncagedPageAllocator()".
File include/v8.h:
Patch Set #7, Line 5429:
NewCagedAllocator
Do we need to bind this to the term "Cage" here? […]
No, it doesn't need to have the name "Cage" in it. I just couldn't think of a better name (as usual) that concisely expresses that this allocator will allocate its backing memory using the provided page allocator. I'm fine with calling it "ManagedAllocator", but I think I'd still have the client explicitly pass a page allocator instance to it, since there'll be at least two different ones when the virtual memory cage is enabled. I'm also fine with keeping it "CagedAllocator" but having it automatically use the PlatformDataCagePageAllocator instead of requiring the client to pass it one.
File src/api/api.cc:
void* Allocate(size_t length) override {
// Page allocator returns zero-initialized pages.
return AllocateUninitialized(length);
}
void* AllocateUninitialized(size_t length) override {
size_t page_size = page_allocator_->AllocatePageSize();
return page_allocator_->AllocatePages(nullptr, RoundUp(length, page_size),
page_size, PageAllocator::kReadWrite);
}
Maybe write this the other way around? […]
Done
Patch Set #7, Line 397:
size_t page_size = page_allocator_->AllocatePageSize();The page size value can be cached in a constant field.
Done
File src/base/platform/platform-posix.cc:
Patch Set #7, Line 494:
bool OS::DecommitPages(void* address, size_t size) {
Non-actionable comment, just for the record: […]
Great, thanks Jann!
File src/d8/d8.cc:
Spurious change?
Done
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Michael Lippautz, Igor Sheludko.
1 comment:
Patchset:
Just FYI. This CL will probably either add a new method (for example called DecommitPages) to the PageAllocator interface or change an existing one to guarantee zero (re-)initialization of the memory. This then needs to be implemented by partition alloc and exposed through gin/ on the Chromium side in a follow-up CL.
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Samuel Groß, Igor Sheludko.
15 comments:
Patchset:
Sorry for the delay. AFAICS, this is already in pretty good shape. Most comments are related to the API surface and what we need where.
File include/v8-platform.h:
Patch Set #8, Line 554:
base_(0), size_(0)nit: move initalizers to field decl where possible
nit: Here and below: Add `const` to getters.
Patch Set #8, Line 568:
size_tnit: uintptr_t
Patch Set #8, Line 593:
kGuardRegionSizenit: I'd move those comments up.
Patch Set #8, Line 601:
kOptimalSizeWhile I understand their intention, the constants should have (doxygen) comments.
Patch Set #8, Line 631:
VirtualMemoryCagePass-by-value intended? If we only need to return a reference, then maybe we should delete copy ctor?
Patch Set #8, Line 640:
bool HasVirtualMemoryCage() { return GetVirtualMemoryCage().IsValid(); }nit: const
File include/v8-platform.h:
he embedder is expected to reserve
* the virtual memory
Yes, essentially. […]
Would it be an option to have a static V8 call that is allowed to be called without having anything else set up already?
E.g., `InitializeGlobalVirtualMemoryCage(PageAllocator*)`
That would allow us to hide all the details.
Patch Set #7, Line 651:
GetDataCagePageAllocator
Unfortunately, I think we can't, and we'll continue to need two page allocators: one that always all […]
I would just add that the allocators work on disjoint areas of memory and that GetPageAllocator() returns an allocator that must not allocate in the cage.
File include/v8.h:
Patch Set #8, Line 5429:
page_allocatorWhy do we need that parameter here? I was assuming that we could (internally) wrap the `v8::Platform::GetDataCagePageAllocator()`.
I have hard time seeing why we allocate ABs before we have a proper Isolate. What would be the usage?
Reading this again after going through the API: I think this could even be implemented purely in the embedder since we merely wrap the `PageAllocator*` provided. In that it could also be part of the default platform to have it in d8 or cctest?
File src/execution/isolate.h:
Patch Set #8, Line 1075:
1218005nit: here and elsewhere chromium:1218005
Do we really want `<=` here?
(I am aware that the one-past-end of array is a valid C ptr but unsure whether we need the semantics here.)
File src/utils/allocation.cc:
Patch Set #8, Line 58:
V8_VIRTUAL_MEMORY_CAGE_BOOLShould we add a BUILD.gn assert for `is_lsan` instead or in addition?
This comment applies for all the incompatible configs.
#endif
#ifdef V8_VIRTUAL_MEMORY_CAGEthen this could be an #elif
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Samuel Groß, Igor Sheludko.
2 comments:
File test/unittests/heap/unmapper-unittest.cc:
Patch Set #8, Line 226:
// when the virtual memory cage is enabled, the tracking page allocator won't benit: 'When'
managed by the Platform. As such,
// these tests don't work.We could pass in a new platform that wraps the old one where the tracking page allocator just wraps the existing platform's page allocator. wdyt?
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Michael Lippautz, Igor Sheludko.
18 comments:
Patchset:
Thanks! I think all remaining open comments now depend on the discussion about who reserves the virtual memory cage: https://groups.google.com/a/google.com/g/v8-heap-sandbox/c/BAiMVrh919k
File include/v8-platform.h:
Patch Set #8, Line 554:
base_(0), size_(0)nit: move initalizers to field decl where possible
Done
nit: Here and below: Add `const` to getters.
Done
Patch Set #8, Line 568:
size_tnit: uintptr_t
Done
File include/v8-platform.h:
Patch Set #7, Line 439:
kNoAccessRight, ok. I guess either Chrome will do it when implementing the new API (if we introduce it), e.g. […]
Done
Patch Set #7, Line 439:
kNoAccessRight, ok. I guess either Chrome will do it when implementing the new API (if we introduce it), e.g. […]
Done
Yes, precisely (and maybe it also guarantees PROT_NONE for the region afterwards, although that is n […]
Looks like PA folks are fine with adding this API: https://groups.google.com/a/google.com/g/v8-heap-sandbox/c/sxQyDtnShEg :)
So I'll remove this comment since we probably can't merge it with any of the other APIs at this point. I'll also add a comment do DiscardSystemPages to explain the differences better.
Patch Set #7, Line 449:
DecommitPages
(I realize you kicked off the discussion with PA folks. We should incorporate their feedback here. […]
I guess the question now is whether we want to force embedders to implement this always? It probably depends on whether we want to use it for this as well: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/cppgc/caged-heap.cc;l=55;drc=1fc240699da5a97b02824b568576ed3992cc1440 WDYT?
he embedder is expected to reserve
* the virtual memory
Would it be an option to have a static V8 call that is allowed to be called without having anything […]
Started a discussion with PA folks about this (https://groups.google.com/a/google.com/g/v8-heap-sandbox/c/BAiMVrh919k) so let's see what comes out of that. In general, I agree that the public interface would be nicer if we do it the way you suggested, at the cost of a slightly more complicated initialization procedure in Chromium.
Patch Set #7, Line 542:
GuardRight, I think currently, a V8 embedder would just use `VirtualMemoryCage::kOptimalSize` to reserve […]
Kicked off a discussion with PA about the public interface here: https://groups.google.com/a/google.com/g/v8-heap-sandbox/c/BAiMVrh919k if we do option 2, then we don't need to expose all these details to the embedder.
Patch Set #7, Line 651:
GetDataCagePageAllocator
I would just add that the allocators work on disjoint areas of memory and that GetPageAllocator() re […]
Done
File include/v8.h:
Patch Set #8, Line 5429:
page_allocator
Why do we need that parameter here? I was assuming that we could (internally) wrap the `v8::Platform […]
As discussed offline, I now just turned the default ArrayBuffer allocator into a caged allocator when V8_VIRTUAL_MEMORY_CAGE is enabled. That way, most of the custom allocators in d8.cc continue to work and there is no additional allocator exposed in api.cc
File include/v8.h:
Patch Set #7, Line 5429:
NewCagedAllocator
No, it doesn't need to have the name "Cage" in it. […]
Done
File src/execution/isolate.h:
Patch Set #8, Line 1075:
1218005nit: here and elsewhere chromium:1218005
Done
Do we really want `<=` here? […]
Yeah I guess we want `<` here, fixed.
File src/utils/allocation.cc:
Patch Set #8, Line 58:
V8_VIRTUAL_MEMORY_CAGE_BOOL
Should we add a BUILD.gn assert for `is_lsan` instead or in addition? […]
Good idea, done
#endif
#ifdef V8_VIRTUAL_MEMORY_CAGEthen this could be an #elif
Done
File test/unittests/heap/unmapper-unittest.cc:
Patch Set #8, Line 226:
// when the virtual memory cage is enabled, the tracking page allocator won't benit: 'When'
Done
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Patchset:
This lg now and can imho go in once we have a proper decomit+zero memory API.
Lmk if you want to somehow land earlier and how that would work 😊
File include/v8-platform.h:
This should be treated as
* a hint to the OS that the pages are no longer needed. It does not guarantee
* that the pages will be discarded immediately or at all.nit: Maybe factor out as separate CL that could go in any time.
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Igor Sheludko.
1 comment:
Patchset:
With the latest patchset, I've changed the way the cage is created. Now, v8 does the cage initialization as that clearly seemed to be the preferred option (see https://groups.google.com/a/google.com/g/v8-heap-sandbox/c/BAiMVrh919k). This *significantly* simplifies the public interface: now, v8 exposes only a `InitializeVirtualMemoryCage` and a `GetVirtualMemoryCageDataPageAllocator` function to the embedder. The embedder can then use the latter to allocate backing memory for its ArrayBufferAllocators. Everything else is now internal, and in particular, the VirtualMemoryCage class is now AllStatic to (hopefully) simplify things further a bit.
Let me know what you think of this!
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Ross McIlroy (Gerrit)
Attention is currently required from: Michael Lippautz, Samuel Groß, Igor Sheludko.
1 comment:
File include/v8.h:
Patch Set #11, Line 9937:
static bool InitializeVirtualMemoryCage();Drive-by comment: Could this be done as part of platform initialization (potentially with a bool IsVirtualMemoryCageEnabled() exposed by the platform implementation to specify whether or not to initialize the virtual cage if we want that to be embedder controlled).
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Ross McIlroy, Michael Lippautz, Igor Sheludko.
1 comment:
File include/v8.h:
Patch Set #11, Line 9937:
static bool InitializeVirtualMemoryCage();
Drive-by comment: Could this be done as part of platform initialization (potentially with a bool IsV […]
Yes, that's an option! I currently have it as a separate function mostly for the initial rollout (where the cage can still be disabled at runtime), so that the cage can easily be disabled (well, not initialized) if the feature isn't enabled and because we'll probably need the return value of this call for statistics (to determine how frequently the cage reservation fails), although I guess we could also log that in V8 directly.
Ultimately, the option to disable the cage at runtime will most likely be removed, at which point we could probably remove this function without requiring a `IsVirtualMemoryCageEnabled()` or similar from the embedder at all. That's why, for now, I just left a comment that the API is not yet stable.
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Ross McIlroy (Gerrit)
Attention is currently required from: Michael Lippautz, Samuel Groß, Igor Sheludko.
1 comment:
File include/v8.h:
Patch Set #11, Line 9937:
static bool InitializeVirtualMemoryCage();
Yes, that's an option! I currently have it as a separate function mostly for the initial rollout (wh […]
Makes sense. I'm happy to go with the separate function approach for now and re-evaluate when the design is finished.
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Samuel Groß, Igor Sheludko.
Patch set 12:Code-Review +1
5 comments:
Patchset:
lgtm % open comments. I will be out for the next two weeks.
Igor, could you review this as well, or find somebody else to from your team to have another look?
File src/base/platform/platform-posix.cc:
Patch Set #12, Line 501:
// mapping is established."nit: Maybe state the consequences of this being zeroed memory as well.
File src/base/platform/platform-win32.cc:
Patch Set #12, Line 912:
VirtualFreeTo double-check: That does bring it back with zeroed memory? (Not a Windows expert.)
File src/base/region-allocator.cc:
Patch Set #12, Line 209:
size_tnit: const
File src/init/vm-cage.h:
Patch Set #12, Line 59:
InitializeNow that I see this here. Can we:
a. Use DI and pass the page allocator that is used to set up the cage.
b. Create some unittests that with a default page allocator.
c. Make it an instance instead of static (performance?)a./b. should be possible, right?
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Michael Lippautz, Igor Sheludko.
9 comments:
Patchset:
Thanks!
File include/v8-platform.h:
Patch Set #8, Line 593:
kGuardRegionSize
nit: I'd move those comments up.
Done
Patch Set #8, Line 601:
kOptimalSize
While I understand their intention, the constants should have (doxygen) comments.
Done
File include/v8-platform.h:
Patch Set #7, Line 449:
DecommitPagesI guess the question now is whether we want to force embedders to implement this always? It probably […]
Done
he embedder is expected to reserve
* the virtual memory
Started a discussion with PA folks about this (https://groups.google.com/a/google. […]
Done
Patch Set #7, Line 542:
GuardKicked off a discussion with PA about the public interface here: https://groups.google.com/a/google. […]
Done
Patch Set #7, Line 590:
kPointerCageSizeAck […]
Done
File src/base/platform/platform-win32.cc:
Patch Set #12, Line 912:
VirtualFree
To double-check: That does bring it back with zeroed memory? (Not a Windows expert. […]
Yes, you need to `VirtualAlloc(MEM_COMMIT)` to bring it back, in which case the pages will be zeroed. I'll add a link to some appropriate documentation though!
File src/init/vm-cage.h:
Patch Set #12, Line 59:
Initialize
Now that I see this here. Can we: […]
Yes, a./b. should be possible, I'll do that next week. I'm also fine with making all of these instance fields and having a global instance of this class somewhere. I guess that would then be pretty similar to https://source.chromium.org/chromium/chromium/src/+/main:v8/src/init/isolate-allocator.cc;l=60;drc=0b5ec843cc9b883cfe9b295fdb812f3393f5453f then?
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Samuel Groß, Igor Sheludko.
1 comment:
File src/init/vm-cage.h:
Patch Set #12, Line 59:
Initialize
Yes, a./b. should be possible, I'll do that next week. I'm also fine with making all of these instance fields and having a global instance of this class somewhere. I guess that would then be pretty similar to https://source.chromium.org/chromium/chromium/src/+/main:v8/src/init/isolate-allocator.cc;l=60;drc=0b5ec843cc9b883cfe9b295fdb812f3393f5453f then?
Yip
(It would be amazing if all the cage-based allocators could be somehow unified but I don't immediately see an easy way and it's also not strictly required)
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Igor Sheludko.
8 comments:
File include/v8-platform.h:
This should be treated as
* a hint to the OS that the pages are no longer needed. It does not guarantee
* that the pages will be discarded immediately or at all.nit: Maybe factor out as separate CL that could go in any time.
As discussed, we'll land this CL in full
File include/v8-platform.h:
Patch Set #8, Line 631:
VirtualMemoryCagePass-by-value intended? If we only need to return a reference, then maybe we should delete copy ctor […]
Done
Patch Set #8, Line 640:
bool HasVirtualMemoryCage() { return GetVirtualMemoryCage().IsValid(); }nit: const
Done
File src/base/platform/platform-posix.cc:
Patch Set #12, Line 501:
// mapping is established."nit: Maybe state the consequences of this being zeroed memory as well.
Done
File src/base/platform/platform-win32.cc:
Patch Set #12, Line 912:
VirtualFree
Yes, you need to `VirtualAlloc(MEM_COMMIT)` to bring it back, in which case the pages will be zeroed […]
Done
File src/base/region-allocator.cc:
Patch Set #12, Line 209:
size_tnit: const
Done
File src/init/vm-cage.h:
Patch Set #12, Line 59:
Initialize
> Yes, a./b. should be possible, I'll do that next week. […]
Made it an instance class now with a global GetProcessWideVirtualMemoryCage getter. Once this CL is landed, I'll start a thread to discuss how to best unify the existing "cages" in V8.
File test/unittests/heap/unmapper-unittest.cc:
managed by the Platform. As such,
// these tests don't work.
We could pass in a new platform that wraps the old one where the tracking page allocator just wraps […]
I (think I) fixed the test now. It's now tearing down and re-initializing the virtual memory cage with the tracking page allocator for those tests, and that seems to work.
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Igor Sheludko (Gerrit)
Attention is currently required from: Samuel Groß.
Patch set 15:Code-Review +1
6 comments:
Patchset:
lgtm with nits
File include/v8-platform.h:
Patch Set #7, Line 445:
* TODO(v8) can this be merged with any of the other similarly-sounding APIs?The issue is that, from what I can tell, none of the other APIs guarantee that the page content will […]
Ack
File src/base/bounded-page-allocator.cc:
Patch Set #15, Line 145:
aboveMaybe "See comment in FreePages()." or just duplicate it here.
File src/init/vm-cage.h:
Patch Set #15, Line 62:
bool IsEnabled() const { return !disabled_; }Here and below: I think in V8 internals we use the underscore naming scheme for semi-trivial getters/setters and CamelCase naming when a method does something non-trivial.
File test/cctest/test-virtual-memory-cage.cc:
Patch Set #15, Line 36:
#endif // V8_COMPRESS_POINTERSOutdated comment.
File test/unittests/heap/unmapper-unittest.cc:
Patch Set #15, Line 363:
#endif // !V8_OS_FUCHSIA && !V8_VIRTUAL_MEMORY_CAGELooks like a spurious change.
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
5 comments:
Patchset:
Thanks!
File src/base/bounded-page-allocator.cc:
Patch Set #15, Line 145:
aboveMaybe "See comment in FreePages()." or just duplicate it here.
Done
File src/init/vm-cage.h:
Patch Set #15, Line 62:
bool IsEnabled() const { return !disabled_; }
Here and below: I think in V8 internals we use the underscore naming scheme for semi-trivial getters […]
Done
File test/cctest/test-virtual-memory-cage.cc:
Patch Set #15, Line 36:
#endif // V8_COMPRESS_POINTERSOutdated comment.
Done
File test/unittests/heap/unmapper-unittest.cc:
Patch Set #15, Line 363:
#endif // !V8_OS_FUCHSIA && !V8_VIRTUAL_MEMORY_CAGELooks like a spurious change.
Done
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Clemens Backes (Gerrit)
Attention is currently required from: Samuel Groß.
Patch set 16:Code-Review +1
1 comment:
Patchset:
src/d8 LGTM.
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Samuel Groß.
Patch set 16:Commit-Queue +2
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted this change.
Introduce v8_enable_virtual_memory_cage
When this is enabled, v8 reserves a large region of virtual address
space during initialization, at the start of which it will place its 4GB
pointer compression cage. The remainder of the cage is used to store
ArrayBuffer backing stores and WASM memory buffers. This will later
allow referencing these buffers from inside V8 through offsets from the
cage base rather than through raw pointers.
Bug: chromium:1218005
Change-Id: I300094b07f64985217104b14c320cc019f8438af
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3010195
Reviewed-by: Clemens Backes <clem...@chromium.org>
Reviewed-by: Igor Sheludko <ish...@chromium.org>
Reviewed-by: Michael Lippautz <mlip...@chromium.org>
Commit-Queue: Samuel Groß <sa...@google.com>
Cr-Commit-Position: refs/heads/master@{#76234}
---
M BUILD.gn
M include/v8-internal.h
M include/v8-platform.h
M include/v8.h
M src/api/api.cc
M src/base/bounded-page-allocator.cc
M src/base/bounded-page-allocator.h
M src/base/page-allocator.cc
M src/base/page-allocator.h
M src/base/platform/platform-fuchsia.cc
M src/base/platform/platform-posix.cc
M src/base/platform/platform-win32.cc
M src/base/platform/platform.h
M src/base/region-allocator.cc
M src/base/region-allocator.h
M src/d8/d8.cc
M src/execution/isolate.h
M src/flags/flag-definitions.h
M src/init/isolate-allocator.cc
M src/init/v8.cc
M src/init/v8.h
A src/init/vm-cage.cc
A src/init/vm-cage.h
M src/libplatform/default-platform.cc
M src/objects/backing-store.cc
M src/objects/js-array-buffer-inl.h
M src/utils/allocation.cc
M src/utils/allocation.h
M test/cctest/BUILD.gn
M test/cctest/cctest.cc
M test/cctest/heap/test-spaces.cc
A test/cctest/test-virtual-memory-cage.cc
M test/unittests/base/region-allocator-unittest.cc
M test/unittests/heap/unmapper-unittest.cc
34 files changed, 773 insertions(+), 27 deletions(-)
Zhi An Ng (Gerrit)
1 comment:
Patchset:
Hi, I think this is missing adding vm-cage.h to BUILD.gn, and also the bazel build is failing https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux64%20-%20bazel/761/overview, vm-cage.cc and vm-cage.h needs to be added to BUILD.baze as well.
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
1 comment:
Patchset:
Hi, I think this is missing adding vm-cage.h to BUILD. […]
Ah, thanks, would this be the right place for vm-cage.h in BUILD.gn: https://chromium-review.googlesource.com/c/v8/v8/+/3090973 ?
There is already a separate CL to fix the bazel build: https://chromium-review.googlesource.com/c/v8/v8/+/3089167
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Zhi An Ng (Gerrit)
1 comment:
Patchset:
Ah, thanks, would this be the right place for vm-cage.h in BUILD.gn: https://chromium-review.googlesource.com/c/v8/v8/+/3090973 ?
That is exactly where I would put it :)
There is already a separate CL to fix the bazel build: https://chromium-review.googlesource.com/c/v8/v8/+/3089167
Great, thanks!
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Milad Farazmand (Gerrit)
Attention is currently required from: Samuel Groß.
1 comment:
File src/base/platform/platform-posix.cc:
Patch Set #17, Line 503:
void* ptr = mmap(address, size, PROT_NONE,Hi Samuel,
I have a question about your recent CL https://crrev.com/c/3386801 which is related to this implementation of `DecommitPages`.
On AIX, calling mmap with MAP_FIXED on a pre allocated address will fail (it returns -1 instead of just discarding the pages, it will only work as above if special env variables are set).
Is the purpose of `DecommitPages` to zero-out the memory or de-allocate it? In which case why munmap can't be used?
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
File src/base/platform/platform-posix.cc:
Patch Set #17, Line 503:
void* ptr = mmap(address, size, PROT_NONE,
Hi Samuel, […]
Hi! the use case of DecommitPages is quite specific. Assuming we have an existing mapping, then we want to
- Free up any physical pages backing that mapping
- Change the permissions to PROT_NONE
- Guarantee that the pages will be zero-initialized on next access (after changing the permissions again)
- *Keep the virtual address space of the mapping reserved*
The last point is the reason why we can't use munmap here. In particular, for the V8 sandbox (https://docs.google.com/document/d/1FM4fQmIhEqPG8uGp5o9A-mnPB5BOeScZYpkHjo0KKA8/edit?usp=sharing) we need to be able to "free" pages inside the sandbox, but without "making holes" in the mapping backing the sandbox. Otherwise, unrelated data could end up inside the sandbox, which would break its security properties. Does that answer your question? Happy to elaborate further if that'd be helpful!
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Milad Farazmand (Gerrit)
File src/base/platform/platform-posix.cc:
Patch Set #17, Line 503:
void* ptr = mmap(address, size, PROT_NONE,
Hi! the use case of DecommitPages is quite specific. […]
Thank you for the information.
I think (in the case of AIX) it would be equal to just add a `munmap(address, size);` before this line to de-allocate the region before reallocating it again with the new flags, would that be correct?
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
File src/base/platform/platform-posix.cc:
Patch Set #17, Line 503:
void* ptr = mmap(address, size, PROT_NONE,
Thank you for the information. […]
Yeah that would probably work (most of the time). The issue is that there's now a race condition IIUC: another thread could place another mapping at the same address after the munmap but before the mmap. This will probably rarely happen during normal operations (but would then most likely lead to a hard-to-debug crash later on), but it could be fairly easy for an attacker to trigger this on purpose, undermining the security properties of the sandbox. So if this is the only way to get it to work, then I think we should `CHECK` that the mmap after the munmap succeeds, and otherwise crash the process, which probably isn't great, but still better than the alternatives.
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Milad Farazmand (Gerrit)
File src/base/platform/platform-posix.cc:
Patch Set #17, Line 503:
void* ptr = mmap(address, size, PROT_NONE,
Yeah that would probably work (most of the time). […]
Thanks again for the detailed information. Would a mutex help in this case? i.e
```
{
MutexGuard guard(rng_mutex.Pointer());
munmap(address, size);
void* ptr = mmap(address, size, PROT_NONE,
MAP_FIXED | MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
}
...
```
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
File src/base/platform/platform-posix.cc:
Patch Set #17, Line 503:
void* ptr = mmap(address, size, PROT_NONE,
Thanks again for the detailed information. Would a mutex help in this case? i.e […]
The problem is that you'd have to take the same mutex at *every mmap call in the binary* (so e.g. also in 3rd party libraries linked into the executable etc.). Otherwise you'd still race with those unrelated calls.
On Linux, there is at least one alternative to the `mmap(MAP_FIXED)` call: `madvise(MADV_DONTNEED)` (https://man7.org/linux/man-pages/man2/madvise.2.html), which, on Linux, guarantees that the pages will be zero-initialized on next access (it doesn't guarantee that on e.g. macOS). If AIX has a similar feature with similar guarantees, then that might be the best alternative. If not, I would assume that it's unlikely enough for an arbitrary mmap call to obtain the address that was just munmap'd (but it's also not unthinkable, e.g. if the kernel uses some kind of fifo free list for these mappings), so I'd probably just try the `munmap + CHECK(mmap())` approach and see if that ever crashes.
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.
Milad Farazmand (Gerrit)
File src/base/platform/platform-posix.cc:
Patch Set #17, Line 503:
void* ptr = mmap(address, size, PROT_NONE,
The problem is that you'd have to take the same mutex at *every mmap call in the binary* (so e.g. […]
Makes sense. Thank you for the suggestions. I have created this CL to implement the `unmap` workaround. We have not ported "heap sandbox/ pntr cage" to PPC yet so assuming it may not be used directly outside of unit tests:
https://chromium-review.googlesource.com/c/v8/v8/+/3403045
To view, visit change 3010195. To unsubscribe, or for help writing mail filters, visit settings.