systrap: track the spinning queue length in a separate counter

The current implementation has a race condition resulting in the skipping of
one element in the queue array. When retrieving objects from the queue, the
stub code can get stuck in an infinite loop due to unexpected unused elements.

Updates #10000

PiperOrigin-RevId: 608679165

pkg/sentry/platform/systrap/sysmsg/sysmsg_lib.c

@@ -108,7 +108,7 @@ void memcpy(uint8_t *dest, uint8_t *src, size_t n) {
 //
 // This queue is lock-less to be sure that any thread scheduled out
 // from CPU doesn't block others.
-#define SPINNING_QUEUE_SIZE 128
+#define SPINNING_QUEUE_SIZE 384
 
 // MAX_SPINNING_THREADS is half of SPINNING_QUEUE_SIZE to be sure that the tail
 // doesn't catch the head. More details are in spinning_queue_remove_first.
@@ -120,6 +120,7 @@ void memcpy(uint8_t *dest, uint8_t *src, size_t n) {
 #define MAX_RE_ENQUEUE 2
 
 struct spinning_queue {
+  uint32_t len;
   uint32_t start;
   uint32_t end;
   uint64_t start_times[SPINNING_QUEUE_SIZE];
@@ -134,19 +135,19 @@ static bool spinning_queue_push(uint8_t re_enqueue_times)
     __attribute__((warn_unused_result));
 static bool spinning_queue_push(uint8_t re_enqueue_times) {
   struct spinning_queue *queue = __export_spinning_queue_addr;
-  uint32_t idx, start, end;
+  uint32_t idx, end, len;
 
   BUILD_BUG_ON(sizeof(struct spinning_queue) > SPINNING_QUEUE_MEM_SIZE);
   if (re_enqueue_times >= MAX_RE_ENQUEUE) {
     return false;
   }
 
-  end = atomic_add(&queue->end, 1);
-  start = atomic_load(&queue->start);
-  if (end - start > MAX_SPINNING_THREADS) {
-    atomic_sub(&queue->end, 1);
+  len = atomic_add(&queue->len, 1);
+  if (len > MAX_SPINNING_THREADS) {
+    atomic_sub(&queue->len, 1);
     return false;
   }
+  end = atomic_add(&queue->end, 1);
 
   idx = end - 1;
   atomic_store(&queue->num_times_re_enqueued[idx % SPINNING_QUEUE_SIZE],
@@ -162,6 +163,7 @@ static void spinning_queue_pop() {
   struct spinning_queue *queue = __export_spinning_queue_addr;
 
   atomic_sub(&queue->end, 1);
+  atomic_sub(&queue->len, 1);
 }
 
 // spinning_queue_remove_first removes one thread from a queue that has been
@@ -195,6 +197,7 @@ static bool spinning_queue_remove_first(uint64_t timeout) {
     }
   }
 
+  atomic_sub(&queue->len, 1);
   if (timeout == 0) return true;
   return !spinning_queue_push(re_enqueue + 1);
 }

test/syscalls/linux/BUILD

@@ -4314,6 +4314,7 @@ cc_binary(
     deps = select_gtest() + [
         "//test/util:capability_util",
         "//test/util:multiprocess_util",
+        "//test/util:save_util",
         "//test/util:test_main",
         "//test/util:test_util",
         "//test/util:thread_util",

test/syscalls/linux/processes.cc

@@ -19,7 +19,9 @@
 
 #include <algorithm>
 #include <cstring>
+#include <memory>
 
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "absl/flags/flag.h"
 #include "absl/strings/str_format.h"
@@ -28,6 +30,7 @@
 #include "absl/time/time.h"
 #include "test/util/capability_util.h"
 #include "test/util/multiprocess_util.h"
+#include "test/util/save_util.h"
 #include "test/util/test_util.h"
 #include "test/util/thread_util.h"
 
@@ -142,6 +145,29 @@ TEST(Processes, TheadSharesSamePID) {
   EXPECT_EQ(test_pid, pid_from_child);
 }
 
+TEST(Processes, ThousandsOfThreads) {
+  const DisableSave ds;  // Too many syscalls.
+  const int kThreadCount = 1000;
+  const int kSyscallsPerThread = 1000;
+  std::unique_ptr<ScopedThread> threads[kThreadCount];
+  int pipe_fds[2];
+  ASSERT_THAT(pipe(pipe_fds), SyscallSucceeds());
+
+  for (int i = 0; i < kThreadCount; i++) {
+    threads[i] = std::make_unique<ScopedThread>([&pipe_fds]() {
+      char c;
+      EXPECT_THAT(read(pipe_fds[0], &c, 1), SyscallSucceedsWithValue(0));
+      for (int j = 0; j < kSyscallsPerThread; j++) {
+        syscall(SYS_close, -1);
+      }
+    });
+  }
+  close(pipe_fds[1]);
+  for (int i = 0; i < kThreadCount; i++) {
+    threads[i]->Join();
+  }
+}
+
 // ExecSwapResult is used to carry PIDs and TIDs in ExecSwapThreadGroupLeader.
 struct ExecSwapResult {
   int pipe_fd;  // FD to write result data to.
@@ -181,8 +207,8 @@ struct ExecSwapArg {
 };
 
 // ExecSwapPostExec is the third part of the ExecSwapThreadGroupLeader test.
-// It is called after the test has fork()'d, clone()'d, and exec()'d into this.
-// It writes all the PIDs and TIDs from each part of the test to a pipe.
+// It is called after the test has fork()'d, clone()'d, and exec()'d into
+// this. It writes all the PIDs and TIDs from each part of the test to a pipe.
 int ExecSwapPostExec() {
   std::cerr << "Test exec'd." << std::endl;
   pid_t pid;
@@ -331,7 +357,8 @@ TEST(Processes, ExecSwapThreadGroupLeader) {
   EXPECT_NE(test_pid, fork_pid);
   EXPECT_EQ(fork_pid, result.pre_clone_pid);  // Sanity check.
 
-  // Before cloning, PID == TID, the child thread is leader of its thread group.
+  // Before cloning, PID == TID, the child thread is leader of its thread
+  // group.
   EXPECT_EQ(result.pre_clone_pid, result.pre_clone_tid);
 
   // PID should not change with clone.