Skip to content

Commit

Permalink
Revert "Reland "[cfi] WritableJit* objects don't act as a write scope…
Browse files Browse the repository at this point in the history 
… anymore""

This reverts commit e5f8886.

Reason for revert: Suspected for failure of  https://crrev.com/c/6052904.

Original change's description:
> Reland "[cfi] WritableJit* objects don't act as a write scope anymore"
>
> This is a reland of commit 867eb8b
>
> Clean reland. The fixes were submitted separately in these two CLs:
> * crrev.com/c/6039247
> * crrev.com/c/6044140
>
> Original change's description:
> > [cfi] WritableJit* objects don't act as a write scope anymore
> >
> > Enable the enforce_write_api by default for all JIT writer classes. With
> > this change, the objects will not act as write scopes in debug builds
> > anymore and require all writes to go through the corresponding writer
> > methods.
> >
> > Bug: 42203297
> > Change-Id: I68d5c8300874505ac0ea76f7ae949fdb06bc1057
> > Cq-Include-Trybots: luci.v8.try:v8_linux64_pku_dbg,v8_linux64_pku_rel
> > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6038840
> > Reviewed-by: Clemens Backes <[email protected]>
> > Commit-Queue: Stephen Röttger <[email protected]>
> > Reviewed-by: Samuel Groß <[email protected]>
> > Cr-Commit-Position: refs/heads/main@{#97348}
>
> Bug: 42203297
> Change-Id: I50e1f07fce6d2561defd12bfe37f6f02606876da
> Cq-Include-Trybots: luci.v8.try:v8_linux64_pku_dbg,v8_linux64_pku_rel
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6044122
> Reviewed-by: Clemens Backes <[email protected]>
> Commit-Queue: Stephen Röttger <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#97414}

Bug: 42203297
Change-Id: Id1e4334683cbe335ee9426014afddbd1e1ee23f8
Cq-Include-Trybots: luci.v8.try:v8_linux64_pku_dbg,v8_linux64_pku_rel
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6048808
Reviewed-by: Stephen Röttger <[email protected]>
Commit-Queue: Stephen Röttger <[email protected]>
Auto-Submit: Manos Koukoutos <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
Commit-Queue: Rubber Stamper <[email protected]>
Cr-Commit-Position: refs/heads/main@{#97437}
  • Loading branch information
@manoskouk
manoskouk authored and V8 LUCI CQ committed Nov 27, 2024
1 parent ae138ee commit 3356f4b
Show file tree
Hide file tree
Showing 10 changed files with 101 additions and 82 deletions.
2 changes: 1 addition & 1 deletion src/builtins/setup-builtins-internal.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ void SetupIsolateDelegate::ReplacePlaceholders(Isolate* isolate) {
Tagged<InstructionStream> istream = code->instruction_stream();
WritableJitAllocation jit_allocation = ThreadIsolation::LookupJitAllocation(
istream.address(), istream->Size(),
ThreadIsolation::JitAllocationType::kInstructionStream);
ThreadIsolation::JitAllocationType::kInstructionStream, true);
bool flush_icache = false;
for (WritableRelocIterator it(jit_allocation, istream,
code->constant_pool(), kRelocMask);
Expand Down
84 changes: 47 additions & 37 deletions src/common/code-memory-access-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ RwxMemoryWriteScope::~RwxMemoryWriteScope() {

WritableJitAllocation::~WritableJitAllocation() {
#ifdef DEBUG
if (page_ref_.has_value()) {
if (enforce_write_api_ && page_ref_.has_value()) {
// We disabled RWX write access for debugging. But we'll need it in the
// destructor again to release the jit page reference.
write_scope_.emplace("~WritableJitAllocation");
Expand All @@ -47,7 +47,7 @@ WritableJitAllocation::~WritableJitAllocation() {

WritableJitAllocation::WritableJitAllocation(
Address addr, size_t size, ThreadIsolation::JitAllocationType type,
JitAllocationSource source)
JitAllocationSource source, bool enforce_write_api)
: address_(addr),
// The order of these is important. We need to create the write scope
// before we lookup the Jit page, since the latter will take a mutex in
Expand All @@ -56,29 +56,36 @@ WritableJitAllocation::WritableJitAllocation(
page_ref_(ThreadIsolation::LookupJitPage(addr, size)),
allocation_(source == JitAllocationSource::kRegister
? page_ref_->RegisterAllocation(addr, size, type)
: page_ref_->LookupAllocation(addr, size, type)) {
: page_ref_->LookupAllocation(addr, size, type)),
enforce_write_api_(enforce_write_api) {
#ifdef DEBUG
// Reset the write scope for debugging. We'll create fine-grained scopes in
// all Write functions of this class instead.
write_scope_.reset();
if (enforce_write_api_) {
// Reset the write scope for debugging. We'll create fine-grained scopes in
// all Write functions of this class instead.
write_scope_.reset();
}
#endif
}

WritableJitAllocation::WritableJitAllocation(
Address addr, size_t size, ThreadIsolation::JitAllocationType type)
: address_(addr), allocation_(size, type) {}
Address addr, size_t size, ThreadIsolation::JitAllocationType type,
bool enforce_write_api)
: address_(addr),
allocation_(size, type),
enforce_write_api_(enforce_write_api) {}

// static
WritableJitAllocation WritableJitAllocation::ForNonExecutableMemory(
Address addr, size_t size, ThreadIsolation::JitAllocationType type) {
return WritableJitAllocation(addr, size, type);
return WritableJitAllocation(addr, size, type, false);
}

// static
std::optional<RwxMemoryWriteScope>
ThreadIsolation::WriteScopeForApiEnforcement() {
WritableJitAllocation::WriteScopeForApiEnforcement() const {
#ifdef DEBUG
return std::optional<RwxMemoryWriteScope>("WriteScopeForApiEnforcement");
if (enforce_write_api_) {
return std::optional<RwxMemoryWriteScope>("WriteScopeForApiEnforcement");
}
#endif
return {};
}
Expand All @@ -90,10 +97,11 @@ WritableJumpTablePair::WritableJumpTablePair(Address jump_table_address,
Address far_jump_table_address,
size_t far_jump_table_size)
: writable_jump_table_(jump_table_address, jump_table_size,
ThreadIsolation::JitAllocationType::kWasmJumpTable),
ThreadIsolation::JitAllocationType::kWasmJumpTable,
true),
writable_far_jump_table_(
far_jump_table_address, far_jump_table_size,
ThreadIsolation::JitAllocationType::kWasmFarJumpTable),
ThreadIsolation::JitAllocationType::kWasmFarJumpTable, true),
write_scope_("WritableJumpTablePair"),
// Always split the pages since we are not guaranteed that the jump table
// and far jump table are on the same JitPage.
Expand All @@ -118,7 +126,8 @@ WritableJumpTablePair::WritableJumpTablePair(Address jump_table_address,

template <typename T, size_t offset>
void WritableJitAllocation::WriteHeaderSlot(T value) {
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
std::optional<RwxMemoryWriteScope> write_scope =
WriteScopeForApiEnforcement();
// This assert is no strict requirement, it just guards against
// non-implemented functionality.
static_assert(!is_taggable_v<T>);
Expand All @@ -133,7 +142,8 @@ void WritableJitAllocation::WriteHeaderSlot(T value) {

template <typename T, size_t offset>
void WritableJitAllocation::WriteHeaderSlot(Tagged<T> value, ReleaseStoreTag) {
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
std::optional<RwxMemoryWriteScope> write_scope =
WriteScopeForApiEnforcement();
// These asserts are no strict requirements, they just guard against
// non-implemented functionality.
static_assert(offset != HeapObject::kMapOffset);
Expand All @@ -144,7 +154,8 @@ void WritableJitAllocation::WriteHeaderSlot(Tagged<T> value, ReleaseStoreTag) {

template <typename T, size_t offset>
void WritableJitAllocation::WriteHeaderSlot(Tagged<T> value, RelaxedStoreTag) {
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
std::optional<RwxMemoryWriteScope> write_scope =
WriteScopeForApiEnforcement();
if constexpr (offset == HeapObject::kMapOffset) {
TaggedField<T, offset>::Relaxed_Store_Map_Word(
HeapObject::FromAddress(address_), value);
Expand All @@ -158,7 +169,8 @@ template <typename T, size_t offset>
void WritableJitAllocation::WriteProtectedPointerHeaderSlot(Tagged<T> value,
RelaxedStoreTag) {
static_assert(offset != HeapObject::kMapOffset);
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
std::optional<RwxMemoryWriteScope> write_scope =
WriteScopeForApiEnforcement();
TaggedField<T, offset, TrustedSpaceCompressionScheme>::Relaxed_Store(
HeapObject::FromAddress(address_), value);
}
Expand All @@ -167,7 +179,8 @@ template <typename T, size_t offset>
void WritableJitAllocation::WriteProtectedPointerHeaderSlot(Tagged<T> value,
ReleaseStoreTag) {
static_assert(offset != HeapObject::kMapOffset);
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
std::optional<RwxMemoryWriteScope> write_scope =
WriteScopeForApiEnforcement();
TaggedField<T, offset, TrustedSpaceCompressionScheme>::Release_Store(
HeapObject::FromAddress(address_), value);
}
Expand Down Expand Up @@ -197,15 +210,17 @@ V8_INLINE void WritableJitAllocation::WriteHeaderSlot(Address address, T value,
template <typename T>
V8_INLINE void WritableJitAllocation::WriteUnalignedValue(Address address,
T value) {
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
std::optional<RwxMemoryWriteScope> write_scope =
WriteScopeForApiEnforcement();
DCHECK_GE(address, address_);
DCHECK_LT(address - address_, size());
base::WriteUnalignedValue<T>(address, value);
}

template <typename T>
V8_INLINE void WritableJitAllocation::WriteValue(Address address, T value) {
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
std::optional<RwxMemoryWriteScope> write_scope =
WriteScopeForApiEnforcement();
DCHECK_GE(address, address_);
DCHECK_LT(address - address_, size());
base::Memory<T>(address) = value;
Expand All @@ -214,7 +229,8 @@ V8_INLINE void WritableJitAllocation::WriteValue(Address address, T value) {
template <typename T>
V8_INLINE void WritableJitAllocation::WriteValue(Address address, T value,
RelaxedStoreTag) {
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
std::optional<RwxMemoryWriteScope> write_scope =
WriteScopeForApiEnforcement();
DCHECK_GE(address, address_);
DCHECK_LT(address - address_, size());
reinterpret_cast<std::atomic<T>*>(address)->store(value,
Expand All @@ -223,40 +239,35 @@ V8_INLINE void WritableJitAllocation::WriteValue(Address address, T value,

void WritableJitAllocation::CopyCode(size_t dst_offset, const uint8_t* src,
size_t num_bytes) {
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
std::optional<RwxMemoryWriteScope> write_scope =
WriteScopeForApiEnforcement();
CopyBytes(reinterpret_cast<uint8_t*>(address_ + dst_offset), src, num_bytes);
}

void WritableJitAllocation::CopyData(size_t dst_offset, const uint8_t* src,
size_t num_bytes) {
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
std::optional<RwxMemoryWriteScope> write_scope =
WriteScopeForApiEnforcement();
CopyBytes(reinterpret_cast<uint8_t*>(address_ + dst_offset), src, num_bytes);
}

void WritableJitAllocation::ClearBytes(size_t offset, size_t len) {
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
std::optional<RwxMemoryWriteScope> write_scope =
WriteScopeForApiEnforcement();
memset(reinterpret_cast<void*>(address_ + offset), 0, len);
}

WritableJitPage::~WritableJitPage() {
#ifdef DEBUG
write_scope_.SetWritable();
#endif
}
WritableJitPage::~WritableJitPage() = default;

WritableJitPage::WritableJitPage(Address addr, size_t size)
: write_scope_("WritableJitPage"),
page_ref_(ThreadIsolation::LookupJitPage(addr, size)) {
#ifdef DEBUG
write_scope_.SetExecutable();
#endif
}
page_ref_(ThreadIsolation::LookupJitPage(addr, size)) {}

WritableJitAllocation WritableJitPage::LookupAllocationContaining(
Address addr) {
auto pair = page_ref_.AllocationContaining(addr);
return WritableJitAllocation(pair.first, pair.second.Size(),
pair.second.Type());
pair.second.Type(), false);
}

V8_INLINE WritableFreeSpace WritableJitPage::FreeRange(Address addr,
Expand All @@ -280,7 +291,6 @@ V8_INLINE WritableFreeSpace::WritableFreeSpace(base::Address addr, size_t size,
template <typename T, size_t offset>
void WritableFreeSpace::WriteHeaderSlot(Tagged<T> value,
RelaxedStoreTag) const {
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
Tagged<HeapObject> object = HeapObject::FromAddress(address_);
// TODO(v8:13355): add validation before the write.
if constexpr (offset == HeapObject::kMapOffset) {
Expand Down
34 changes: 17 additions & 17 deletions src/common/code-memory-access.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -472,23 +472,25 @@ bool ThreadIsolation::MakeExecutable(Address address, size_t size) {

// static
WritableJitAllocation ThreadIsolation::RegisterJitAllocation(
Address obj, size_t size, JitAllocationType type) {
Address obj, size_t size, JitAllocationType type, bool enforce_write_api) {
return WritableJitAllocation(
obj, size, type, WritableJitAllocation::JitAllocationSource::kRegister);
obj, size, type, WritableJitAllocation::JitAllocationSource::kRegister,
enforce_write_api);
}

// static
WritableJitAllocation ThreadIsolation::RegisterInstructionStreamAllocation(
Address addr, size_t size) {
return RegisterJitAllocation(addr, size,
JitAllocationType::kInstructionStream);
Address addr, size_t size, bool enforce_write_api) {
return RegisterJitAllocation(
addr, size, JitAllocationType::kInstructionStream, enforce_write_api);
}

// static
WritableJitAllocation ThreadIsolation::LookupJitAllocation(
Address addr, size_t size, JitAllocationType type) {
Address addr, size_t size, JitAllocationType type, bool enforce_write_api) {
return WritableJitAllocation(
addr, size, type, WritableJitAllocation::JitAllocationSource::kLookup);
addr, size, type, WritableJitAllocation::JitAllocationSource::kLookup,
enforce_write_api);
}

// static
Expand Down Expand Up @@ -677,9 +679,11 @@ WritableJumpTablePair ThreadIsolation::LookupJumpTableAllocations(

WritableJumpTablePair::~WritableJumpTablePair() {
#ifdef DEBUG
// We disabled RWX write access for debugging. But we'll need it in the
// destructor again to release the jit page reference.
write_scope_.SetWritable();
if (jump_table_pages_.has_value()) {
// We disabled RWX write access for debugging. But we'll need it in the
// destructor again to release the jit page reference.
write_scope_.SetWritable();
}
#endif
}

Expand All @@ -693,11 +697,7 @@ WritableJumpTablePair::WritableJumpTablePair(
writable_far_jump_table_(WritableJitAllocation::ForNonExecutableMemory(
far_jump_table_address, far_jump_table_size,
ThreadIsolation::JitAllocationType::kWasmFarJumpTable)),
write_scope_("for testing") {
#ifdef DEBUG
write_scope_.SetExecutable();
#endif
}
write_scope_("for testing") {}

// static
WritableJumpTablePair WritableJumpTablePair::ForTesting(
Expand All @@ -710,7 +710,8 @@ WritableJumpTablePair WritableJumpTablePair::ForTesting(

void WritableJumpTablePair::SetCodePointerTableEntry(uint32_t index,
Address target) {
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
std::optional<RwxMemoryWriteScope> write_scope =
writable_jump_table_.WriteScopeForApiEnforcement();

wasm::GetProcessWideWasmCodePointerTable()->SetEntrypointWithRwxWriteScope(
index, target, write_scope_);
Expand All @@ -720,7 +721,6 @@ void WritableJumpTablePair::SetCodePointerTableEntry(uint32_t index,

template <size_t offset>
void WritableFreeSpace::ClearTagged(size_t count) const {
auto write_scope = ThreadIsolation::WriteScopeForApiEnforcement();
base::Address start = address_ + offset;
// TODO(v8:13355): add validation before the write.
MemsetTagged(ObjectSlot(start), Tagged<Object>(kClearedFreeMemoryValue),
Expand Down
Loading

0 comments on commit 3356f4b

Please sign in to comment.