Use FlushInstructionCache to protect stub reads instead of using a barrier

Author: davidwrighton

src/coreclr/clrdefinitions.cmake

@@ -207,6 +207,10 @@ if (FEATURE_STUBPRECODE_DYNAMIC_HELPERS)
   add_definitions(-DFEATURE_STUBPRECODE_DYNAMIC_HELPERS)
 endif()
 
+if (FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS)
+  add_definitions(-DFEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS)
+endif()
+
 if (CLR_CMAKE_TARGET_APPLE)
 #  Re-enable when dbgshim containing https://github.com/dotnet/diagnostics/pull/5487 is generally available
 #  add_definitions(-DFEATURE_MAP_THUNKS_FROM_IMAGE)

src/coreclr/clrfeatures.cmake

@@ -84,6 +84,12 @@ else()
   set(FEATURE_CORECLR_VIRTUAL_STUB_DISPATCH 1)
 endif()
 
+# We use a flush instruction cache to protect reads from the StubPrecodeData/CallCountingStub structures in the stubs.
+# This is needed because the StubPrecodeData structure is initialized after the stub code is written, and we need to ensure that
+# the reads in the stub happen after the writes to the StubPrecodeData structure. We could do this with a barrier instruction in the stub,
+# but that would be more expensive.
+set(FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS 1)
+
 if (CLR_CMAKE_HOST_UNIX AND CLR_CMAKE_HOST_ARCH_AMD64)
   # Allow 16 byte compare-exchange (cmpxchg16b)
   add_compile_options($<${FEATURE_CORECLR_CACHED_INTERFACE_DISPATCH}:-mcx16>)

src/coreclr/vm/arm64/thunktemplates.S

@@ -30,11 +30,11 @@
     IN_PAGE_INDEX = 0
     .rept STUB_PRECODE_NUM_THUNKS_PER_MAPPING
 
-    dmb ishld
     ldr x10, DATA_SLOT(StubPrecode, Target, STUB_PRECODE_CODESIZE, StubPrecodeCodeTemplate)
     ldr x12, DATA_SLOT(StubPrecode, SecretParam, STUB_PRECODE_CODESIZE, StubPrecodeCodeTemplate)
     br x10
 
+    brk     0xf000      // Stubs need to be 24-byte in size to allow for the data to be 2 pointers + 1 byte
     brk     0xf000      // Stubs need to be 24-byte in size to allow for the data to be 2 pointers + 1 byte
     brk     0xf000      // Stubs need to be 24-byte in size to allow for the data to be 2 pointers + 1 byte
 
@@ -52,7 +52,7 @@ LEAF_END_MARKED StubPrecodeCodeTemplate, _TEXT
 // FixupPrecode
 // ----------
 
-#define FIXUP_PRECODE_CODESIZE 0x18 // 5 instructions, 4 bytes each (and we also have 4 bytes of padding)
+#define FIXUP_PRECODE_CODESIZE 0x18 // 6 instructions, 4 bytes each
 #define FIXUP_PRECODE_DATASIZE 0x18 // 3 qwords
 .set FIXUP_PRECODE_NUM_THUNKS_PER_MAPPING,(THUNKS_MAP_SIZE / FIXUP_PRECODE_CODESIZE)
 
@@ -89,7 +89,6 @@ LEAF_END_MARKED FixupPrecodeCodeTemplate, _TEXT
     IN_PAGE_INDEX = 0
     .rept CALLCOUNTING_NUM_THUNKS_PER_MAPPING
 
-        dmb ishld
         ldr  x9, DATA_SLOT(CallCountingStub, RemainingCallCountCell, CALLCOUNTING_CODESIZE, CallCountingStubCodeTemplate)
         ldrh w10, [x9]
         subs w10, w10, #1
@@ -100,6 +99,7 @@ LEAF_END_MARKED FixupPrecodeCodeTemplate, _TEXT
 0:
         ldr  x10, DATA_SLOT(CallCountingStub, TargetForThresholdReached, CALLCOUNTING_CODESIZE, CallCountingStubCodeTemplate)
         br   x10
+        brk     0xf000      // Stubs need to be aligned
 
     IN_PAGE_INDEX = IN_PAGE_INDEX + 1
     .endr
@@ -120,12 +120,12 @@ LEAF_END_MARKED CallCountingStubCodeTemplate, _TEXT
     .irp STUB_PAGE_SIZE, 16384, 32768, 65536
 
     LEAF_ENTRY StubPrecodeCode\STUB_PAGE_SIZE
-        dmb ishld
         ldr x10, DATA_SLOT(StubPrecode, Target)
         ldr x12, DATA_SLOT(StubPrecode, SecretParam)
         br x10
         brk     0xf000      // Stubs need to be 24-byte in size to allow for the data to be 3 pointers
         brk     0xf000      // Stubs need to be 24-byte in size to allow for the data to be 3 pointers
+        brk     0xf000      // Stubs need to be 24-byte in size to allow for the data to be 3 pointers
     LEAF_END_MARKED StubPrecodeCode\STUB_PAGE_SIZE
 
     LEAF_ENTRY FixupPrecodeCode\STUB_PAGE_SIZE
