From 09f6262caaa9a080a151deb02fc6a86f361ba543 Mon Sep 17 00:00:00 2001 From: Rafael David Tinoco Date: Mon, 26 Jun 2023 20:08:49 +0000 Subject: [PATCH] fix(pkg/events): fix tailcall dependencies race issues --- pkg/ebpf/tracee.go | 6 +- pkg/ebpf/tracee_test.go | 27 +------ pkg/events/events.go | 170 ++++++++++++++++------------------------ 3 files changed, 71 insertions(+), 132 deletions(-) diff --git a/pkg/ebpf/tracee.go b/pkg/ebpf/tracee.go index 04a0510d64b..d9b66b25741 100644 --- a/pkg/ebpf/tracee.go +++ b/pkg/ebpf/tracee.go @@ -517,7 +517,7 @@ func (t *Tracee) initTailCall(tailCall *events.TailCall) error { } // Attach internal syscall probes if needed. - for _, index := range tailCall.GetMapIndexes() { + for _, index := range tailCall.GetIndexes() { def := events.Definitions.Get(events.ID(index)) if def.Syscall { err := t.probes.Attach(probes.SyscallEnter__Internal) @@ -1179,10 +1179,10 @@ func getTailCalls(eventConfigs map[events.ID]eventConfig) ([]*events.TailCall, e def := events.Definitions.Get(e) for _, tailCall := range def.Dependencies.TailCalls { - if tailCall.GetMapIndexesLen() == 0 { + if tailCall.GetIndexesLen() == 0 { continue // skip if tailcall has no indexes defined } - for _, index := range tailCall.GetMapIndexes() { + for _, index := range tailCall.GetIndexes() { if index >= uint32(events.MaxCommonID) { logger.Debugw( "Removing index from tail call (over max event id)", diff --git a/pkg/ebpf/tracee_test.go b/pkg/ebpf/tracee_test.go index ebc39a9c520..3eabfdc7faa 100644 --- a/pkg/ebpf/tracee_test.go +++ b/pkg/ebpf/tracee_test.go @@ -1,11 +1,9 @@ package ebpf import ( - "reflect" "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "github.com/aquasecurity/tracee/pkg/events" ) @@ -114,29 +112,8 @@ func Test_getTailCalls(t *testing.T) { t.Run(tc.name, func(t *testing.T) { tailCalls, err := getTailCalls(tc.events) - if tc.expectedErr != nil { - assert.ErrorIs(t, err, tc.expectedErr) - return - } - require.NoError(t, err) - for i := 0; i < len(tailCalls); i++ { - found := false - for j := 0; j < len(tc.expectedTailCalls); j++ { - if tailCalls[i].GetMapName() != tc.expectedTailCalls[j].GetMapName() { - continue - } - if tailCalls[i].GetProgName() != tc.expectedTailCalls[j].GetProgName() { - continue - } - if !reflect.DeepEqual(tailCalls[i].GetMapIndexes(), - tc.expectedTailCalls[j].GetMapIndexes(), - ) { - continue - } - found = true - } - assert.True(t, found) - } + assert.NoError(t, err) + assert.ElementsMatch(t, tc.expectedTailCalls, tailCalls) }, ) } diff --git a/pkg/events/events.go b/pkg/events/events.go index a76b0d9ff9c..7ae38301d6d 100644 --- a/pkg/events/events.go +++ b/pkg/events/events.go @@ -2,7 +2,6 @@ package events import ( "sync" - "sync/atomic" "kernel.org/pub/linux/libs/security/libcap/cap" @@ -14,42 +13,8 @@ import ( ) // -// Extensions WIP: +// Extensions WIP (PR: #3252 turning events instantiable) // -// NOTE: Historically the events were defined as static data with value receivers, instead of -// pointer receivers, and public fields were accessed directly from many places. That happened -// because the data never changed and there was no need for concurrency. That is no longer -// true, so all the events should now be defined as pointers with private fields and public -// getters/setters. This way access to events can be synchronized in the object themselves, -// instead of having to do it from the outside (and missing to do so). -// -// TODO: Because of dynamically loading/unloading work (and parallelism): -// -// 1. Simple types should be atomics (bool, int, uint, pointers, etc.): -// - os long as object changes are atomic (no need for multiple field updates) -// - usually requires object to have an immutable handle (ID) -// - protect loads/stores but not objects single view among 2 CPUs -// 2. Complex types should have mutexes (maps, slices, etc.) -// - updates aren't copy-on-write like atomic types -// - complex types are walked through, and not copy-on-write changed -// 3. All types should be private (no public fields) -// 4. All types should have getters/setters -// 5. Return copies of complex types (maps, slices, etc.) -// -// -// Types (keep in a single file for now): -// -// 0. ID immutable -// 1. EventGroup mutable, map protected by a mutex -// 2. Event mutable with atomic types (no transactional updates needed) -// 3. Dependencies mutable with atomic types (no transactional updates needed) -// 4. ProbeDependency mutable (atomic "required" field) -// 5. KSymbolDependency mutable (atomic "required" field) -// 6. CapDependency mutable, map protected by a mutex -// 7. TailCall mutable, slice protected by a mutex, atomic strings -// - -// TODO: turn event definitions into EventDefGroup of EventDefs: // // TailCall @@ -74,113 +39,110 @@ const ( ) type TailCall struct { - mapName *atomic.Pointer[string] // atomic: atomic pointer load/store (get/set) - progName *atomic.Pointer[string] // atomic: atomic pointer load/store (get/set) - mapIndexes []uint32 // load/store for array pointer also protected by mutex - mutex *sync.RWMutex // mutex: protect the array iterations (transactions) + mapName string + progName string + indexes map[uint32]struct{} + mutex *sync.RWMutex } // NewTailCall creates a new TailCall with default values. func NewTailCall(mapName, progName string, mapIndexes []uint32) *TailCall { - mapNamePtr := &atomic.Pointer[string]{} - progNamePtr := &atomic.Pointer[string]{} + indexes := make(map[uint32]struct{}) - mapNamePtr.Store(&mapName) - progNamePtr.Store(&progName) + for _, index := range mapIndexes { + indexes[index] = struct{}{} + } return &TailCall{ - mapName: mapNamePtr, - progName: progNamePtr, - mapIndexes: mapIndexes, - mutex: &sync.RWMutex{}, + mapName: mapName, + progName: progName, + indexes: indexes, + mutex: &sync.RWMutex{}, } } -// AddIndex adds an index to the tail call. -func (tc *TailCall) AddIndex(givenIndex uint32) { +// SetIndexes sets the indexes of the tailcall (thread-safe). +func (tc *TailCall) SetIndexes(idxs []uint32) { tc.mutex.Lock() defer tc.mutex.Unlock() - tc.addIndexes([]uint32{givenIndex}) + + // delete all previous indexes + for k := range tc.indexes { + delete(tc.indexes, k) + } + + tc.addIndexes(idxs) } -// AddIndexes adds indexes to the tail call. -func (tc *TailCall) AddIndexes(givenIndexes []uint32) { +// GetIndexes returns a slice copy of instanced tailcall indexes (thread-safe). +func (tc *TailCall) GetIndexes() []uint32 { + tc.mutex.RLock() + defer tc.mutex.RUnlock() + + indexes := make([]uint32, 0, len(tc.indexes)) + for k := range tc.indexes { + indexes = append(indexes, k) + } + + return indexes +} + +// AddIndex adds a tailcall index to the tailcall (thread-safe). +func (tc *TailCall) AddIndex(idx uint32) { tc.mutex.Lock() defer tc.mutex.Unlock() - tc.addIndexes(givenIndexes) -} -// addIndexes adds indexes to the tail call (thread-unsafe). -func (tc *TailCall) addIndexes(givenIndexes []uint32) { - tc.mapIndexes = append(tc.mapIndexes, givenIndexes...) + tc.indexes[idx] = struct{}{} } -// DelIndexes removes indexes from the tail call. -func (tc *TailCall) DelIndexes(givenIndexes []uint32) { +// AddIndexes adds tailcall indexes to the tailcall (thread-safe). +func (tc *TailCall) AddIndexes(idx []uint32) { tc.mutex.Lock() defer tc.mutex.Unlock() - for _, index := range givenIndexes { - tc.delIndex(index) - } + tc.addIndexes(idx) } -// DelIndex removes an index from the tail call. -func (tc *TailCall) DelIndex(givenIndex uint32) { +// DelIndex deletes a tailcall index from the tailcall (thread-safe). +func (tc *TailCall) DelIndex(idx uint32) { tc.mutex.Lock() defer tc.mutex.Unlock() - tc.delIndex(givenIndex) + + delete(tc.indexes, idx) } -// delIndex removes an index from the tail call (thread-unsafe). -func (tc *TailCall) delIndex(givenIndex uint32) { - indexOfIndex := -1 +// DelIndexes deletes tailcall indexes from the tailcall (thread-safe). +func (tc *TailCall) DelIndexes(idx []uint32) { + tc.mutex.Lock() + defer tc.mutex.Unlock() - for index, val := range tc.mapIndexes { - if givenIndex == val { - indexOfIndex = index - break - } - } - if indexOfIndex != -1 { - tc.mapIndexes = append( - tc.mapIndexes[:indexOfIndex], - tc.mapIndexes[indexOfIndex+1:]..., - ) + for _, i := range idx { + delete(tc.indexes, i) } } -// GetMapName returns a copy of the map's name. -func (tc *TailCall) GetMapName() string { - return *tc.mapName.Load() +// GetIndexesLen returns the number of indexes in the tailcall (thread-safe). +func (tc *TailCall) GetIndexesLen() int { + tc.mutex.RLock() + defer tc.mutex.RUnlock() + return len(tc.indexes) } -// SetMapName sets the map's name. -func (tc *TailCall) SetMapName(mapName string) { - tc.mapName.Store(&mapName) +// GetMapName returns the name of the tailcall map (thread-safe). +func (tc *TailCall) GetMapName() string { + return tc.mapName } -// GetProgName returns a copy of the program's name. +// GetProgName returns the name of the tailcall program (thread-safe). func (tc *TailCall) GetProgName() string { - return *tc.progName.Load() + return tc.progName } -// SetProgName sets the program's name. -func (tc *TailCall) SetProgName(progName string) { - tc.progName.Store(&progName) -} - -// GetMapIndexes returns a copy of the map's indexes at the time of the call. -func (tc *TailCall) GetMapIndexes() []uint32 { - tc.mutex.RLock() - defer tc.mutex.RUnlock() - return tc.mapIndexes // array copy protected by mutex -} - -func (tc *TailCall) GetMapIndexesLen() int { - tc.mutex.RLock() - defer tc.mutex.RUnlock() - return len(tc.mapIndexes) // array length protected by mutex +// addIndexes adds tailcall indexes to the tailcall (no locking). +func (tc *TailCall) addIndexes(idxs []uint32) { + for _, i := range idxs { + tc.indexes[i] = struct{}{} + } } //