From d432952bbedc81a657a4e70d705eb8847481d4bd Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Tue, 12 Mar 2024 17:28:19 -0700 Subject: [PATCH] systrap: prevent corruptions of spinning sueues The current synchronization is based on an assumption that a queue buffer can't be recycled if the current thread itself is in the queue. Unfortunately, this assumption is incorrect, and thus the queue buffer can be corrupted. This change reworks the synchronization part so that the start index is updated only after committing changes in the queue buffer. Fixes #10000 PiperOrigin-RevId: 615226911 --- .../platform/systrap/sysmsg/sysmsg_lib.c | 35 +++++++++---------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/pkg/sentry/platform/systrap/sysmsg/sysmsg_lib.c b/pkg/sentry/platform/systrap/sysmsg/sysmsg_lib.c index efd5429bc..b84c5cbda 100644 --- a/pkg/sentry/platform/systrap/sysmsg/sysmsg_lib.c +++ b/pkg/sentry/platform/systrap/sysmsg/sysmsg_lib.c @@ -108,11 +108,10 @@ 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 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. -#define MAX_SPINNING_THREADS (SPINNING_QUEUE_SIZE / 2) +// +// The size of the queue must be a divisor of 2^32, because queue indexes are +// calculated as modules of uint32 values. +#define SPINNING_QUEUE_SIZE 256 // MAX_RE_ENQUEUE defines the amount of time a given entry in the spinning queue // needs to reach timeout in order to be removed. Re-enqueuing a timeout is done @@ -143,7 +142,7 @@ static bool spinning_queue_push(uint8_t re_enqueue_times) { } len = atomic_add(&queue->len, 1); - if (len > MAX_SPINNING_THREADS) { + if (len > SPINNING_QUEUE_SIZE) { atomic_sub(&queue->len, 1); return false; } @@ -177,22 +176,22 @@ static bool spinning_queue_remove_first(uint64_t timeout) static bool spinning_queue_remove_first(uint64_t timeout) { struct spinning_queue *queue = __export_spinning_queue_addr; uint64_t ts; - uint32_t idx; uint8_t re_enqueue = 0; while (1) { - idx = atomic_load(&queue->start); - ts = atomic_load(&queue->start_times[idx % SPINNING_QUEUE_SIZE]); - if (ts == 0 && timeout == 0) continue; - if (ts == 0 || rdtsc() - ts < timeout) return false; + uint32_t idx, qidx; - // The current thread is still in a queue and the length of the queue is - // twice of the maximum number of threads, so we can zero the element and be - // sure that nobody is trying to set it in a non-zero value. - atomic_store(&queue->start_times[idx % SPINNING_QUEUE_SIZE], 0); - re_enqueue = - atomic_load(&queue->num_times_re_enqueued[idx % SPINNING_QUEUE_SIZE]); - if (atomic_compare_exchange(&queue->start, &idx, idx + 1)) { + idx = atomic_load(&queue->start); + qidx = idx % SPINNING_QUEUE_SIZE; + ts = atomic_load(&queue->start_times[qidx]); + + if (ts == 0) continue; + if (rdtsc() - ts < timeout) return false; + if (idx != atomic_load(&queue->start)) continue; // Lose the race. + + re_enqueue = atomic_load(&queue->num_times_re_enqueued[qidx]); + if (atomic_compare_exchange(&queue->start_times[qidx], &ts, 0)) { + atomic_add(&queue->start, 1); break; } }