Commit 5ada213c authored by Adam Klein's avatar Adam Klein Committed by V8 LUCI CQ

Revert "[wasm] Reset PKRU before spawning new threads"

This reverts commit 8218c061.

Reason for revert: compile failures, e.g.:
https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Mac%20-%20arm64%20-%20release%20builder/11040/overview

Original change's description:
> [wasm] Reset PKRU before spawning new threads
>
> We sometimes hit the DCHECK in the wasm code manager:
>   DCHECK_IMPLIES(writable, !MemoryProtectionKeyWritable());
>
> This is because we spawn new threads while having a
> {CodeSpaceWriteScope} open. In the case of PKU, this changes the PKRU
> register to allow writes to the code space, and the value of that
> register is inherited by any new thread. If this thread then tries to
> switch to writable code spaces, it hits the DCHECK. It would hit a
> similar DCHECK when trying to execute code.
>
> We fix this issue by temporarily resetting the PKRU register to
> non-writable while we call the {NotifyConcurrencyIncrease} method. This
> is not a very robust solution, as any new call that potentially happens
> inside a {CodeSpaceWriteScope} needs to do the same, but refactoring the
> code to avoid spawning new threads while being in writable state would
> be a lot of work with other downsides.
>
> R=​jkummerow@chromium.org
>
> Bug: v8:13075
> Change-Id: Ibc7270aa597902dc6d9649cb6bcdfce8b1a9bafc
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3762579
> Commit-Queue: Clemens Backes <clemensb@chromium.org>
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#81729}

