From a99c480496542c1b2f11e223bae141d4ae61fb36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20Putegnat?= Date: Thu, 3 Sep 2026 17:08:25 +0200 Subject: [PATCH] Fix deadlock in Manager.Stop/StopReaders --- manager.go | 47 +++++++++++++++++++++++++++++++++++++++++------ perfmap.go | 23 ++++++++++++++++++++++- ringbuffer.go | 23 ++++++++++++++++++++++- 3 files changed, 85 insertions(+), 8 deletions(-) diff --git a/manager.go b/manager.go index f5b4a14..395405f 100644 --- a/manager.go +++ b/manager.go @@ -926,7 +926,9 @@ func (m *Manager) Stop(cleanup MapCleanupType) error { return m.stop(cleanup) } -// StopReaders stop the kernel events readers Perf or Ring buffer +// StopReaders stop the kernel events readers Perf or Ring buffer. +// It is not safe to call NewPerfRing or NewRingBuffer concurrently +// with StopReaders since we cannot put state to reset here. func (m *Manager) StopReaders(cleanup MapCleanupType) error { m.stateLock.Lock() defer m.stateLock.Unlock() @@ -938,15 +940,44 @@ func (m *Manager) stopReaders(cleanup MapCleanupType) error { // Stop perf ring readers for _, perfRing := range m.PerfMaps { - if stopErr := perfRing.Stop(cleanup); stopErr != nil { - errs = append(errs, fmt.Errorf("perf ring reader %s couldn't gracefully shut down: %w", perfRing.Name, stopErr)) + if closeErr := perfRing.closeReader(); closeErr != nil { + errs = append(errs, fmt.Errorf("perf ring reader %s couldn't gracefully shut down: %w", perfRing.Name, closeErr)) } } // Stop ring buffer readers for _, ringBuffer := range m.RingBuffers { - if stopErr := ringBuffer.Stop(cleanup); stopErr != nil { - errs = append(errs, fmt.Errorf("ring buffer reader %s couldn't gracefully shut down: %w", ringBuffer.Name, stopErr)) + if closeErr := ringBuffer.closeReader(); closeErr != nil { + errs = append(errs, fmt.Errorf("ring buffer reader %s couldn't gracefully shut down: %w", ringBuffer.Name, closeErr)) + } + } + + // Temporarily release manager.stateLock to avoid a deadlock: + // reader goroutine handlers may call Manager methods that need + // stateLock.RLock(). By releasing the write lock, handlers can + // complete, allowing goroutines to loop back to ReadInto which + // will return ErrClosed. + m.stateLock.Unlock() + + for _, perfRing := range m.PerfMaps { + perfRing.wgReader.Wait() + } + for _, ringBuffer := range m.RingBuffers { + ringBuffer.wgReader.Wait() + } + + // Now get the lock again + m.stateLock.Lock() + + // Clean up underlying maps + for _, perfRing := range m.PerfMaps { + if closeErr := perfRing.cleanupMap(cleanup); closeErr != nil { + errs = append(errs, fmt.Errorf("perf ring reader %s couldn't close map: %w", perfRing.Name, closeErr)) + } + } + for _, ringBuffer := range m.RingBuffers { + if closeErr := ringBuffer.cleanupMap(cleanup); closeErr != nil { + errs = append(errs, fmt.Errorf("ring buffer reader %s couldn't close map: %w", ringBuffer.Name, closeErr)) } } @@ -970,6 +1001,11 @@ func (m *Manager) stopProbes() error { } func (m *Manager) stop(cleanup MapCleanupType) error { + // Set state to reset early, to prevent concurrent operations (like + // NewPerfRing, NewRingBuffer) from adding new readers during the + // unlock window. + m.state = reset + var errs []error errs = append(errs, m.stopReaders(cleanup)) @@ -992,7 +1028,6 @@ func (m *Manager) stop(cleanup MapCleanupType) error { // removed from the collection. m.collection.Close() - m.state = reset return errors.Join(errs...) } diff --git a/perfmap.go b/perfmap.go index d5eb755..14334bf 100644 --- a/perfmap.go +++ b/perfmap.go @@ -208,7 +208,10 @@ func (m *PerfMap) Flush() { _ = m.perfReader.Flush() } -// Stop - Stops the perf ring buffer +// Stop - Stops the perf ring buffer. +// Warning: Stop waits for the reader goroutine while holding stateLock. +// If the caller also holds manager.stateLock, and a handler callback needs +// manager.stateLock.RLock(), this will deadlock. func (m *PerfMap) Stop(cleanup MapCleanupType) error { m.stateLock.Lock() defer m.stateLock.Unlock() @@ -234,6 +237,24 @@ func (m *PerfMap) Stop(cleanup MapCleanupType) error { return err } +// closeReader closes the perf reader to signal the reader goroutine to stop. +func (m *PerfMap) closeReader() error { + m.stateLock.Lock() + defer m.stateLock.Unlock() + if m.state <= stopped { + return nil + } + m.state = stopped + return m.perfReader.Close() +} + +// cleanupMap closes the underlying map with the given cleanup type. +func (m *PerfMap) cleanupMap(cleanup MapCleanupType) error { + m.stateLock.Lock() + defer m.stateLock.Unlock() + return m.close(cleanup) +} + // Pause - Pauses a perf ring buffer reader func (m *PerfMap) Pause() error { m.stateLock.Lock() diff --git a/ringbuffer.go b/ringbuffer.go index 03777a4..39abdf9 100644 --- a/ringbuffer.go +++ b/ringbuffer.go @@ -157,7 +157,10 @@ func (rb *RingBuffer) Flush() { _ = rb.ringReader.Flush() } -// Stop - Stops the perf ring buffer +// Stop - Stops the ring buffer. +// Warning: Stop waits for the reader goroutine while holding stateLock. +// If the caller also holds manager.stateLock, and a handler callback needs +// manager.stateLock.RLock(), this will deadlock. func (rb *RingBuffer) Stop(cleanup MapCleanupType) error { rb.stateLock.Lock() defer rb.stateLock.Unlock() @@ -183,6 +186,24 @@ func (rb *RingBuffer) Stop(cleanup MapCleanupType) error { return err } +// closeReader closes the ring reader to signal the reader goroutine to stop. +func (rb *RingBuffer) closeReader() error { + rb.stateLock.Lock() + defer rb.stateLock.Unlock() + if rb.state <= stopped { + return nil + } + rb.state = stopped + return rb.ringReader.Close() +} + +// cleanupMap closes the underlying map with the given cleanup type. +func (rb *RingBuffer) cleanupMap(cleanup MapCleanupType) error { + rb.stateLock.Lock() + defer rb.stateLock.Unlock() + return rb.close(cleanup) +} + // BufferSize returns the size in bytes of the ring buffer func (rb *RingBuffer) BufferSize() int { rb.stateLock.Lock()