From bfd27a1e43cbcf7155638539f1fd7a309eb76df1 Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Tue, 20 Feb 2024 11:31:39 -0800 Subject: [PATCH] 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 --- .../platform/systrap/sysmsg/sysmsg_lib.c | 15 +++++---- test/syscalls/linux/BUILD | 1 + test/syscalls/linux/processes.cc | 33 +++++++++++++++++-- 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/pkg/sentry/platform/systrap/sysmsg/sysmsg_lib.c b/pkg/sentry/platform/systrap/sysmsg/sysmsg_lib.c index 204b642fd..efd5429bc 100644 --- a/pkg/sentry/platform/systrap/sysmsg/sysmsg_lib.c +++ b/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); } diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD index ffbf86afc..a333c3ae9 100644 --- a/test/syscalls/linux/BUILD +++ b/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", diff --git a/test/syscalls/linux/processes.cc b/test/syscalls/linux/processes.cc index c4473cf3a..a2d755c26 100644 --- a/test/syscalls/linux/processes.cc +++ b/test/syscalls/linux/processes.cc @@ -19,7 +19,9 @@ #include #include +#include +#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 threads[kThreadCount]; + int pipe_fds[2]; + ASSERT_THAT(pipe(pipe_fds), SyscallSucceeds()); + + for (int i = 0; i < kThreadCount; i++) { + threads[i] = std::make_unique([&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.