Skip to content

Commit e990781

Browse files
safchaingithub-actions[bot]
safchain
authored and
github-actions[bot]
committed
[CWS] process cache entry refcount fixes (#36412)
(cherry picked from commit 7b87646)
1 parent bcf2b8a commit e990781

File tree

6 files changed

+158
-51
lines changed

6 files changed

+158
-51
lines changed

‎pkg/security/ebpf/map.go

+7
Original file line numberDiff line numberDiff line change
@@ -164,3 +164,10 @@ var (
164164
// BufferSelectorDNSResponseFilteredMonitorKey is the key used to select the filtered DNS responses
165165
BufferSelectorDNSResponseFilteredMonitorKey = Uint32MapItem(4)
166166
)
167+
168+
// Map is the interface for all eBPF maps
169+
type Map interface {
170+
LookupBytes(interface{}) ([]byte, error)
171+
Put(interface{}, interface{}) error
172+
Delete(interface{}) error
173+
}

‎pkg/security/resolvers/process/resolver_ebpf.go

+58-43
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
"go.uber.org/atomic"
3030

3131
"github.com/DataDog/datadog-agent/pkg/process/procutil"
32+
"github.com/DataDog/datadog-agent/pkg/security/ebpf"
3233
"github.com/DataDog/datadog-agent/pkg/security/metrics"
3334
"github.com/DataDog/datadog-agent/pkg/security/probe/config"
3435
"github.com/DataDog/datadog-agent/pkg/security/probe/managerhelper"
@@ -77,9 +78,9 @@ type EBPFResolver struct {
7778
pathResolver spath.ResolverInterface
7879
envVarsResolver *envvars.Resolver
7980

80-
inodeFileMap *lib.Map
81-
procCacheMap *lib.Map
82-
pidCacheMap *lib.Map
81+
inodeFileMap ebpf.Map
82+
procCacheMap ebpf.Map
83+
pidCacheMap ebpf.Map
8384
opts ResolverOpts
8485

8586
// stats
@@ -564,15 +565,16 @@ func (p *EBPFResolver) RetrieveFileFieldsFromProcfs(filename string) (*model.Fil
564565
return &fileFields, nil
565566
}
566567

567-
func (p *EBPFResolver) insertEntry(entry, prev *model.ProcessCacheEntry, source uint64) {
568+
func (p *EBPFResolver) insertEntry(entry *model.ProcessCacheEntry, source uint64) {
568569
entry.Source = source
569-
p.entryCache[entry.Pid] = entry
570-
entry.Retain()
571570

572-
if prev != nil {
571+
if prev := p.entryCache[entry.Pid]; prev != nil {
573572
prev.Release()
574573
}
575574

575+
p.entryCache[entry.Pid] = entry
576+
entry.Retain()
577+
576578
if p.cgroupResolver != nil && entry.CGroup.CGroupID != "" {
577579
// add the new PID in the right cgroup_resolver bucket
578580
p.cgroupResolver.AddPID(entry)
@@ -591,7 +593,6 @@ func (p *EBPFResolver) insertEntry(entry, prev *model.ProcessCacheEntry, source
591593
}
592594

593595
func (p *EBPFResolver) insertForkEntry(entry *model.ProcessCacheEntry, inode uint64, source uint64, newEntryCb func(*model.ProcessCacheEntry, error)) {
594-
595596
if entry.Pid == 0 {
596597
return
597598
}
@@ -619,7 +620,7 @@ func (p *EBPFResolver) insertForkEntry(entry *model.ProcessCacheEntry, inode uin
619620
}
620621
}
621622

622-
p.insertEntry(entry, prev, source)
623+
p.insertEntry(entry, source)
623624
}
624625

625626
func (p *EBPFResolver) insertExecEntry(entry *model.ProcessCacheEntry, inode uint64, source uint64) {
@@ -634,7 +635,7 @@ func (p *EBPFResolver) insertExecEntry(entry *model.ProcessCacheEntry, inode uin
634635
p.inodeErrStats.Inc()
635636
}
636637

637-
// check exec bomb
638+
// check exec bomb, keep the prev entry and update it
638639
if prev.Equals(entry) {
639640
prev.ApplyExecTimeOf(entry)
640641
return
@@ -644,7 +645,7 @@ func (p *EBPFResolver) insertExecEntry(entry *model.ProcessCacheEntry, inode uin
644645
entry.IsParentMissing = true
645646
}
646647

647-
p.insertEntry(entry, prev, source)
648+
p.insertEntry(entry, source)
648649
}
649650

650651
func (p *EBPFResolver) deleteEntry(pid uint32, exitTime time.Time) {
@@ -706,7 +707,7 @@ func (p *EBPFResolver) resolve(pid, tid uint32, inode uint64, useProcFS bool, ne
706707

707708
if p.procFallbackLimiter.Allow(pid) {
708709
// fallback to /proc, the in-kernel LRU may have deleted the entry
709-
if entry := p.resolveFromProcfs(pid, procResolveMaxDepth, newEntryCb); entry != nil {
710+
if entry := p.resolveFromProcfs(pid, inode, procResolveMaxDepth, newEntryCb); entry != nil {
710711
p.hitsStats[metrics.ProcFSTag].Inc()
711712
return entry
712713
}
@@ -950,13 +951,13 @@ func (p *EBPFResolver) resolveFromKernelMaps(pid, tid uint32, inode uint64, newE
950951
}
951952

952953
// ResolveFromProcfs resolves the entry from procfs
953-
func (p *EBPFResolver) ResolveFromProcfs(pid uint32, newEntryCb func(*model.ProcessCacheEntry, error)) *model.ProcessCacheEntry {
954+
func (p *EBPFResolver) ResolveFromProcfs(pid uint32, inode uint64, newEntryCb func(*model.ProcessCacheEntry, error)) *model.ProcessCacheEntry {
954955
p.Lock()
955956
defer p.Unlock()
956-
return p.resolveFromProcfs(pid, procResolveMaxDepth, newEntryCb)
957+
return p.resolveFromProcfs(pid, inode, procResolveMaxDepth, newEntryCb)
957958
}
958959

959-
func (p *EBPFResolver) resolveFromProcfs(pid uint32, maxDepth int, newEntryCb func(*model.ProcessCacheEntry, error)) *model.ProcessCacheEntry {
960+
func (p *EBPFResolver) resolveFromProcfs(pid uint32, inode uint64, maxDepth int, newEntryCb func(*model.ProcessCacheEntry, error)) *model.ProcessCacheEntry {
960961
if maxDepth < 1 {
961962
seclog.Tracef("max depth reached during procfs resolution: %d", pid)
962963
return nil
@@ -986,10 +987,12 @@ func (p *EBPFResolver) resolveFromProcfs(pid uint32, maxDepth int, newEntryCb fu
986987

987988
ppid := uint32(filledProc.Ppid)
988989
if ppid != 0 && p.entryCache[ppid] == nil {
989-
p.resolveFromProcfs(ppid, maxDepth-1, newEntryCb)
990+
// do not use the inode from the pid context on the parent
991+
// it may be a different process
992+
p.resolveFromProcfs(ppid, 0, maxDepth-1, newEntryCb)
990993
}
991994

992-
return p.newEntryFromProcfsAndSyncKernelMaps(proc, filledProc, model.ProcessCacheEntryFromProcFS, newEntryCb)
995+
return p.newEntryFromProcfsAndSyncKernelMaps(proc, filledProc, inode, model.ProcessCacheEntryFromProcFS, newEntryCb)
993996
}
994997

995998
// SetProcessArgs set arguments to cache entry
@@ -1284,7 +1287,7 @@ func (p *EBPFResolver) SyncCache(proc *process.Process) {
12841287
return
12851288
}
12861289

1287-
p.newEntryFromProcfsAndSyncKernelMaps(proc, filledProc, model.ProcessCacheEntryFromSnapshot, nil)
1290+
p.newEntryFromProcfsAndSyncKernelMaps(proc, filledProc, 0, model.ProcessCacheEntryFromSnapshot, nil)
12881291
}
12891292

12901293
func (p *EBPFResolver) setAncestor(pce *model.ProcessCacheEntry) {
@@ -1294,20 +1297,52 @@ func (p *EBPFResolver) setAncestor(pce *model.ProcessCacheEntry) {
12941297
}
12951298
}
12961299

1300+
func (p *EBPFResolver) syncKernelMaps(entry *model.ProcessCacheEntry) {
1301+
bootTime := p.timeResolver.GetBootTime()
1302+
1303+
// insert new entry in kernel maps
1304+
procCacheEntryB := make([]byte, 248)
1305+
_, err := entry.Process.MarshalProcCache(procCacheEntryB, bootTime)
1306+
if err != nil {
1307+
seclog.Errorf("couldn't marshal proc_cache entry: %s", err)
1308+
} else {
1309+
if err = p.procCacheMap.Put(entry.Cookie, procCacheEntryB); err != nil {
1310+
seclog.Errorf("couldn't push proc_cache entry to kernel space: %s", err)
1311+
}
1312+
}
1313+
pidCacheEntryB := make([]byte, 88)
1314+
_, err = entry.Process.MarshalPidCache(pidCacheEntryB, bootTime)
1315+
if err != nil {
1316+
seclog.Errorf("couldn't marshal pid_cache entry: %s", err)
1317+
} else {
1318+
if err = p.pidCacheMap.Put(entry.PIDContext.Pid, pidCacheEntryB); err != nil {
1319+
seclog.Errorf("couldn't push pid_cache entry to kernel space: %s", err)
1320+
}
1321+
}
1322+
}
1323+
12971324
// newEntryFromProcfsAndSyncKernelMaps snapshots /proc for the provided pid and sync the kernel maps
1298-
func (p *EBPFResolver) newEntryFromProcfsAndSyncKernelMaps(proc *process.Process, filledProc *utils.FilledProcess, source uint64, newEntryCb func(*model.ProcessCacheEntry, error)) *model.ProcessCacheEntry {
1325+
func (p *EBPFResolver) newEntryFromProcfsAndSyncKernelMaps(proc *process.Process, filledProc *utils.FilledProcess, inode uint64, source uint64, newEntryCb func(*model.ProcessCacheEntry, error)) *model.ProcessCacheEntry {
12991326
pid := uint32(proc.Pid)
13001327

13011328
entry := p.NewProcessCacheEntry(model.PIDContext{Pid: pid, Tid: pid})
13021329

13031330
// update the cache entry
13041331
if err := p.enrichEventFromProcfs(entry, proc, filledProc); err != nil {
1305-
entry.Release()
1306-
13071332
seclog.Trace(err)
13081333
return nil
13091334
}
13101335

1336+
// use the inode from the pid context if set so that we don't propagate a potentially wrong inode
1337+
if inode != 0 {
1338+
if entry.FileEvent.Inode != inode {
1339+
seclog.Errorf("inode mismatch, using inode from pid context %d: %d != %d", pid, entry.FileEvent.Inode, inode)
1340+
1341+
entry.FileEvent.Inode = inode
1342+
entry.IsParentMissing = true
1343+
}
1344+
}
1345+
13111346
entry.IsKworker = filledProc.Ppid == 0 && filledProc.Pid != 1
13121347

13131348
parent := p.entryCache[entry.PPid]
@@ -1325,29 +1360,9 @@ func (p *EBPFResolver) newEntryFromProcfsAndSyncKernelMaps(proc *process.Process
13251360
seclog.Debugf("unable to set the type of process, not pid 1, no parent in cache: %+v", entry)
13261361
}
13271362

1328-
p.insertEntry(entry, p.entryCache[pid], source)
1363+
p.insertEntry(entry, source)
13291364

1330-
bootTime := p.timeResolver.GetBootTime()
1331-
1332-
// insert new entry in kernel maps
1333-
procCacheEntryB := make([]byte, 248)
1334-
_, err := entry.Process.MarshalProcCache(procCacheEntryB, bootTime)
1335-
if err != nil {
1336-
seclog.Errorf("couldn't marshal proc_cache entry: %s", err)
1337-
} else {
1338-
if err = p.procCacheMap.Put(entry.Cookie, procCacheEntryB); err != nil {
1339-
seclog.Errorf("couldn't push proc_cache entry to kernel space: %s", err)
1340-
}
1341-
}
1342-
pidCacheEntryB := make([]byte, 88)
1343-
_, err = entry.Process.MarshalPidCache(pidCacheEntryB, bootTime)
1344-
if err != nil {
1345-
seclog.Errorf("couldn't marshal pid_cache entry: %s", err)
1346-
} else {
1347-
if err = p.pidCacheMap.Put(pid, pidCacheEntryB); err != nil {
1348-
seclog.Errorf("couldn't push pid_cache entry to kernel space: %s", err)
1349-
}
1350-
}
1365+
p.syncKernelMaps(entry)
13511366

13521367
seclog.Tracef("New process cache entry added: %s %s %d/%d", entry.Comm, entry.FileEvent.PathnameStr, pid, entry.FileEvent.Inode)
13531368

‎pkg/security/resolvers/process/resolver_test.go

+88-3
Original file line numberDiff line numberDiff line change
@@ -9,21 +9,83 @@
99
package process
1010

1111
import (
12+
"encoding/binary"
13+
"errors"
1214
"fmt"
15+
"os"
1316
"testing"
1417
"time"
1518

1619
"github.com/avast/retry-go/v4"
1720
"github.com/stretchr/testify/assert"
1821

22+
"github.com/DataDog/datadog-agent/pkg/security/probe/config"
1923
"github.com/DataDog/datadog-agent/pkg/security/resolvers/cgroup"
24+
"github.com/DataDog/datadog-agent/pkg/security/resolvers/container"
2025
"github.com/DataDog/datadog-agent/pkg/security/resolvers/path"
2126
"github.com/DataDog/datadog-agent/pkg/security/resolvers/usergroup"
2227
"github.com/DataDog/datadog-agent/pkg/security/secl/model"
2328
"github.com/DataDog/datadog-agent/pkg/util/ktime"
2429
"github.com/DataDog/datadog-go/v5/statsd"
2530
)
2631

32+
type fakeEBPMap struct {
33+
data map[string]interface{}
34+
}
35+
36+
func newFakeEBPMap() *fakeEBPMap {
37+
return &fakeEBPMap{
38+
data: make(map[string]interface{}),
39+
}
40+
}
41+
42+
func (f *fakeEBPMap) marshal(i interface{}) ([]byte, error) {
43+
switch value := i.(type) {
44+
case []byte:
45+
return value, nil
46+
case uint32:
47+
return binary.NativeEndian.AppendUint32(make([]byte, 0, 4), value), nil
48+
case uint64:
49+
return binary.NativeEndian.AppendUint64(make([]byte, 0, 8), value), nil
50+
default:
51+
return nil, fmt.Errorf("unsupported type %T", value)
52+
}
53+
}
54+
55+
func (f *fakeEBPMap) LookupBytes(key interface{}) ([]byte, error) {
56+
keyB, err := f.marshal(key)
57+
if err != nil {
58+
return nil, err
59+
}
60+
61+
if value, ok := f.data[string(keyB)]; ok {
62+
if b, ok := value.([]byte); ok {
63+
return b, nil
64+
}
65+
}
66+
return nil, errors.New("not found")
67+
}
68+
69+
func (f *fakeEBPMap) Put(key, value interface{}) error {
70+
keyB, err := f.marshal(key)
71+
if err != nil {
72+
return err
73+
}
74+
75+
f.data[string(keyB)] = value
76+
return nil
77+
}
78+
79+
func (f *fakeEBPMap) Delete(key interface{}) error {
80+
keyB, err := f.marshal(key)
81+
if err != nil {
82+
return err
83+
}
84+
85+
delete(f.data, string(keyB))
86+
return nil
87+
}
88+
2789
func newFakeForkEvent(ppid, pid int, inode uint64, resolver *EBPFResolver) *model.Event {
2890
e := model.NewFakeEvent()
2991
e.Type = uint32(model.ForkEventType)
@@ -83,7 +145,9 @@ func newResolver() (*EBPFResolver, error) {
83145
return nil, err
84146
}
85147

86-
resolver, err := NewEBPFResolver(nil, nil, &statsd.NoOpClient{}, nil, nil, nil, cgroupsResolver, userGroupResolver, timeResolver, &path.NoOpResolver{}, nil, NewResolverOpts())
148+
containerResolver := container.New()
149+
150+
resolver, err := NewEBPFResolver(nil, &config.Config{}, &statsd.NoOpClient{}, nil, containerResolver, nil, cgroupsResolver, userGroupResolver, timeResolver, &path.NoOpResolver{}, nil, NewResolverOpts())
87151
if err != nil {
88152
return nil, err
89153
}
@@ -232,6 +296,27 @@ func TestForkExec(t *testing.T) {
232296
testCacheSize(t, resolver)
233297
}
234298

299+
func TestResolveFromProcfs(t *testing.T) {
300+
resolver, err := newResolver()
301+
if err != nil {
302+
t.Fatal()
303+
}
304+
resolver.procCacheMap = newFakeEBPMap()
305+
resolver.pidCacheMap = newFakeEBPMap()
306+
resolver.inodeFileMap = newFakeEBPMap()
307+
308+
// use self pid so that the procfs entry exists and we have the permissions to read it
309+
pid := os.Getpid()
310+
311+
t.Run("sanitize-inode", func(t *testing.T) {
312+
entry := resolver.resolveFromProcfs(uint32(pid), 222, 1, func(pce *model.ProcessCacheEntry, _ error) {
313+
assert.Equal(t, uint64(222), pce.FileEvent.Inode)
314+
assert.True(t, pce.IsParentMissing)
315+
})
316+
assert.NotNil(t, entry)
317+
})
318+
}
319+
235320
func TestOrphanExec(t *testing.T) {
236321
resolver, err := newResolver()
237322
if err != nil {
@@ -746,15 +831,15 @@ func TestIsExecExecSnapshot(t *testing.T) {
746831
child3 := newFakeExecEvent(3, 4, 769, resolver)
747832

748833
// X(pid:3)
749-
resolver.insertEntry(parent.ProcessCacheEntry, nil, model.ProcessCacheEntryFromSnapshot)
834+
resolver.insertEntry(parent.ProcessCacheEntry, model.ProcessCacheEntryFromSnapshot)
750835
assert.Equal(t, parent.ProcessCacheEntry, resolver.entryCache[parent.ProcessCacheEntry.Pid])
751836
assert.Equal(t, 1, len(resolver.entryCache))
752837

753838
// X(pid:3)
754839
// |
755840
// X(pid:4)
756841
resolver.setAncestor(child.ProcessCacheEntry)
757-
resolver.insertEntry(child.ProcessCacheEntry, nil, model.ProcessCacheEntryFromSnapshot)
842+
resolver.insertEntry(child.ProcessCacheEntry, model.ProcessCacheEntryFromSnapshot)
758843
assert.Equal(t, child.ProcessCacheEntry, resolver.entryCache[child.ProcessCacheEntry.Pid])
759844
assert.Equal(t, 2, len(resolver.entryCache))
760845
assert.Equal(t, parent.ProcessCacheEntry, child.ProcessCacheEntry.Ancestor)

‎pkg/security/secl/model/model.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -461,8 +461,8 @@ func (pc *ProcessCacheEntry) callReleaseCallbacks() {
461461

462462
// Release decrement and eventually release the entry
463463
func (pc *ProcessCacheEntry) Release() {
464-
pc.refCount--
465-
if pc.refCount > 0 {
464+
if pc.refCount > 1 {
465+
pc.refCount--
466466
return
467467
}
468468

‎pkg/security/seclwin/model/model.go

+2-2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/security/tests/process_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -2284,7 +2284,7 @@ func TestProcessResolution(t *testing.T) {
22842284

22852285
// This makes use of the cache and do not parse /proc
22862286
// it still checks the ResolveFromProcfs returns the correct entry
2287-
procEntry := resolver.ResolveFromProcfs(pid, nil)
2287+
procEntry := resolver.ResolveFromProcfs(pid, 0, nil)
22882288
if procEntry == nil {
22892289
t.Errorf("not able to resolve the entry of pid %d", pid)
22902290
return

0 commit comments

Comments
 (0)