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

This is a reland of dacce720

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: I7086aa3f1cd12615e6f12bbd061084ecd325eb11
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3021180Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75707}

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(false);
+      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;