Revert "[wasm] Fix fallback from PKU to mprotect"

This reverts commit dacce720.

Reason for revert: Needs a fix.

Original change's description:
> [wasm] Fix fallback from PKU to mprotect
>
> The {WasmCodeManager::SetThreadWritable} method would return true if
> called in a nested scope, even if PKU is not available. The caller
> cannot tell then whether permission switching happened or not.
>
> This CL refactors the code to do an explicit check for PKU support, and
> removes the boolean return value from {SetThreadWritable}.
>
> R=​jkummerow@chromium.org
>
> Bug: v8:11959, v8:11974
> Change-Id: I2d45f1fa240305c6f92f63cdf190131d637bfe95
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3021383
> Commit-Queue: Clemens Backes <clemensb@chromium.org>
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#75699}

Bug: v8:11959, v8:11974
Change-Id: I199cf6dd6e12a209649fcf86f922e2500b50bbde
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3021179
Auto-Submit: Clemens Backes <clemensb@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#75700}

src/wasm/code-space-access.cc

@@ -39,14 +39,13 @@ CodeSpaceWriteScope::CodeSpaceWriteScope(NativeModule* native_module)
     : native_module_(native_module) {
   DCHECK_NOT_NULL(native_module_);
   if (FLAG_wasm_memory_protection_keys) {
-    auto* code_manager = GetWasmCodeManager();
-    if (code_manager->HasMemoryProtectionKeySupport()) {
-      code_manager->SetThreadWritable(true);
-      return;
+    bool success = GetWasmCodeManager()->SetThreadWritable(true);
+    if (!success && FLAG_wasm_write_protect_code_memory) {
+      // Fallback to mprotect-based write protection (much slower).
+      success = native_module_->SetWritable(true);
+      CHECK(success);
     }
-    // Fallback to mprotect-based write protection, if enabled.
-  }
-  if (FLAG_wasm_write_protect_code_memory) {
+  } else if (FLAG_wasm_write_protect_code_memory) {
     bool success = native_module_->SetWritable(true);
     CHECK(success);
   }
@@ -54,14 +53,13 @@ CodeSpaceWriteScope::CodeSpaceWriteScope(NativeModule* native_module)
 
 CodeSpaceWriteScope::~CodeSpaceWriteScope() {
   if (FLAG_wasm_memory_protection_keys) {
-    auto* code_manager = GetWasmCodeManager();
-    if (code_manager->HasMemoryProtectionKeySupport()) {
-      code_manager->SetThreadWritable(true);
-      return;
+    bool success = GetWasmCodeManager()->SetThreadWritable(false);
+    if (!success && FLAG_wasm_write_protect_code_memory) {
+      // Fallback to mprotect-based write protection (much slower).
+      success = native_module_->SetWritable(false);
+      CHECK(success);
     }
-    // Fallback to mprotect-based write protection, if enabled.
-  }
-  if (FLAG_wasm_write_protect_code_memory) {
+  } else if (FLAG_wasm_write_protect_code_memory) {
     bool success = native_module_->SetWritable(false);
     CHECK(success);
   }

src/wasm/memory-protection-key.cc

@@ -164,9 +164,9 @@ bool SetPermissionsAndMemoryProtectionKey(
 }
 
 DISABLE_CFI_ICALL
-void SetPermissionsForMemoryProtectionKey(
+bool SetPermissionsForMemoryProtectionKey(
     int key, MemoryProtectionKeyPermission permissions) {
-  CHECK_NE(kNoMemoryProtectionKey, key);
+  if (key == kNoMemoryProtectionKey) return false;
 
 #if defined(V8_OS_LINUX) && defined(V8_HOST_ARCH_X64)
   typedef int (*pkey_set_t)(int, unsigned int);
@@ -175,10 +175,11 @@ void SetPermissionsForMemoryProtectionKey(
   DCHECK_NOT_NULL(pkey_set);
 
   int ret = pkey_set(key, permissions);
-  CHECK_EQ(0 /* success */, ret);
+
+  return ret == /* success */ 0;
 #else
-  // On platforms without PKU support, we should have failed the CHECK above
-  // because the key must be {kNoMemoryProtectionKey}.
+  // On platforms without PKU support, we should have already returned because
+  // the key must be {kNoMemoryProtectionKey}.
   UNREACHABLE();
 #endif
 }

src/wasm/memory-protection-key.h

@@ -77,9 +77,10 @@ bool SetPermissionsAndMemoryProtectionKey(
     PageAllocator* page_allocator, base::AddressRegion region,
     PageAllocator::Permission page_permissions, int key);
 
-// Set the key's permissions. {key} must be valid, i.e. not
-// {kNoMemoryProtectionKey}.
-void SetPermissionsForMemoryProtectionKey(
+// Set the key's permissions and return whether this was successful.
+// Returns false on platforms without PKU support or when the operation failed,
+// e.g., because the key was invalid.
+bool SetPermissionsForMemoryProtectionKey(
     int key, MemoryProtectionKeyPermission permissions);
 
 }  // namespace wasm

src/wasm/wasm-code-manager.cc

@@ -1954,14 +1954,13 @@ size_t WasmCodeManager::EstimateNativeModuleMetaDataSize(
   return wasm_module_estimate + native_module_estimate;
 }
 
-void WasmCodeManager::SetThreadWritable(bool writable) {
-  DCHECK(HasMemoryProtectionKeySupport());
+bool WasmCodeManager::SetThreadWritable(bool writable) {
   static thread_local int writable_nesting_level = 0;
   if (writable) {
-    if (++writable_nesting_level > 1) return;
+    if (++writable_nesting_level > 1) return true;
   } else {
     DCHECK_GT(writable_nesting_level, 0);
-    if (--writable_nesting_level > 0) return;
+    if (--writable_nesting_level > 0) return true;
   }
   writable = writable_nesting_level > 0;
 
@@ -1970,11 +1969,8 @@ void WasmCodeManager::SetThreadWritable(bool writable) {
 
   TRACE_HEAP("Setting memory protection key %d to writable: %d.\n",
              memory_protection_key_, writable);
-  SetPermissionsForMemoryProtectionKey(memory_protection_key_, permissions);
-}
-
-bool WasmCodeManager::HasMemoryProtectionKeySupport() const {
-  return memory_protection_key_ != kNoMemoryProtectionKey;
+  return SetPermissionsForMemoryProtectionKey(memory_protection_key_,
+                                              permissions);
 }
 
 std::shared_ptr<NativeModule> WasmCodeManager::NewNativeModule(

src/wasm/wasm-code-manager.h

@@ -956,14 +956,10 @@ class V8_EXPORT_PRIVATE WasmCodeManager final {
   static size_t EstimateNativeModuleMetaDataSize(const WasmModule* module);
 
   // Set this thread's permission of all owned code space to read-write or
-  // read-only (if {writable} is false). Can only be called if
-  // {HasMemoryProtectionKeySupport()} is {true}.
-  // Since the permission is thread-local, there is no requirement to hold any
-  // lock when calling this method.
-  void SetThreadWritable(bool writable);
-
-  // Returns true if there is PKU support, false otherwise.
-  bool HasMemoryProtectionKeySupport() const;
+  // read-only (if {writable} is false). Uses memory protection keys.
+  // Returns true on success. Since the permission is thread-local, there is no
+  // requirement to hold any lock when calling this method.
+  bool SetThreadWritable(bool writable);
 
  private:
   friend class WasmCodeAllocator;