@@ -150,6 +150,7 @@ LOCAL_LABEL(StubStart\STUB_PAGE_SIZE):
 LOCAL_LABEL(CountReachedZero\STUB_PAGE_SIZE):
         ldr  x10, DATA_SLOT(CallCountingStub, TargetForThresholdReached)
         br   x10
+        brk     0xf000      // Stubs need to be aligned in total size
     LEAF_END_MARKED CallCountingStubCode\STUB_PAGE_SIZE
 
     .endr

src/coreclr/vm/arm64/thunktemplates.asm

@@ -11,7 +11,6 @@
 #define DATA_SLOT(stub, field) (stub##Code + STUB_PAGE_SIZE + stub##Data__##field)
 
     LEAF_ENTRY StubPrecodeCode
-        dmb ishld
         ldr x10, DATA_SLOT(StubPrecode, Target)
         ldr x12, DATA_SLOT(StubPrecode, SecretParam)
         br x10
@@ -26,7 +25,6 @@
     LEAF_END_MARKED FixupPrecodeCode
 
     LEAF_ENTRY CallCountingStubCode
-        dmb ishld
         ldr  x9, DATA_SLOT(CallCountingStub, RemainingCallCountCell)
         ldrh w10, [x9]
         subs w10, w10, #1

src/coreclr/vm/callcounting.cpp

@@ -263,6 +263,15 @@ const CallCountingStub *CallCountingManager::CallCountingStubAllocator::Allocate
     allocationAddressHolder.SuppressRelease();
     stub->Initialize(targetForMethod, remainingCallCountCell);
 
+#ifdef FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS
+    // While the allocation of a precode will use a memory mapping technique, and not actually write to the set of instructions,
+    // the set of instructions in the stub has non-barrier protected reads from the StubPrecodeData structure. In order to protect those
+    // reads we would either need barrier instructions in the stub, or we need to ensure that the precode is flushed in the instruction cache
+    // which will have the side effect of ensuring that the reads within the stub will happen *after* the writes to the StubPrecodeData structure which
+    // happened in the Init routine above.
+    ClrFlushInstructionCache(stub, CallCountingStub::CodeSize);
+#endif // FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS
+
     return stub;
 }
 

src/coreclr/vm/callcounting.h

@@ -98,7 +98,7 @@ class CallCountingStub
 #elif defined(TARGET_ARM)
     static const SIZE_T CodeSize = 32;
 #elif defined(TARGET_LOONGARCH64)
-    static const SIZE_T CodeSize = 48;
+    static const SIZE_T CodeSize = 40;
 #elif defined(TARGET_RISCV64)
     static const SIZE_T CodeSize = 40;
 #elif defined(TARGET_WASM)

src/coreclr/vm/dllimportcallback.cpp

@@ -289,6 +289,15 @@ void UMEntryThunkData::Terminate()
     // TheUMEntryPrestub includes diagnostic for collected delegates
     m_pUMEntryThunk->SetTargetUnconditional(TheUMThunkPreStub());
 
+#ifdef FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS
+    // While the allocation of a precode will use a memory mapping technique, and not actually write to the set of instructions,
+    // the set of instructions in the stub has non-barrier protected reads from the StubPrecodeData structure. In order to protect those
+    // reads we would either need barrier instructions in the stub, or we need to ensure that the precode is flushed in the instruction cache
+    // which will have the side effect of ensuring that the reads within the stub will happen *after* the writes to the StubPrecodeData structure which
+    // happened in the Init routine above.
+    ClrFlushInstructionCache(m_pUMEntryThunk, sizeof(m_pUMEntryThunk), true);
+#endif // FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS
+
     OBJECTHANDLE pObjectHandle = m_pObjectHandle;
 
     // Set m_pObjectHandle indicate the collected state

src/coreclr/vm/dllimportcallback.h

@@ -208,6 +208,15 @@ class UMEntryThunkData
 #ifdef _DEBUG
         m_state = kLoadTimeInited;
 #endif
+
+#ifdef FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS
+        // While the allocation of a precode will use a memory mapping technique, and not actually write to the set of instructions,
+        // the set of instructions in the stub has non-barrier protected reads from the StubPrecodeData structure. In order to protect those
+        // reads we would either need barrier instructions in the stub, or we need to ensure that the precode is flushed in the instruction cache
+        // which will have the side effect of ensuring that the reads within the stub will happen *after* the writes to the StubPrecodeData structure which
+        // happened in the Init routine above.
+        ClrFlushInstructionCache(m_pUMEntryThunk, sizeof(m_pUMEntryThunk));
+#endif // FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS
     }
 
     void Terminate();

src/coreclr/vm/loongarch64/thunktemplates.S

@@ -7,8 +7,7 @@
 // Note that the offsets specified in pcaddi must match the behavior of GetStubCodePageSize() on this architecture/os.
 
 LEAF_ENTRY StubPrecodeCode
