[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}

src/wasm/code-space-access.cc

@@ -39,13 +39,14 @@ CodeSpaceWriteScope::CodeSpaceWriteScope(NativeModule* native_module)
     : native_module_(native_module) {
   DCHECK_NOT_NULL(native_module_);
   if (FLAG_wasm_memory_protection_keys) {
-    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);
+    auto* code_manager = GetWasmCodeManager();
+    if (code_manager->HasMemoryProtectionKeySupport()) {
+      code_manager->SetThreadWritable(true);
+      return;
     }
-  } else if (FLAG_wasm_write_protect_code_memory) {
+    // Fallback to mprotect-based write protection, if enabled.
+  }
+  if (FLAG_wasm_write_protect_code_memory) {
     bool success = native_module_->SetWritable(true);
     CHECK(success);
   }
@@ -53,13 +54,14 @@ CodeSpaceWriteScope::CodeSpaceWriteScope(NativeModule* native_module)
 
 CodeSpaceWriteScope::~CodeSpaceWriteScope() {
   if (FLAG_wasm_memory_protection_keys) {
-    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);
+    auto* code_manager = GetWasmCodeManager();
+    if (code_manager->HasMemoryProtectionKeySupport()) {
+      code_manager->SetThreadWritable(true);
+      return;
     }
-  } else if (FLAG_wasm_write_protect_code_memory) {
+    // Fallback to mprotect-based write protection, if enabled.
+  }
+  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
-bool SetPermissionsForMemoryProtectionKey(
+void SetPermissionsForMemoryProtectionKey(
     int key, MemoryProtectionKeyPermission permissions) {
-  if (key == kNoMemoryProtectionKey) return false;
+  CHECK_NE(kNoMemoryProtectionKey, key);
 
 #if defined(V8_OS_LINUX) && defined(V8_HOST_ARCH_X64)
   typedef int (*pkey_set_t)(int, unsigned int);
@@ -175,11 +175,10 @@ bool SetPermissionsForMemoryProtectionKey(
   DCHECK_NOT_NULL(pkey_set);
 
   int ret = pkey_set(key, permissions);
-
-  return ret == /* success */ 0;
+  CHECK_EQ(0 /* success */, ret);
 #else
-  // On platforms without PKU support, we should have already returned because
-  // the key must be {kNoMemoryProtectionKey}.
+  // On platforms without PKU support, we should have failed the CHECK above
+  // because the key must be {kNoMemoryProtectionKey}.
   UNREACHABLE();
 #endif
 }

src/wasm/memory-protection-key.h

@@ -77,10 +77,9 @@ bool SetPermissionsAndMemoryProtectionKey(
     PageAllocator* page_allocator, base::AddressRegion region,
     PageAllocator::Permission page_permissions, int key);
 
-// 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(
+// Set the key's permissions. {key} must be valid, i.e. not
+// {kNoMemoryProtectionKey}.
+void SetPermissionsForMemoryProtectionKey(
     int key, MemoryProtectionKeyPermission permissions);
 
 }  // namespace wasm

src/wasm/wasm-code-manager.cc

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

src/wasm/wasm-code-manager.h

@@ -956,10 +956,14 @@ 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). 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);
+  // 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;
 
  private:
   friend class WasmCodeAllocator;