[PATCH] D129089: [BOLT] Fix concurrency bugs in the instrumentation runtime

[Michał Chojnowski via Phabricator via llvm-commits]

michoecho created this revision.
michoecho added reviewers: maksfb, rafaelauler.
michoecho added projects: LLVM, All.
Herald added a reviewer: rafauler.
Herald added a subscriber: ayermolo.
Herald added a reviewer: Amir.
michoecho requested review of this revision.
Herald added subscribers: llvm-commits, yota9.

The Mutex implementation used by the BOLT instrumentation runtime is
broken. `acquire()` performs an `xchg` of the `=r` `Result` with the
spinlock's `InUse` flag, which results in register garbage being written
to the flag instead of the intended `true`. If the register garbage
happens to be 0, the lock is "acquired" without setting the flag. This
has been observed to happen in practice, leading to concurrent hash table
modification and memory corruption.

This fixed by using `+r` instead of `=r`.

`__bolt_instr_data_dump()` does not lock the hash tables when iterating
over them, so the iteration can happen concurrently with a modification
done in another thread, when the table is in an inconsistent state. This
also has been observed in practice, when it caused a segmentation fault.

We fix this by locking hash tables during iteration. This is done by taking
the lock in `forEachElement()`.
The only other site of iteration, `resetCounters()`, has been correctly
locking the table even before this patch. This patch removes its `Lock`
because the lock is now taken in the inner `forEachElement()`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129089

Files:
  bolt/runtime/common.h
  bolt/runtime/instr.cpp


Index: bolt/runtime/instr.cpp
===================================================================
--- bolt/runtime/instr.cpp
+++ bolt/runtime/instr.cpp
@@ -289,6 +289,7 @@
   /// Traverses all elements in the table
   template <typename... Args>
   void forEachElement(void (*Callback)(MapEntry &, Args...), Args... args) {
+    Lock L(M);
     if (!TableRoot)
       return;
     return forEachElement(Callback, InitialSize, TableRoot, args...);
@@ -378,7 +379,6 @@
 
 template <typename T, uint32_t X, uint32_t Y>
 void SimpleHashTable<T, X, Y>::resetCounters() {
-  Lock L(M);
   forEachElement(resetIndCallCounter);
 }
 
Index: bolt/runtime/common.h
===================================================================
--- bolt/runtime/common.h
+++ bolt/runtime/common.h
@@ -475,7 +475,7 @@
 public:
   bool acquire() {
     bool Result = true;
-    asm volatile("lock; xchg %0, %1" : "+m"(InUse), "=r"(Result) : : "cc");
+    asm volatile("lock; xchg %0, %1" : "+m"(InUse), "+r"(Result) : : "cc");
     return !Result;
   }
   void release() { InUse = false; }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D129089.442113.patch
Type: text/x-patch
Size: 1065 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220704/8be291a6/attachment.bin>