-    dbar 0
-    pcaddi  $r21, 0xFFF
+    pcaddi  $r21, 0x1000
     ld.d  $t2,$r21, StubPrecodeData__SecretParam
     ld.d  $r21,$r21, StubPrecodeData__Target
     jirl  $r0,$r21,0
@@ -28,8 +27,7 @@ LEAF_END_MARKED FixupPrecodeCode
 // NOTE: For LoongArch64 `CallCountingStubData__RemainingCallCountCell` must be zero !!!
 // Because the stub-identifying token is $t1 within the `OnCallCountThresholdReachedStub`.
 LEAF_ENTRY CallCountingStubCode
-    dbar 0
-    pcaddi  $t2, 0xFFF
+    pcaddi  $t2, 0x1000
     ld.d  $t1, $t2, CallCountingStubData__RemainingCallCountCell
     ld.h  $r21, $t1, 0
     addi.w  $r21, $r21, -1

src/coreclr/vm/precode.cpp

@@ -229,6 +229,16 @@ InterpreterPrecode* Precode::AllocateInterpreterPrecode(PCODE byteCode,
 
     InterpreterPrecode* pPrecode = (InterpreterPrecode*)pamTracker->Track(pLoaderAllocator->GetNewStubPrecodeHeap()->AllocStub());
     pPrecode->Init(pPrecode, byteCode);
+
+#ifdef FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS
+    // While the allocation of a precode will use a memory mapping technique, and not actually write to the set of instructions,
+    // the set of instructions in the stub has non-barrier protected reads from the StubPrecodeData structure. In order to protect those
+    // reads we would either need barrier instructions in the stub, or we need to ensure that the precode is flushed in the instruction cache
+    // which will have the side effect of ensuring that the reads within the stub will happen *after* the writes to the StubPrecodeData structure which
+    // happened in the Init routine above.
+    ClrFlushInstructionCache(pPrecode->GetCode(), Precode::SizeOf(t));
+#endif // FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS
+
 #ifdef FEATURE_PERFMAP
     PerfMap::LogStubs(__FUNCTION__, "UMEntryThunk", (PCODE)pPrecode, sizeof(InterpreterPrecode), PerfMapStubType::IndividualWithinBlock);
 #endif
@@ -265,6 +275,16 @@ Precode* Precode::Allocate(PrecodeType t, MethodDesc* pMD,
         ThisPtrRetBufPrecodeData *pData = (ThisPtrRetBufPrecodeData*)pamTracker->Track(pLoaderAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(sizeof(ThisPtrRetBufPrecodeData))));
         pThisPtrRetBufPrecode->Init(pData, pMD, pLoaderAllocator);
         pPrecode = (Precode*)pThisPtrRetBufPrecode;
+
+#ifdef FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS
+        // While the allocation of a precode will use a memory mapping technique, and not actually write to the set of instructions,
+        // the set of instructions in the stub has non-barrier protected reads from the StubPrecodeData structure. In order to protect those
+        // reads we would either need barrier instructions in the stub, or we need to ensure that the precode is flushed in the instruction cache
+        // which will have the side effect of ensuring that the reads within the stub will happen *after* the writes to the StubPrecodeData structure which
+        // happened in the Init routine above.
+        ClrFlushInstructionCache(pPrecode->GetCode(), Precode::SizeOf(t));
+#endif // FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS
+
 #ifdef FEATURE_PERFMAP
         PerfMap::LogStubs(__FUNCTION__, "ThisPtrRetBuf", (PCODE)pPrecode, sizeof(ThisPtrRetBufPrecodeData), PerfMapStubType::IndividualWithinBlock);
 #endif
@@ -275,6 +295,16 @@ Precode* Precode::Allocate(PrecodeType t, MethodDesc* pMD,
         _ASSERTE(t == PRECODE_STUB || t == PRECODE_NDIRECT_IMPORT);
         pPrecode = (Precode*)pamTracker->Track(pLoaderAllocator->GetNewStubPrecodeHeap()->AllocStub());
         pPrecode->Init(pPrecode, t, pMD, pLoaderAllocator);
+
+#ifdef FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS
+        // While the allocation of a precode will use a memory mapping technique, and not actually write to the set of instructions,
+        // the set of instructions in the stub has non-barrier protected reads from the StubPrecodeData structure. In order to protect those
+        // reads we would either need barrier instructions in the stub, or we need to ensure that the precode is flushed in the instruction cache
+        // which will have the side effect of ensuring that the reads within the stub will happen *after* the writes to the StubPrecodeData structure which
+        // happened in the Init routine above.
+        ClrFlushInstructionCache(pPrecode->GetCode(), Precode::SizeOf(t));
+#endif // FEATURE_CORECLR_FLUSH_INSTRUCTION_CACHE_TO_PROTECT_STUB_READS
+
 #ifdef FEATURE_PERFMAP
         PerfMap::LogStubs(__FUNCTION__, t == PRECODE_STUB ? "StubPrecode" : "PInvokeImportPrecode", (PCODE)pPrecode, sizeof(StubPrecode), PerfMapStubType::IndividualWithinBlock);
 #endif