[BOLT] Fix thread-safety of PointerAuthCFIAnalyzer

[bgergely0]

The pass calls setIgnored() on functions in parallel, but setIgnored is
not thread safe. The patch adds a mutex to guard setIgnored calls.

Fixes: #165362

[bgergely0]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• [BOLT] Fix thread-safety of PointerAuthCFIAnalyzer #165365 👈 (View in Graphite)
• [BOLT][PAC] Warn about synchronous unwind tables #165227
• [BOLT][NFC] Rename Pointer Auth DWARF rewriter passes #164622
• [BOLT] Improve InsertNegateRAStatePass::inferUnknownStates #163381
• [BOLT] Simplify RAState helpers (NFCI) #162820
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Gergely Bálint (bgergely0)

Changes

The pass calls setIgnored() on functions in parallel, but setIgnored is
not thread safe. The patch adds a mutex to guard setIgnored calls.

Fixes: #165362

---

Full diff: https://github.com/llvm/llvm-project/pull/165365.diff

2 Files Affected:

• (modified) bolt/include/bolt/Passes/PointerAuthCFIAnalyzer.h (+5)
• (modified) bolt/lib/Passes/PointerAuthCFIAnalyzer.cpp (+3)

diff --git a/bolt/include/bolt/Passes/PointerAuthCFIAnalyzer.h b/bolt/include/bolt/Passes/PointerAuthCFIAnalyzer.h
index e63de077fad18..54da398f0b2b3 100644
--- a/bolt/include/bolt/Passes/PointerAuthCFIAnalyzer.h
+++ b/bolt/include/bolt/Passes/PointerAuthCFIAnalyzer.h
@@ -13,11 +13,16 @@
 #define BOLT_PASSES_POINTER_AUTH_CFI_ANALYZER
 
 #include "bolt/Passes/BinaryPasses.h"
+#include <mutex>
 
 namespace llvm {
 namespace bolt {
 
 class PointerAuthCFIAnalyzer : public BinaryFunctionPass {
+  // setIgnored() is not thread-safe, but the pass is running on functions in
+  // parallel.
+  std::mutex IgnoreMutex;
+
 public:
   explicit PointerAuthCFIAnalyzer() : BinaryFunctionPass(false) {}
 
diff --git a/bolt/lib/Passes/PointerAuthCFIAnalyzer.cpp b/bolt/lib/Passes/PointerAuthCFIAnalyzer.cpp
index 17486536202b8..68913a4785af6 100644
--- a/bolt/lib/Passes/PointerAuthCFIAnalyzer.cpp
+++ b/bolt/lib/Passes/PointerAuthCFIAnalyzer.cpp
@@ -47,6 +47,7 @@ bool PointerAuthCFIAnalyzer::runOnFunction(BinaryFunction &BF) {
         // Not all functions have .cfi_negate_ra_state in them. But if one does,
         // we expect psign/pauth instructions to have the hasNegateRAState
         // annotation.
+        std::lock_guard<std::mutex> Lock(IgnoreMutex);
         BF.setIgnored();
         if (opts::Verbosity >= 1)
           BC.outs() << "BOLT-INFO: inconsistent RAStates in function "
@@ -73,6 +74,7 @@ bool PointerAuthCFIAnalyzer::runOnFunction(BinaryFunction &BF) {
             BC.outs() << "BOLT-INFO: inconsistent RAStates in function "
                       << BF.getPrintName()
                       << ": ptr signing inst encountered in Signed RA state\n";
+          std::lock_guard<std::mutex> Lock(IgnoreMutex);
           BF.setIgnored();
           return false;
         }
@@ -84,6 +86,7 @@ bool PointerAuthCFIAnalyzer::runOnFunction(BinaryFunction &BF) {
                       << BF.getPrintName()
                       << ": ptr authenticating inst encountered in Unsigned RA "
                          "state\n";
+          std::lock_guard<std::mutex> Lock(IgnoreMutex);
           BF.setIgnored();
           return false;
         }

[bgergely0]

There were two options to implement this fix:

1. the one implemented in this PR: guarding the setIgnored calls with a Mutex.

2. add the functions to ignore to a vector (guarded by a mutex), and do all setIgnored calls on a single thread after the parallel part is done.

To make sure option 1 is correct, we have to look for what members of BC are set by setIgnored, and what members are read in the rest of the parallel part of the pass.

Writes to BC from the setIgnored call

• BC.UndefinedSymbols
• BC.SymbolicDisAsm
• BC.SymbolToFunctionMap modified from ScanExternalRefs

What runOnFunction reads from the BC

• the rest of the parallel function only accesses the AArch64MCPlusBuilder through BC.MIB, to get bool information about individual instructions, (e.g. BC.MIB->isPSignOnLR(Inst))

---

For this reason I believe it's sufficient to only guard the setIgnored calls without creating a vector to hold to-be-ignored functions, and ignore them later on a single thread.

[bgergely0]

thanks for the approval @peterwaller-arm ! I opened #165368, which is this + your nit against main, so it can land faster. PTAL