Bug: v8:13075
Change-Id: I235e7263856a37cf0f4aa1c27493aac8e6db7910
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3763587
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81730}
parent 8218c061
...@@ -96,17 +96,6 @@ bool CodeSpaceWriteScope::SwitchingPerNativeModule() { ...@@ -96,17 +96,6 @@ bool CodeSpaceWriteScope::SwitchingPerNativeModule() {
FLAG_wasm_write_protect_code_memory; FLAG_wasm_write_protect_code_memory;
} }
ResetPKUPermissionsForThreadSpawning::ResetPKUPermissionsForThreadSpawning() {
auto* code_manager = GetWasmCodeManager();
was_writable_ = code_manager->MemoryProtectionKeysEnabled() &&
code_manager->MemoryProtectionKeyWritable();
if (was_writable_) code_manager->SetThreadWritable(false);
}
ResetPKUPermissionsForThreadSpawning::~ResetPKUPermissionsForThreadSpawning() {
if (was_writable_) GetWasmCodeManager()->SetThreadWritable(true);
}
#endif // !V8_HAS_PTHREAD_JIT_WRITE_PROTECT #endif // !V8_HAS_PTHREAD_JIT_WRITE_PROTECT
} // namespace wasm } // namespace wasm
......
...@@ -12,7 +12,10 @@ ...@@ -12,7 +12,10 @@
#include "src/base/build_config.h" #include "src/base/build_config.h"
#include "src/base/macros.h" #include "src/base/macros.h"
namespace v8::internal::wasm { namespace v8 {
namespace internal {
namespace wasm {
class NativeModule; class NativeModule;
...@@ -72,24 +75,8 @@ class V8_NODISCARD CodeSpaceWriteScope final { ...@@ -72,24 +75,8 @@ class V8_NODISCARD CodeSpaceWriteScope final {
NativeModule* const previous_native_module_; NativeModule* const previous_native_module_;
}; };
// Sometimes we need to call a function which will / might spawn a new thread, } // namespace wasm
// like {JobHandle::NotifyConcurrencyIncrease}, while holding a } // namespace internal
// {CodeSpaceWriteScope}. This is problematic since the new thread will inherit } // namespace v8
// the parent thread's PKU permissions.
// The {ResetPKUPermissionsForThreadSpawning} scope will thus reset the PKU
// permissions as long as it is in scope, such that it is safe to spawn new
// threads.
class V8_NODISCARD ResetPKUPermissionsForThreadSpawning {
#if !V8_HAS_PTHREAD_JIT_WRITE_PROTECT
public:
ResetPKUPermissionsForThreadSpawning();
~ResetPKUPermissionsForThreadSpawning();
private:
bool was_writable_;
#endif
};
} // namespace v8::internal::wasm
#endif // V8_WASM_CODE_SPACE_ACCESS_H_ #endif // V8_WASM_CODE_SPACE_ACCESS_H_
...@@ -3404,7 +3404,6 @@ void CompilationStateImpl::CommitCompilationUnits( ...@@ -3404,7 +3404,6 @@ void CompilationStateImpl::CommitCompilationUnits(
compilation_unit_queues_.AddUnits(baseline_units, top_tier_units, compilation_unit_queues_.AddUnits(baseline_units, top_tier_units,
native_module_->module()); native_module_->module());
} }
ResetPKUPermissionsForThreadSpawning pku_reset_scope;
compile_job_->NotifyConcurrencyIncrease(); compile_job_->NotifyConcurrencyIncrease();
} }
...@@ -3416,10 +3415,6 @@ void CompilationStateImpl::CommitTopTierCompilationUnit( ...@@ -3416,10 +3415,6 @@ void CompilationStateImpl::CommitTopTierCompilationUnit(
void CompilationStateImpl::AddTopTierPriorityCompilationUnit( void CompilationStateImpl::AddTopTierPriorityCompilationUnit(
WasmCompilationUnit unit, size_t priority) { WasmCompilationUnit unit, size_t priority) {
compilation_unit_queues_.AddTopTierPriorityUnit(unit, priority); compilation_unit_queues_.AddTopTierPriorityUnit(unit, priority);
// We should not have a {CodeSpaceWriteScope} open at this point, as
// {NotifyConcurrencyIncrease} can spawn new threads which could inherit PKU
// permissions (which would be a security issue).
DCHECK(!CodeSpaceWriteScope::IsInScope());
compile_job_->NotifyConcurrencyIncrease(); compile_job_->NotifyConcurrencyIncrease();
} }
......
...@@ -596,7 +596,6 @@ class DeserializeCodeTask : public JobTask { ...@@ -596,7 +596,6 @@ class DeserializeCodeTask : public JobTask {
deserializer_->CopyAndRelocate(unit); deserializer_->CopyAndRelocate(unit);
} }
publish_queue_.Add(std::move(batch)); publish_queue_.Add(std::move(batch));
ResetPKUPermissionsForThreadSpawning pku_reset_scope;
delegate->NotifyConcurrencyIncrease(); delegate->NotifyConcurrencyIncrease();
} }
} }
...@@ -679,7 +678,6 @@ bool NativeModuleDeserializer::Read(Reader* reader) { ...@@ -679,7 +678,6 @@ bool NativeModuleDeserializer::Read(Reader* reader) {
reloc_queue.Add(std::move(batch)); reloc_queue.Add(std::move(batch));
DCHECK(batch.empty()); DCHECK(batch.empty());
batch_size = 0; batch_size = 0;
ResetPKUPermissionsForThreadSpawning pku_reset_scope;
job_handle->NotifyConcurrencyIncrease(); job_handle->NotifyConcurrencyIncrease();
} }
} }
...@@ -691,7 +689,6 @@ bool NativeModuleDeserializer::Read(Reader* reader) { ...@@ -691,7 +689,6 @@ bool NativeModuleDeserializer::Read(Reader* reader) {
if (!batch.empty()) { if (!batch.empty()) {
reloc_queue.Add(std::move(batch)); reloc_queue.Add(std::move(batch));
ResetPKUPermissionsForThreadSpawning pku_reset_scope;
job_handle->NotifyConcurrencyIncrease(); job_handle->NotifyConcurrencyIncrease();
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment