From 88365f0b87650a919ce6ca84a0bd356f8f402ada Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Fri, 30 Oct 2026 11:27:10 +0100 Subject: [PATCH 1/5] fix(processtree): re-read the parent from procfs when an exec's parent is an unknown host process A `runc exec`'d process is reported with whichever parent it had when its exec event was captured: the containerd-shim if runc has already exited, or runc itself if it is still alive. runc is a host process, so the tree only holds a parentless placeholder for it and GetPidBranch cannot reach the shim. Alerts then fall back to a depth-1 tree with pcomm "runc", which made Test_24_ProcessTreeDepthTest flaky. On the exec path, when the parent chain reaches neither pid 1 nor the container's shim, re-read the current parent from /proc//stat and adopt it through UpdatePPID if it is already a tree node. The read targets the topmost node below the unknown ancestor, so a child exec repairs its parent too. It is guarded against pid reuse with the recorded procfs start time and a pending-exit check. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Matthias Bertschy --- .../processtree-runc-parent-reread.md | 46 +++++ pkg/processtree/creator/parent_reader.go | 45 +++++ .../creator/processtree_creator.go | 81 +++++++++ pkg/processtree/creator/runc_parent_test.go | 159 ++++++++++++++++++ 4 files changed, 331 insertions(+) create mode 100644 docs/features/processtree-runc-parent-reread.md create mode 100644 pkg/processtree/creator/parent_reader.go create mode 100644 pkg/processtree/creator/runc_parent_test.go diff --git a/docs/features/processtree-runc-parent-reread.md b/docs/features/processtree-runc-parent-reread.md new file mode 100644 index 0000000000..7330ac17bb --- /dev/null +++ b/docs/features/processtree-runc-parent-reread.md @@ -0,0 +1,46 @@ +# Process tree — re-reading an exec's parent after `runc exec` + +A `kubectl exec` (or any `runc exec`) starts a process whose parent depends on +timing. When its exec event is captured, the parent is either: + +- the **containerd-shim**, if `runc exec` has already exited and the kernel has + reparented the process; or +- the **runc** process, if runc is still alive. + +runc is a host process. In Kubernetes mode the tree holds only a parentless +placeholder for it, so the branch from the exec'd process never reaches the +shim. `GetPidBranch` then fails and the alert falls back to a depth-1 tree whose +parent is `runc` and whose start time is zero. Before this fix only a later exec +on the same pid repaired it, through `UpdatePPID`. CI saw this on roughly 30% of +process-tree roots, which made `Test_24_ProcessTreeDepthTest` flaky. + +## What the creator does + +`handleExecEvent` calls `rereadUnknownParent` after the normal `UpdatePPID`. It +re-reads the process's **current** parent from `/proc//stat` field 4. By +the time the event is handled, runc has usually exited and the parent is the +shim. + +| Step | Rule | +|---|---| +| Gate | Kubernetes mode only, a non-host container id, a non-zero parent, an acyclic chain that reaches neither pid 1 nor the container's shim. Every other exec skips the read. | +| Target | The topmost node below the unknown ancestor. This is usually the exec'd process. It is the process's parent when a child execs before the parent's chain was repaired. | +| Pid reuse | Skip when the target has a pending exit. Skip when the `/proc` start time (field 22) differs from the recorded `pidStartTimeNs`. Skip when an ancestor target has no recorded start time. The exec being handled proves that its own pid is live. | +| Adoption | Only a parent that is already a tree node, and never one recorded as younger than the target. Adoption goes through `UpdatePPID`, so the container-subtree rules still apply. | +| No change | The pid is gone, the stat fails to parse, the parent is unchanged, or the new parent is unknown. The tree keeps today's behaviour. | + +## Cost and locking + +The read is one `/proc//stat` open-read-parse, about 7.5 µs. It runs under +the tree's write lock, the same trade-off `ensureStartTime` makes on the exec +path. It is acceptable because the gate keeps it off the common path: an exec +whose parent chain is already known never reads `/proc`. + +The reader is the `readParent` field on the creator (`parent_reader.go`). Tests +inject a stub in its place. + +## Limits + +If runc is still alive when the event is handled, the read returns runc and +nothing changes. The next exec in that subtree, or the next `/proc` scan, can +still repair the chain. diff --git a/pkg/processtree/creator/parent_reader.go b/pkg/processtree/creator/parent_reader.go new file mode 100644 index 0000000000..a6c89a256a --- /dev/null +++ b/pkg/processtree/creator/parent_reader.go @@ -0,0 +1,45 @@ +package processtreecreator + +import ( + "sync" + + "github.com/kubescape/go-logger" + "github.com/kubescape/go-logger/helpers" + "github.com/prometheus/procfs" +) + +// newProcfsParentReader returns a reader for a pid's CURRENT parent and its +// boot-relative start time, both from /proc//stat (fields 4 and 22). It +// uses the same /proc mount as the on-demand start-time reader. +// +// ok is false when the process is gone (ENOENT/ESRCH) or the stat cannot be +// parsed; callers must then keep the parent they already have. +func newProcfsParentReader() func(pid uint32) (ppid uint32, startTimeNs uint64, ok bool) { + var ( + once sync.Once + fs procfs.FS + fsReady bool + ) + return func(pid uint32) (uint32, uint64, bool) { + once.Do(func() { + f, err := procfs.NewDefaultFS() + if err != nil { + logger.L().Warning("processtree: cannot open /proc, parent re-read disabled", helpers.Error(err)) + return + } + fs, fsReady = f, true + }) + if !fsReady { + return 0, 0, false + } + proc, err := fs.Proc(int(pid)) + if err != nil { + return 0, 0, false + } + stat, err := proc.Stat() + if err != nil || stat.PPID <= 0 { + return 0, 0, false + } + return uint32(stat.PPID), stat.Starttime * nsPerTick, true + } +} diff --git a/pkg/processtree/creator/processtree_creator.go b/pkg/processtree/creator/processtree_creator.go index a4a08d5df3..aa0aeb0c85 100644 --- a/pkg/processtree/creator/processtree_creator.go +++ b/pkg/processtree/creator/processtree_creator.go @@ -61,6 +61,11 @@ type processTreeCreatorImpl struct { // recorded creation time. Injectable for tests; read through pt.bootTimeNs(), // which tolerates a nil hook by returning 0 ("unknown"). nowBootNs func() uint64 + + // readParent fetches a pid's CURRENT parent and boot-relative start time + // from /proc//stat. Used only by rereadUnknownParent, on the exec path + // whose parent has no known ancestry. Injectable for tests. + readParent func(pid uint32) (ppid uint32, startTimeNs uint64, ok bool) } func NewProcessTreeCreator(containerTree containerprocesstree.ContainerProcessTree, config config.Config) ProcessTreeCreator { @@ -79,6 +84,7 @@ func NewProcessTreeCreator(containerTree containerprocesstree.ContainerProcessTr pidStartTimeNs: make(map[uint32]uint64), readStartTime: newProcfsStartTimeReader(), nowBootNs: readBootTimeNs, + readParent: newProcfsParentReader(), config: config, } @@ -216,6 +222,79 @@ func (pt *processTreeCreatorImpl) ancestryRoot(pid uint32) (root uint32, cyclic return 0, true } +// rereadUnknownParent repairs the `runc exec` race. A `kubectl exec`'d process +// is reported with whichever parent it had when its exec event was captured: +// the containerd-shim if `runc exec` has already exited, or the runc process if +// it is still alive. runc is a host process, so the tree only holds a +// parentless placeholder for it and the branch cannot reach the shim. By the +// time the event is handled runc has usually exited and the kernel has +// reparented the process to the shim, so the current parent is re-read from +// /proc and adopted when it is already a tree node. +// +// The re-read targets the topmost node below the unknown ancestor, not +// necessarily proc: a child of the exec'd process may exec before its parent's +// chain was repaired, and it is the parent that hangs off runc. +// +// Only reached when the chain does not reach the container's shim or a known +// host root, so the /proc read under the tree lock stays off the common path, +// the same trade-off ensureStartTime makes for exec. +// +// Must be called with pt.mutex held. +func (pt *processTreeCreatorImpl) rereadUnknownParent(proc *armotypes.Process, containerID string) { + if proc.PPID == 0 || pt.readParent == nil || containerID == "" || utils.IsHost(containerID) { + return + } + if root, cyclic := pt.ancestryRoot(proc.PID); cyclic || root == 1 { + return + } + if pt.containerTree.IsProcessUnderContainer(proc.PID, containerID, &pt.processMap) { + return + } + + // Walk up to the last node whose parent is missing or a placeholder. + // ancestryRoot proved the chain acyclic, so this terminates. + anchor := proc + for { + parent, ok := pt.processMap.Load(anchor.PPID) + if !ok || parent.PPID == 0 { + break + } + anchor = parent + } + + // A node awaiting deletion may have had its pid recycled already. + if _, exited := pt.pendingExits[anchor.PID]; exited { + return + } + + ppid, startNs, ok := pt.readParent(anchor.PID) + if !ok || ppid == anchor.PPID || ppid == 0 { + return + } + + // Pid-reuse guard: the /proc reading must describe the same incarnation the + // node does. The exec being handled proves proc itself is live, so its + // identity may be unknown; an ancestor's may not. + if known := pt.pidStartTimeNs[anchor.PID]; known != 0 { + if known != startNs { + return + } + } else if anchor != proc { + return + } + + // Only adopt a parent the tree already knows, and never one recorded as + // younger than its child — that node belongs to a recycled pid. + if _, ok := pt.processMap.Load(ppid); !ok { + return + } + if parentNs := pt.pidStartTimeNs[ppid]; parentNs != 0 && startNs != 0 && parentNs > startNs { + return + } + + pt.UpdatePPID(anchor, conversion.ProcessEvent{PID: anchor.PID, PPID: ppid, ContainerID: containerID}) +} + // ensureStartTime gives a fork/exec-created node its process identity without // waiting up to a full scan interval for the periodic /proc sweep — short-lived // processes are the population most exposed to pid reuse, and they are exactly @@ -442,6 +521,8 @@ func (pt *processTreeCreatorImpl) handleExecEvent(event conversion.ProcessEvent) pt.UpdatePPID(proc, event) if pt.config.KubernetesMode { + pt.rereadUnknownParent(proc, event.ContainerID) + root, cyclic := pt.ancestryRoot(proc.PID) // Unknown parents can arrive after this exec. Preserve the raw parent // instead of permanently cutting that chain at the shim. Retain fallback diff --git a/pkg/processtree/creator/runc_parent_test.go b/pkg/processtree/creator/runc_parent_test.go new file mode 100644 index 0000000000..1ecb48314d --- /dev/null +++ b/pkg/processtree/creator/runc_parent_test.go @@ -0,0 +1,159 @@ +package processtreecreator + +import ( + "os" + "testing" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/kubescape/node-agent/pkg/processtree/conversion" + "github.com/stretchr/testify/require" +) + +const runcPID, execPID, execChildPID = uint32(4000000500), uint32(4000000600), uint32(4000000700) + +// stubParentReader answers readParent from a fixed table and counts calls. +func stubParentReader(table map[uint32][2]uint64, calls *int) func(uint32) (uint32, uint64, bool) { + return func(pid uint32) (uint32, uint64, bool) { + *calls++ + e, ok := table[pid] + if !ok { + return 0, 0, false + } + return uint32(e[0]), e[1], true + } +} + +func execUnderRunc(c *processTreeCreatorImpl, pid, ppid uint32, comm string) { + c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: pid, PPID: ppid, Comm: comm, Pcomm: "runc", ContainerID: ancestryContainerID}) +} + +func TestExecRereadsRuncParentToShim(t *testing.T) { + c, tree, shim := newAncestryTestCreator(t) + calls := 0 + c.readParent = stubParentReader(map[uint32][2]uint64{execPID: {uint64(shim), 0}}, &calls) + + execUnderRunc(c, execPID, runcPID, "sh") + require.Equal(t, 1, calls) + require.Equal(t, shim, c.processMap.Get(execPID).PPID, "the exec must be reparented to the shim") + require.NotContains(t, c.processMap.Get(runcPID).ChildrenMap, armotypes.CommPID{PID: execPID}, "runc placeholder must drop the child") + + // A child of the repaired process resolves through the shim with full depth. + c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execChildPID, PPID: execPID, Comm: "sleep", ContainerID: ancestryContainerID}) + require.Equal(t, 1, calls, "a known parent chain must not re-read /proc") + branch, err := tree.GetPidBranch(ancestryContainerID, execChildPID, &c.processMap) + require.NoError(t, err) + require.Equal(t, execPID, branch.PID) + require.Len(t, branch.ChildrenMap, 1) + for _, child := range branch.ChildrenMap { + require.Equal(t, execChildPID, child.PID) + } +} + +func TestExecRereadsAncestorWhenChildExecsFirst(t *testing.T) { + c, tree, shim := newAncestryTestCreator(t) + calls := 0 + table := map[uint32][2]uint64{execPID: {uint64(runcPID), 0}} + c.readParent = stubParentReader(table, &calls) + + // runc still alive when the exec is handled: the parent stays runc. + execUnderRunc(c, execPID, runcPID, "sh") + require.Equal(t, runcPID, c.processMap.Get(execPID).PPID) + + // runc exits; the child's exec repairs its parent's chain. + c.pidStartTimeNs[execPID] = 5_000_000_000 + table[execPID] = [2]uint64{uint64(shim), 5_000_000_000} + c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execChildPID, PPID: execPID, Comm: "sleep", ContainerID: ancestryContainerID}) + require.Equal(t, shim, c.processMap.Get(execPID).PPID) + _, err := tree.GetPidBranch(ancestryContainerID, execChildPID, &c.processMap) + require.NoError(t, err) +} + +func TestExecRereadRejectsRecycledAncestor(t *testing.T) { + for _, tc := range []struct { + name string + recorded uint64 + readStart uint64 + }{ + {name: "start time differs", recorded: 5_000_000_000, readStart: 6_000_000_000}, + {name: "ancestor identity unknown", recorded: 0, readStart: 6_000_000_000}, + } { + t.Run(tc.name, func(t *testing.T) { + c, _, shim := newAncestryTestCreator(t) + calls := 0 + table := map[uint32][2]uint64{execPID: {uint64(runcPID), 0}} + c.readParent = stubParentReader(table, &calls) + execUnderRunc(c, execPID, runcPID, "sh") + if tc.recorded != 0 { + c.pidStartTimeNs[execPID] = tc.recorded + } + table[execPID] = [2]uint64{uint64(shim), tc.readStart} + c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execChildPID, PPID: execPID, ContainerID: ancestryContainerID}) + require.Equal(t, runcPID, c.processMap.Get(execPID).PPID) + }) + } +} + +func TestExecRereadRejectsYoungerParent(t *testing.T) { + c, _, shim := newAncestryTestCreator(t) + calls := 0 + c.readParent = stubParentReader(map[uint32][2]uint64{execPID: {uint64(shim), 5_000_000_000}}, &calls) + c.pidStartTimeNs[shim] = 9_000_000_000 + execUnderRunc(c, execPID, runcPID, "sh") + require.Equal(t, 1, calls) + require.Equal(t, runcPID, c.processMap.Get(execPID).PPID) +} + +func TestExecRereadKeepsParentWhenUnresolved(t *testing.T) { + for _, tc := range []struct { + name string + table map[uint32][2]uint64 + }{ + {name: "pid vanished", table: map[uint32][2]uint64{}}, + {name: "ppid unchanged", table: map[uint32][2]uint64{execPID: {uint64(runcPID), 0}}}, + {name: "ppid not in tree", table: map[uint32][2]uint64{execPID: {4000000999, 0}}}, + } { + t.Run(tc.name, func(t *testing.T) { + c, tree, _ := newAncestryTestCreator(t) + calls := 0 + c.readParent = stubParentReader(tc.table, &calls) + execUnderRunc(c, execPID, runcPID, "sh") + require.Equal(t, 1, calls) + require.Equal(t, runcPID, c.processMap.Get(execPID).PPID) + require.Equal(t, "sh", c.processMap.Get(execPID).Comm) + _, ok := c.processMap.Load(4000000999) + require.False(t, ok) + _, err := tree.GetPidBranch(ancestryContainerID, execPID, &c.processMap) + require.Error(t, err) + }) + } +} + +func TestExecRereadNotAttemptedOutsideKubernetes(t *testing.T) { + c, _, shim := newAncestryTestCreator(t) + c.config = createTestConfig(false) + calls := 0 + c.readParent = stubParentReader(map[uint32][2]uint64{execPID: {uint64(shim), 0}}, &calls) + execUnderRunc(c, execPID, runcPID, "sh") + require.Zero(t, calls) + require.Equal(t, runcPID, c.processMap.Get(execPID).PPID) +} + +func TestExecRereadNotAttemptedForKnownParent(t *testing.T) { + c, _, shim := newAncestryTestCreator(t) + calls := 0 + c.readParent = stubParentReader(nil, &calls) + c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execPID, PPID: shim, ContainerID: ancestryContainerID}) + require.Zero(t, calls) +} + +func TestProcfsParentReaderReadsSelf(t *testing.T) { + read := newProcfsParentReader() + ppid, start, ok := read(uint32(os.Getpid())) + require.True(t, ok) + require.Equal(t, uint32(os.Getppid()), ppid) + require.NotZero(t, start) + wantStart, _ := newProcfsStartTimeReader()(uint32(os.Getpid())) + require.Equal(t, wantStart, start) + _, _, ok = read(4000000999) + require.False(t, ok, "a vanished pid must report not ok") +} From ef59d15df0f32a97dab6789b3e99be6c63766b06 Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Fri, 30 Oct 2026 11:48:53 +0100 Subject: [PATCH 2/5] fix(processtree): verify process identity before adopting a re-read parent Address review findings on #1063: - 4422755922: the procfs parent reader retries opening /proc on each call until it succeeds instead of caching a first failure. - 4422755936, 4422790111: a first-seen exec target must prove its identity. Its /proc start time must not be later than the event's exec timestamp mapped back to the boot clock; otherwise, or when the timestamp is unknown, the event's parent is kept. - 4422755943: the new parent node must have no pending exit, and its /proc start time must match the recorded one when known and must not be later than the child's. - 4422755956: the repaired node's Pcomm is set to the adopted parent's Comm, and the event's stale Pcomm is not applied after adoption. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Matthias Bertschy --- .../processtree-runc-parent-reread.md | 15 ++- pkg/processtree/creator/parent_reader.go | 28 +++-- .../creator/processtree_creator.go | 86 ++++++++++--- pkg/processtree/creator/runc_parent_test.go | 115 ++++++++++++++---- 4 files changed, 189 insertions(+), 55 deletions(-) diff --git a/docs/features/processtree-runc-parent-reread.md b/docs/features/processtree-runc-parent-reread.md index 7330ac17bb..f5a1845803 100644 --- a/docs/features/processtree-runc-parent-reread.md +++ b/docs/features/processtree-runc-parent-reread.md @@ -25,19 +25,26 @@ shim. |---|---| | Gate | Kubernetes mode only, a non-host container id, a non-zero parent, an acyclic chain that reaches neither pid 1 nor the container's shim. Every other exec skips the read. | | Target | The topmost node below the unknown ancestor. This is usually the exec'd process. It is the process's parent when a child execs before the parent's chain was repaired. | -| Pid reuse | Skip when the target has a pending exit. Skip when the `/proc` start time (field 22) differs from the recorded `pidStartTimeNs`. Skip when an ancestor target has no recorded start time. The exec being handled proves that its own pid is live. | -| Adoption | Only a parent that is already a tree node, and never one recorded as younger than the target. Adoption goes through `UpdatePPID`, so the container-subtree rules still apply. | +| Pid reuse (target) | Skip when the target has a pending exit. With a recorded `pidStartTimeNs`, skip unless the `/proc` start time (field 22) matches it. Without one, only the exec'd process itself can be identified: its `/proc` start time must not be later than the event's exec timestamp mapped back to the boot clock. A later start time means the exec was handled after its pid was recycled. If the timestamp is unknown, or the target is an ancestor with no recorded start time, skip. | +| Pid reuse (parent) | The new parent must already be a tree node with no pending exit. Its `/proc` start time is read too. It must match the node's recorded start time when there is one, and must not be later than the target's. Host shims have no recorded start time in Kubernetes mode, because host procfs events never reach the side map. | +| Adoption | Adoption goes through `UpdatePPID`, so the container-subtree rules still apply. The repaired node's `Pcomm` becomes the adopted parent's `Comm`. For the exec'd process itself, the event's `Pcomm` (which names runc) is then not applied. | | No change | The pid is gone, the stat fails to parse, the parent is unchanged, or the new parent is unknown. The tree keeps today's behaviour. | ## Cost and locking -The read is one `/proc//stat` open-read-parse, about 7.5 µs. It runs under +Each read is one `/proc//stat` open-read-parse, about 7.5 µs. An adoption +takes two reads: the target and the new parent. It runs under the tree's write lock, the same trade-off `ensureStartTime` makes on the exec path. It is acceptable because the gate keeps it off the common path: an exec whose parent chain is already known never reads `/proc`. The reader is the `readParent` field on the creator (`parent_reader.go`). Tests -inject a stub in its place. +inject a stub in its place. If `/proc` cannot be opened, the reader retries on +the next call instead of staying disabled. + +The exec timestamp is inspektor-gadget's `bpf_ktime_get_boot_ns` value plus a +fixed wall-clock offset. The creator samples the same offset once at +construction (`bootToWallNs`) and subtracts it. ## Limits diff --git a/pkg/processtree/creator/parent_reader.go b/pkg/processtree/creator/parent_reader.go index a6c89a256a..e8e634d489 100644 --- a/pkg/processtree/creator/parent_reader.go +++ b/pkg/processtree/creator/parent_reader.go @@ -12,26 +12,34 @@ import ( // boot-relative start time, both from /proc//stat (fields 4 and 22). It // uses the same /proc mount as the on-demand start-time reader. // +// Opening /proc is retried on every call until it succeeds, then kept. The +// reader only runs on the rare unknown-parent exec path, so a retry costs one +// stat of the mount point there and never touches the hot path. +// // ok is false when the process is gone (ENOENT/ESRCH) or the stat cannot be // parsed; callers must then keep the parent they already have. func newProcfsParentReader() func(pid uint32) (ppid uint32, startTimeNs uint64, ok bool) { var ( - once sync.Once - fs procfs.FS - fsReady bool + mu sync.Mutex + fs procfs.FS + ready bool + warned bool ) return func(pid uint32) (uint32, uint64, bool) { - once.Do(func() { + mu.Lock() + if !ready { f, err := procfs.NewDefaultFS() if err != nil { - logger.L().Warning("processtree: cannot open /proc, parent re-read disabled", helpers.Error(err)) - return + if !warned { + logger.L().Warning("processtree: cannot open /proc, parent re-read unavailable until it can", helpers.Error(err)) + warned = true + } + mu.Unlock() + return 0, 0, false } - fs, fsReady = f, true - }) - if !fsReady { - return 0, 0, false + fs, ready = f, true } + mu.Unlock() proc, err := fs.Proc(int(pid)) if err != nil { return 0, 0, false diff --git a/pkg/processtree/creator/processtree_creator.go b/pkg/processtree/creator/processtree_creator.go index aa0aeb0c85..0ee2d205ae 100644 --- a/pkg/processtree/creator/processtree_creator.go +++ b/pkg/processtree/creator/processtree_creator.go @@ -66,6 +66,14 @@ type processTreeCreatorImpl struct { // from /proc//stat. Used only by rereadUnknownParent, on the exec path // whose parent has no known ancestry. Injectable for tests. readParent func(pid uint32) (ppid uint32, startTimeNs uint64, ok bool) + + // bootToWallNs is wall-clock ns minus CLOCK_BOOTTIME ns, sampled once at + // construction: the same fixed offset inspektor-gadget adds to an exec + // event's bpf_ktime_get_boot_ns timestamp. Subtracting it maps the exec + // instant back to the boot clock, so rereadUnknownParent can tell whether a + // /proc reading describes a process created after the exec, i.e. a recycled + // pid. Zero means unknown. Injectable for tests. + bootToWallNs int64 } func NewProcessTreeCreator(containerTree containerprocesstree.ContainerProcessTree, config config.Config) ProcessTreeCreator { @@ -87,6 +95,9 @@ func NewProcessTreeCreator(containerTree containerprocesstree.ContainerProcessTr readParent: newProcfsParentReader(), config: config, } + if boot := readBootTimeNs(); boot != 0 { + creator.bootToWallNs = time.Now().UnixNano() - int64(boot) + } return creator } @@ -239,16 +250,20 @@ func (pt *processTreeCreatorImpl) ancestryRoot(pid uint32) (root uint32, cyclic // host root, so the /proc read under the tree lock stays off the common path, // the same trade-off ensureStartTime makes for exec. // +// Returns true when proc itself was given a new parent, so the caller must not +// overwrite the adopted parent's comm with the event's stale Pcomm. +// // Must be called with pt.mutex held. -func (pt *processTreeCreatorImpl) rereadUnknownParent(proc *armotypes.Process, containerID string) { +func (pt *processTreeCreatorImpl) rereadUnknownParent(proc *armotypes.Process, event conversion.ProcessEvent) bool { + containerID := event.ContainerID if proc.PPID == 0 || pt.readParent == nil || containerID == "" || utils.IsHost(containerID) { - return + return false } if root, cyclic := pt.ancestryRoot(proc.PID); cyclic || root == 1 { - return + return false } if pt.containerTree.IsProcessUnderContainer(proc.PID, containerID, &pt.processMap) { - return + return false } // Walk up to the last node whose parent is missing or a placeholder. @@ -264,35 +279,64 @@ func (pt *processTreeCreatorImpl) rereadUnknownParent(proc *armotypes.Process, c // A node awaiting deletion may have had its pid recycled already. if _, exited := pt.pendingExits[anchor.PID]; exited { - return + return false } ppid, startNs, ok := pt.readParent(anchor.PID) - if !ok || ppid == anchor.PPID || ppid == 0 { - return + if !ok || ppid == anchor.PPID || ppid == 0 || startNs == 0 { + return false } // Pid-reuse guard: the /proc reading must describe the same incarnation the - // node does. The exec being handled proves proc itself is live, so its - // identity may be unknown; an ancestor's may not. + // node does. A recorded start time must match. Without one, only proc can be + // identified: the event's exec instant, mapped to the boot clock, must not + // precede the /proc creation time. An exec handled after its pid was + // recycled reads a replacement created after that instant. if known := pt.pidStartTimeNs[anchor.PID]; known != 0 { if known != startNs { - return + return false } - } else if anchor != proc { - return + } else if anchor != proc || !pt.createdBeforeExec(startNs, event.StartTimeNs) { + return false } - // Only adopt a parent the tree already knows, and never one recorded as - // younger than its child — that node belongs to a recycled pid. - if _, ok := pt.processMap.Load(ppid); !ok { - return + // Only adopt a parent the tree already knows, and only the incarnation the + // node describes: no pending exit, a matching recorded start time, and never + // younger than the child. Host shims have no recorded start time in + // Kubernetes mode, because host procfs events never reach the side map. + parent, ok := pt.processMap.Load(ppid) + if !ok { + return false } - if parentNs := pt.pidStartTimeNs[ppid]; parentNs != 0 && startNs != 0 && parentNs > startNs { - return + if _, exited := pt.pendingExits[ppid]; exited { + return false + } + _, parentNs, ok := pt.readParent(ppid) + if !ok || parentNs == 0 || parentNs > startNs { + return false + } + if known := pt.pidStartTimeNs[ppid]; known != 0 && known != parentNs { + return false } pt.UpdatePPID(anchor, conversion.ProcessEvent{PID: anchor.PID, PPID: ppid, ContainerID: containerID}) + if anchor.PPID != ppid { + return false + } + anchor.Pcomm = parent.Comm + return anchor == proc +} + +// createdBeforeExec reports whether a process created at startNs (boot clock, +// /proc field 22) can be the one that performed an exec stamped execWallNs +// (inspektor-gadget wall time). /proc truncates to clock ticks, so the same +// process always reads at or before its exec. Unknown values prove nothing and +// report false. +func (pt *processTreeCreatorImpl) createdBeforeExec(startNs, execWallNs uint64) bool { + if pt.bootToWallNs == 0 || execWallNs == 0 || int64(execWallNs) <= pt.bootToWallNs { + return false + } + return startNs <= uint64(int64(execWallNs)-pt.bootToWallNs) } // ensureStartTime gives a fork/exec-created node its process identity without @@ -520,8 +564,9 @@ func (pt *processTreeCreatorImpl) handleExecEvent(event conversion.ProcessEvent) pt.UpdatePPID(proc, event) + adopted := false if pt.config.KubernetesMode { - pt.rereadUnknownParent(proc, event.ContainerID) + adopted = pt.rereadUnknownParent(proc, event) root, cyclic := pt.ancestryRoot(proc.PID) // Unknown parents can arrive after this exec. Preserve the raw parent @@ -541,7 +586,8 @@ func (pt *processTreeCreatorImpl) handleExecEvent(event conversion.ProcessEvent) if event.Comm != "" { proc.Comm = event.Comm } - if event.Pcomm != "" { + // The event's Pcomm names the parent it reported, not one adopted above. + if event.Pcomm != "" && !adopted { proc.Pcomm = event.Pcomm } if event.Cmdline != "" { diff --git a/pkg/processtree/creator/runc_parent_test.go b/pkg/processtree/creator/runc_parent_test.go index 1ecb48314d..360828e5d5 100644 --- a/pkg/processtree/creator/runc_parent_test.go +++ b/pkg/processtree/creator/runc_parent_test.go @@ -5,12 +5,24 @@ import ( "testing" "github.com/armosec/armoapi-go/armotypes" + containerprocesstree "github.com/kubescape/node-agent/pkg/processtree/container" "github.com/kubescape/node-agent/pkg/processtree/conversion" "github.com/stretchr/testify/require" ) const runcPID, execPID, execChildPID = uint32(4000000500), uint32(4000000600), uint32(4000000700) +// Boot-relative times used by the runc tests. The exec happens after the +// process was created; a recycled pid reads a later creation time. +const ( + testWallOffsetNs = int64(1_700_000_000_000_000_000) + shimStartNs = uint64(1_000_000_000) + execStartNs = uint64(5_000_000_000) + execBootNs = uint64(5_500_000_000) + recycledStartNs = uint64(7_000_000_000) + shimComm = "containerd-shim" +) + // stubParentReader answers readParent from a fixed table and counts calls. func stubParentReader(table map[uint32][2]uint64, calls *int) func(uint32) (uint32, uint64, bool) { return func(pid uint32) (uint32, uint64, bool) { @@ -23,23 +35,34 @@ func stubParentReader(table map[uint32][2]uint64, calls *int) func(uint32) (uint } } +// newRuncTestCreator names the shim node and fixes the wall/boot offset so an +// exec stamped execBootNs maps back to the boot clock. +func newRuncTestCreator(t *testing.T) (*processTreeCreatorImpl, containerprocesstree.ContainerProcessTree, uint32) { + t.Helper() + c, tree, shim := newAncestryTestCreator(t) + c.bootToWallNs = testWallOffsetNs + c.processMap.Get(shim).Comm = shimComm + return c, tree, shim +} + func execUnderRunc(c *processTreeCreatorImpl, pid, ppid uint32, comm string) { - c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: pid, PPID: ppid, Comm: comm, Pcomm: "runc", ContainerID: ancestryContainerID}) + c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: pid, PPID: ppid, Comm: comm, Pcomm: "runc", ContainerID: ancestryContainerID, StartTimeNs: uint64(testWallOffsetNs) + execBootNs}) } func TestExecRereadsRuncParentToShim(t *testing.T) { - c, tree, shim := newAncestryTestCreator(t) + c, tree, shim := newRuncTestCreator(t) calls := 0 - c.readParent = stubParentReader(map[uint32][2]uint64{execPID: {uint64(shim), 0}}, &calls) + c.readParent = stubParentReader(map[uint32][2]uint64{execPID: {uint64(shim), execStartNs}, shim: {1, shimStartNs}}, &calls) execUnderRunc(c, execPID, runcPID, "sh") - require.Equal(t, 1, calls) + require.Equal(t, 2, calls) require.Equal(t, shim, c.processMap.Get(execPID).PPID, "the exec must be reparented to the shim") + require.Equal(t, shimComm, c.processMap.Get(execPID).Pcomm, "the event's runc pcomm must not survive adoption") require.NotContains(t, c.processMap.Get(runcPID).ChildrenMap, armotypes.CommPID{PID: execPID}, "runc placeholder must drop the child") // A child of the repaired process resolves through the shim with full depth. c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execChildPID, PPID: execPID, Comm: "sleep", ContainerID: ancestryContainerID}) - require.Equal(t, 1, calls, "a known parent chain must not re-read /proc") + require.Equal(t, 2, calls, "a known parent chain must not re-read /proc") branch, err := tree.GetPidBranch(ancestryContainerID, execChildPID, &c.processMap) require.NoError(t, err) require.Equal(t, execPID, branch.PID) @@ -50,20 +73,23 @@ func TestExecRereadsRuncParentToShim(t *testing.T) { } func TestExecRereadsAncestorWhenChildExecsFirst(t *testing.T) { - c, tree, shim := newAncestryTestCreator(t) + c, tree, shim := newRuncTestCreator(t) calls := 0 - table := map[uint32][2]uint64{execPID: {uint64(runcPID), 0}} + table := map[uint32][2]uint64{execPID: {uint64(runcPID), execStartNs}, shim: {1, shimStartNs}} c.readParent = stubParentReader(table, &calls) // runc still alive when the exec is handled: the parent stays runc. execUnderRunc(c, execPID, runcPID, "sh") require.Equal(t, runcPID, c.processMap.Get(execPID).PPID) + require.Equal(t, "runc", c.processMap.Get(execPID).Pcomm) // runc exits; the child's exec repairs its parent's chain. - c.pidStartTimeNs[execPID] = 5_000_000_000 - table[execPID] = [2]uint64{uint64(shim), 5_000_000_000} - c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execChildPID, PPID: execPID, Comm: "sleep", ContainerID: ancestryContainerID}) + c.pidStartTimeNs[execPID] = execStartNs + table[execPID] = [2]uint64{uint64(shim), execStartNs} + c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execChildPID, PPID: execPID, Comm: "sleep", Pcomm: "sh", ContainerID: ancestryContainerID}) require.Equal(t, shim, c.processMap.Get(execPID).PPID) + require.Equal(t, shimComm, c.processMap.Get(execPID).Pcomm, "the repaired ancestor must name its adopted parent") + require.Equal(t, "sh", c.processMap.Get(execChildPID).Pcomm, "the child's own parent is unchanged") _, err := tree.GetPidBranch(ancestryContainerID, execChildPID, &c.processMap) require.NoError(t, err) } @@ -74,13 +100,13 @@ func TestExecRereadRejectsRecycledAncestor(t *testing.T) { recorded uint64 readStart uint64 }{ - {name: "start time differs", recorded: 5_000_000_000, readStart: 6_000_000_000}, - {name: "ancestor identity unknown", recorded: 0, readStart: 6_000_000_000}, + {name: "start time differs", recorded: execStartNs, readStart: recycledStartNs}, + {name: "ancestor identity unknown", recorded: 0, readStart: recycledStartNs}, } { t.Run(tc.name, func(t *testing.T) { - c, _, shim := newAncestryTestCreator(t) + c, _, shim := newRuncTestCreator(t) calls := 0 - table := map[uint32][2]uint64{execPID: {uint64(runcPID), 0}} + table := map[uint32][2]uint64{execPID: {uint64(runcPID), execStartNs}, shim: {1, shimStartNs}} c.readParent = stubParentReader(table, &calls) execUnderRunc(c, execPID, runcPID, "sh") if tc.recorded != 0 { @@ -93,13 +119,60 @@ func TestExecRereadRejectsRecycledAncestor(t *testing.T) { } } +// An exec handled after its pid was recycled: /proc describes the replacement, +// created after the exec instant, and its parent must not be adopted. +func TestExecRereadRejectsRecycledExecTarget(t *testing.T) { + for _, tc := range []struct { + name string + execWall uint64 + }{ + {name: "replacement created after the exec", execWall: uint64(testWallOffsetNs) + execBootNs}, + {name: "exec timestamp unknown", execWall: 0}, + } { + t.Run(tc.name, func(t *testing.T) { + c, _, shim := newRuncTestCreator(t) + calls := 0 + start := recycledStartNs + if tc.execWall == 0 { + start = execStartNs + } + c.readParent = stubParentReader(map[uint32][2]uint64{execPID: {uint64(shim), start}, shim: {1, shimStartNs}}, &calls) + c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execPID, PPID: runcPID, Comm: "sh", Pcomm: "runc", ContainerID: ancestryContainerID, StartTimeNs: tc.execWall}) + require.Equal(t, 1, calls) + require.Equal(t, runcPID, c.processMap.Get(execPID).PPID, "the event's parent must be kept") + require.Equal(t, "runc", c.processMap.Get(execPID).Pcomm) + }) + } +} + +// The tree's node for the new ppid may be a stale, exited process whose pid was +// recycled; it must not be adopted. +func TestExecRereadRejectsStaleParentNode(t *testing.T) { + for _, tc := range []struct { + name string + setup func(c *processTreeCreatorImpl, shim uint32) + }{ + {name: "recorded start time differs", setup: func(c *processTreeCreatorImpl, shim uint32) { c.pidStartTimeNs[shim] = shimStartNs + 1 }}, + {name: "pending exit", setup: func(c *processTreeCreatorImpl, shim uint32) { c.pendingExits[shim] = &pendingExit{} }}, + } { + t.Run(tc.name, func(t *testing.T) { + c, _, shim := newRuncTestCreator(t) + calls := 0 + c.readParent = stubParentReader(map[uint32][2]uint64{execPID: {uint64(shim), execStartNs}, shim: {1, shimStartNs}}, &calls) + tc.setup(c, shim) + execUnderRunc(c, execPID, runcPID, "sh") + require.Equal(t, runcPID, c.processMap.Get(execPID).PPID) + require.Equal(t, "runc", c.processMap.Get(execPID).Pcomm) + }) + } +} + func TestExecRereadRejectsYoungerParent(t *testing.T) { - c, _, shim := newAncestryTestCreator(t) + c, _, shim := newRuncTestCreator(t) calls := 0 - c.readParent = stubParentReader(map[uint32][2]uint64{execPID: {uint64(shim), 5_000_000_000}}, &calls) - c.pidStartTimeNs[shim] = 9_000_000_000 + c.readParent = stubParentReader(map[uint32][2]uint64{execPID: {uint64(shim), execStartNs}, shim: {1, execStartNs + 1}}, &calls) execUnderRunc(c, execPID, runcPID, "sh") - require.Equal(t, 1, calls) + require.Equal(t, 2, calls) require.Equal(t, runcPID, c.processMap.Get(execPID).PPID) } @@ -109,11 +182,11 @@ func TestExecRereadKeepsParentWhenUnresolved(t *testing.T) { table map[uint32][2]uint64 }{ {name: "pid vanished", table: map[uint32][2]uint64{}}, - {name: "ppid unchanged", table: map[uint32][2]uint64{execPID: {uint64(runcPID), 0}}}, - {name: "ppid not in tree", table: map[uint32][2]uint64{execPID: {4000000999, 0}}}, + {name: "ppid unchanged", table: map[uint32][2]uint64{execPID: {uint64(runcPID), execStartNs}}}, + {name: "ppid not in tree", table: map[uint32][2]uint64{execPID: {4000000999, execStartNs}}}, } { t.Run(tc.name, func(t *testing.T) { - c, tree, _ := newAncestryTestCreator(t) + c, tree, _ := newRuncTestCreator(t) calls := 0 c.readParent = stubParentReader(tc.table, &calls) execUnderRunc(c, execPID, runcPID, "sh") From 6cdf49ade50467a52210e36a102e8f25d1677521 Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Fri, 30 Oct 2026 11:55:11 +0100 Subject: [PATCH 3/5] fix(processtree): keep re-read parent consistent across successive delayed execs Address Copilot review 5711226822 on #1063: - The exec target's /proc start time is checked against the event's exec timestamp even when a start time is already recorded. An earlier delayed exec can record a recycled pid's replacement through ensureStartTime, which made the second exec pass the recorded-time match and adopt unrelated ancestry. - An exec event's Pcomm is applied only when its PPID is the parent the node keeps, so a queued exec that still reports runc no longer relabels a node whose parent was repaired to the shim. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Matthias Bertschy --- .../processtree-runc-parent-reread.md | 4 +- .../creator/processtree_creator.go | 58 ++++++++++--------- pkg/processtree/creator/runc_parent_test.go | 31 ++++++++++ 3 files changed, 63 insertions(+), 30 deletions(-) diff --git a/docs/features/processtree-runc-parent-reread.md b/docs/features/processtree-runc-parent-reread.md index f5a1845803..d83bed40c2 100644 --- a/docs/features/processtree-runc-parent-reread.md +++ b/docs/features/processtree-runc-parent-reread.md @@ -25,9 +25,9 @@ shim. |---|---| | Gate | Kubernetes mode only, a non-host container id, a non-zero parent, an acyclic chain that reaches neither pid 1 nor the container's shim. Every other exec skips the read. | | Target | The topmost node below the unknown ancestor. This is usually the exec'd process. It is the process's parent when a child execs before the parent's chain was repaired. | -| Pid reuse (target) | Skip when the target has a pending exit. With a recorded `pidStartTimeNs`, skip unless the `/proc` start time (field 22) matches it. Without one, only the exec'd process itself can be identified: its `/proc` start time must not be later than the event's exec timestamp mapped back to the boot clock. A later start time means the exec was handled after its pid was recycled. If the timestamp is unknown, or the target is an ancestor with no recorded start time, skip. | +| Pid reuse (target) | Skip when the target has a pending exit. With a recorded `pidStartTimeNs`, skip unless the `/proc` start time (field 22) matches it. When the target is the exec'd process itself, its `/proc` start time must also not be later than the event's exec timestamp mapped back to the boot clock. This applies even with a recorded start time, because an earlier delayed exec may have recorded the replacement's time. A later start time means the exec was handled after its pid was recycled. If the timestamp is unknown, or the target is an ancestor with no recorded start time, skip. | | Pid reuse (parent) | The new parent must already be a tree node with no pending exit. Its `/proc` start time is read too. It must match the node's recorded start time when there is one, and must not be later than the target's. Host shims have no recorded start time in Kubernetes mode, because host procfs events never reach the side map. | -| Adoption | Adoption goes through `UpdatePPID`, so the container-subtree rules still apply. The repaired node's `Pcomm` becomes the adopted parent's `Comm`. For the exec'd process itself, the event's `Pcomm` (which names runc) is then not applied. | +| Adoption | Adoption goes through `UpdatePPID`, so the container-subtree rules still apply. The repaired node's `Pcomm` becomes the adopted parent's `Comm`. An exec event's `Pcomm` is applied only when the event's parent is the parent the node keeps, so a later exec that still reports runc does not relabel a repaired node. | | No change | The pid is gone, the stat fails to parse, the parent is unchanged, or the new parent is unknown. The tree keeps today's behaviour. | ## Cost and locking diff --git a/pkg/processtree/creator/processtree_creator.go b/pkg/processtree/creator/processtree_creator.go index 0ee2d205ae..f8a4bcce7d 100644 --- a/pkg/processtree/creator/processtree_creator.go +++ b/pkg/processtree/creator/processtree_creator.go @@ -250,20 +250,17 @@ func (pt *processTreeCreatorImpl) ancestryRoot(pid uint32) (root uint32, cyclic // host root, so the /proc read under the tree lock stays off the common path, // the same trade-off ensureStartTime makes for exec. // -// Returns true when proc itself was given a new parent, so the caller must not -// overwrite the adopted parent's comm with the event's stale Pcomm. -// // Must be called with pt.mutex held. -func (pt *processTreeCreatorImpl) rereadUnknownParent(proc *armotypes.Process, event conversion.ProcessEvent) bool { +func (pt *processTreeCreatorImpl) rereadUnknownParent(proc *armotypes.Process, event conversion.ProcessEvent) { containerID := event.ContainerID if proc.PPID == 0 || pt.readParent == nil || containerID == "" || utils.IsHost(containerID) { - return false + return } if root, cyclic := pt.ancestryRoot(proc.PID); cyclic || root == 1 { - return false + return } if pt.containerTree.IsProcessUnderContainer(proc.PID, containerID, &pt.processMap) { - return false + return } // Walk up to the last node whose parent is missing or a placeholder. @@ -279,25 +276,30 @@ func (pt *processTreeCreatorImpl) rereadUnknownParent(proc *armotypes.Process, e // A node awaiting deletion may have had its pid recycled already. if _, exited := pt.pendingExits[anchor.PID]; exited { - return false + return } ppid, startNs, ok := pt.readParent(anchor.PID) if !ok || ppid == anchor.PPID || ppid == 0 || startNs == 0 { - return false + return } // Pid-reuse guard: the /proc reading must describe the same incarnation the - // node does. A recorded start time must match. Without one, only proc can be - // identified: the event's exec instant, mapped to the boot clock, must not - // precede the /proc creation time. An exec handled after its pid was - // recycled reads a replacement created after that instant. - if known := pt.pidStartTimeNs[anchor.PID]; known != 0 { - if known != startNs { - return false + // node does. A recorded start time must match. For proc, the event's exec + // instant, mapped to the boot clock, must also not precede the /proc + // creation time, even when a start time is recorded: an exec handled after + // its pid was recycled reads a replacement created after that instant, and + // an earlier delayed exec may already have recorded that replacement's time. + // Without a recorded start time, an ancestor cannot be identified. + if known := pt.pidStartTimeNs[anchor.PID]; known != 0 && known != startNs { + return + } + if anchor == proc { + if !pt.createdBeforeExec(startNs, event.StartTimeNs) { + return } - } else if anchor != proc || !pt.createdBeforeExec(startNs, event.StartTimeNs) { - return false + } else if pt.pidStartTimeNs[anchor.PID] == 0 { + return } // Only adopt a parent the tree already knows, and only the incarnation the @@ -306,25 +308,24 @@ func (pt *processTreeCreatorImpl) rereadUnknownParent(proc *armotypes.Process, e // Kubernetes mode, because host procfs events never reach the side map. parent, ok := pt.processMap.Load(ppid) if !ok { - return false + return } if _, exited := pt.pendingExits[ppid]; exited { - return false + return } _, parentNs, ok := pt.readParent(ppid) if !ok || parentNs == 0 || parentNs > startNs { - return false + return } if known := pt.pidStartTimeNs[ppid]; known != 0 && known != parentNs { - return false + return } pt.UpdatePPID(anchor, conversion.ProcessEvent{PID: anchor.PID, PPID: ppid, ContainerID: containerID}) if anchor.PPID != ppid { - return false + return } anchor.Pcomm = parent.Comm - return anchor == proc } // createdBeforeExec reports whether a process created at startNs (boot clock, @@ -564,9 +565,8 @@ func (pt *processTreeCreatorImpl) handleExecEvent(event conversion.ProcessEvent) pt.UpdatePPID(proc, event) - adopted := false if pt.config.KubernetesMode { - adopted = pt.rereadUnknownParent(proc, event) + pt.rereadUnknownParent(proc, event) root, cyclic := pt.ancestryRoot(proc.PID) // Unknown parents can arrive after this exec. Preserve the raw parent @@ -586,8 +586,10 @@ func (pt *processTreeCreatorImpl) handleExecEvent(event conversion.ProcessEvent) if event.Comm != "" { proc.Comm = event.Comm } - // The event's Pcomm names the parent it reported, not one adopted above. - if event.Pcomm != "" && !adopted { + // The event's Pcomm names the parent it reported. Apply it only when that + // parent was kept: a re-read adoption, or an earlier exec's adoption that a + // delayed exec could not undo, must not be relabelled with a stale comm. + if event.Pcomm != "" && (event.PPID == 0 || event.PPID == proc.PPID) { proc.Pcomm = event.Pcomm } if event.Cmdline != "" { diff --git a/pkg/processtree/creator/runc_parent_test.go b/pkg/processtree/creator/runc_parent_test.go index 360828e5d5..114884b059 100644 --- a/pkg/processtree/creator/runc_parent_test.go +++ b/pkg/processtree/creator/runc_parent_test.go @@ -3,6 +3,7 @@ package processtreecreator import ( "os" "testing" + "time" "github.com/armosec/armoapi-go/armotypes" containerprocesstree "github.com/kubescape/node-agent/pkg/processtree/container" @@ -143,6 +144,36 @@ func TestExecRereadRejectsRecycledExecTarget(t *testing.T) { require.Equal(t, "runc", c.processMap.Get(execPID).Pcomm) }) } + + // Successive delayed execs: the first one records the replacement's start + // time through ensureStartTime, so the second sees a matching recorded time + // and must still be rejected by its exec timestamp. + t.Run("successive delayed execs", func(t *testing.T) { + c, _, shim := newRuncTestCreator(t) + calls := 0 + c.readParent = stubParentReader(map[uint32][2]uint64{execPID: {uint64(shim), recycledStartNs}, shim: {1, shimStartNs}}, &calls) + c.readStartTime = func(uint32) (uint64, time.Time) { return recycledStartNs, time.Time{} } + execUnderRunc(c, execPID, runcPID, "sh") + require.Equal(t, recycledStartNs, c.pidStartTimeNs[execPID], "the first exec caches the replacement's start time") + execUnderRunc(c, execPID, runcPID, "sh") + require.Equal(t, runcPID, c.processMap.Get(execPID).PPID, "the second delayed exec must not adopt the replacement's parent") + require.Equal(t, "runc", c.processMap.Get(execPID).Pcomm) + }) +} + +// Two queued execs for the same pid both report runc. The first repairs the +// parent; the second keeps it and must not relabel Pcomm back to runc. +func TestExecRereadKeepsRepairedPcommOnSuccessiveExecs(t *testing.T) { + c, _, shim := newRuncTestCreator(t) + calls := 0 + c.readParent = stubParentReader(map[uint32][2]uint64{execPID: {uint64(shim), execStartNs}, shim: {1, shimStartNs}}, &calls) + c.readStartTime = func(uint32) (uint64, time.Time) { return execStartNs, time.Time{} } + execUnderRunc(c, execPID, runcPID, "sh") + require.Equal(t, shim, c.processMap.Get(execPID).PPID) + require.Equal(t, shimComm, c.processMap.Get(execPID).Pcomm) + execUnderRunc(c, execPID, runcPID, "sh") + require.Equal(t, shim, c.processMap.Get(execPID).PPID) + require.Equal(t, shimComm, c.processMap.Get(execPID).Pcomm, "a stale runc pcomm must not overwrite the repaired parent's comm") } // The tree's node for the new ppid may be a stale, exited process whose pid was From 4eb7f0326cd750ac02a20c080d1fa7d107d0b49d Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Fri, 30 Oct 2026 12:02:00 +0100 Subject: [PATCH 4/5] fix(processtree): check ancestors against the child exec time and reuse IG's clock offset Address Copilot review 5711296045 on #1063: - An ancestor re-read target's /proc start time must also not be later than the child event's exec timestamp. A delayed parent exec can cache a recycled pid's replacement start time through ensureStartTime, which let a queued child exec stamped before the replacement existed attach the old ancestry to the replacement's parent. - The boot-to-wall offset is read back from gadgets.WallTimeFromBootTime, which produces exec timestamps in this node-agent version, instead of being resampled at construction. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Matthias Bertschy --- .../processtree-runc-parent-reread.md | 7 +-- .../creator/processtree_creator.go | 43 ++++++++++--------- pkg/processtree/creator/runc_parent_test.go | 20 ++++++++- 3 files changed, 45 insertions(+), 25 deletions(-) diff --git a/docs/features/processtree-runc-parent-reread.md b/docs/features/processtree-runc-parent-reread.md index d83bed40c2..f73031b5e5 100644 --- a/docs/features/processtree-runc-parent-reread.md +++ b/docs/features/processtree-runc-parent-reread.md @@ -25,7 +25,7 @@ shim. |---|---| | Gate | Kubernetes mode only, a non-host container id, a non-zero parent, an acyclic chain that reaches neither pid 1 nor the container's shim. Every other exec skips the read. | | Target | The topmost node below the unknown ancestor. This is usually the exec'd process. It is the process's parent when a child execs before the parent's chain was repaired. | -| Pid reuse (target) | Skip when the target has a pending exit. With a recorded `pidStartTimeNs`, skip unless the `/proc` start time (field 22) matches it. When the target is the exec'd process itself, its `/proc` start time must also not be later than the event's exec timestamp mapped back to the boot clock. This applies even with a recorded start time, because an earlier delayed exec may have recorded the replacement's time. A later start time means the exec was handled after its pid was recycled. If the timestamp is unknown, or the target is an ancestor with no recorded start time, skip. | +| Pid reuse (target) | Skip when the target has a pending exit. With a recorded `pidStartTimeNs`, skip unless the `/proc` start time (field 22) matches it. The target's `/proc` start time must also not be later than the event's exec timestamp mapped back to the boot clock. The target is the exec'd process or one of its ancestors, and both existed before that exec. This applies even with a recorded start time, because an earlier delayed exec may have recorded the replacement's time. A later start time means the exec was handled after a pid was recycled. If the timestamp is unknown, or the target is an ancestor with no recorded start time, skip. | | Pid reuse (parent) | The new parent must already be a tree node with no pending exit. Its `/proc` start time is read too. It must match the node's recorded start time when there is one, and must not be later than the target's. Host shims have no recorded start time in Kubernetes mode, because host procfs events never reach the side map. | | Adoption | Adoption goes through `UpdatePPID`, so the container-subtree rules still apply. The repaired node's `Pcomm` becomes the adopted parent's `Comm`. An exec event's `Pcomm` is applied only when the event's parent is the parent the node keeps, so a later exec that still reports runc does not relabel a repaired node. | | No change | The pid is gone, the stat fails to parse, the parent is unchanged, or the new parent is unknown. The tree keeps today's behaviour. | @@ -43,8 +43,9 @@ inject a stub in its place. If `/proc` cannot be opened, the reader retries on the next call instead of staying disabled. The exec timestamp is inspektor-gadget's `bpf_ktime_get_boot_ns` value plus a -fixed wall-clock offset. The creator samples the same offset once at -construction (`bootToWallNs`) and subtracts it. +fixed wall-clock offset, added by `gadgets.WallTimeFromBootTime`. The creator +reads that same offset back from the function at construction +(`bootToWallNs`) instead of sampling its own, and subtracts it. ## Limits diff --git a/pkg/processtree/creator/processtree_creator.go b/pkg/processtree/creator/processtree_creator.go index f8a4bcce7d..7288fcf786 100644 --- a/pkg/processtree/creator/processtree_creator.go +++ b/pkg/processtree/creator/processtree_creator.go @@ -7,6 +7,7 @@ import ( "github.com/armosec/armoapi-go/armotypes" "github.com/goradd/maps" + "github.com/inspektor-gadget/inspektor-gadget/pkg/gadgets" "github.com/kubescape/go-logger" "github.com/kubescape/go-logger/helpers" "github.com/kubescape/node-agent/pkg/config" @@ -67,12 +68,13 @@ type processTreeCreatorImpl struct { // whose parent has no known ancestry. Injectable for tests. readParent func(pid uint32) (ppid uint32, startTimeNs uint64, ok bool) - // bootToWallNs is wall-clock ns minus CLOCK_BOOTTIME ns, sampled once at - // construction: the same fixed offset inspektor-gadget adds to an exec - // event's bpf_ktime_get_boot_ns timestamp. Subtracting it maps the exec - // instant back to the boot clock, so rereadUnknownParent can tell whether a - // /proc reading describes a process created after the exec, i.e. a recycled - // pid. Zero means unknown. Injectable for tests. + // bootToWallNs is the fixed wall-minus-boot offset that + // gadgets.WallTimeFromBootTime adds to an exec event's bpf_ktime_get_boot_ns + // timestamp. It is read back from that function rather than resampled, so + // it is exactly the offset IG sampled at package init. Subtracting it maps + // the exec instant back to the boot clock, so rereadUnknownParent can tell + // whether a /proc reading describes a process created after the exec, i.e. + // a recycled pid. Zero means unknown. Injectable for tests. bootToWallNs int64 } @@ -93,10 +95,9 @@ func NewProcessTreeCreator(containerTree containerprocesstree.ContainerProcessTr readStartTime: newProcfsStartTimeReader(), nowBootNs: readBootTimeNs, readParent: newProcfsParentReader(), - config: config, - } - if boot := readBootTimeNs(); boot != 0 { - creator.bootToWallNs = time.Now().UnixNano() - int64(boot) + // WallTimeFromBootTime(ts) is ts plus IG's offset for any non-zero ts. + bootToWallNs: int64(gadgets.WallTimeFromBootTime(1)) - 1, + config: config, } return creator @@ -285,20 +286,20 @@ func (pt *processTreeCreatorImpl) rereadUnknownParent(proc *armotypes.Process, e } // Pid-reuse guard: the /proc reading must describe the same incarnation the - // node does. A recorded start time must match. For proc, the event's exec - // instant, mapped to the boot clock, must also not precede the /proc - // creation time, even when a start time is recorded: an exec handled after - // its pid was recycled reads a replacement created after that instant, and - // an earlier delayed exec may already have recorded that replacement's time. - // Without a recorded start time, an ancestor cannot be identified. + // node does. A recorded start time must match. The event's exec instant, + // mapped to the boot clock, must also not precede the /proc creation time, + // even when a start time is recorded: proc and every ancestor of it existed + // before this exec. An exec handled after a pid was recycled reads a + // replacement created after that instant, and an earlier delayed exec may + // already have recorded that replacement's time. Without a recorded start + // time, an ancestor cannot be identified. if known := pt.pidStartTimeNs[anchor.PID]; known != 0 && known != startNs { return } - if anchor == proc { - if !pt.createdBeforeExec(startNs, event.StartTimeNs) { - return - } - } else if pt.pidStartTimeNs[anchor.PID] == 0 { + if anchor != proc && pt.pidStartTimeNs[anchor.PID] == 0 { + return + } + if !pt.createdBeforeExec(startNs, event.StartTimeNs) { return } diff --git a/pkg/processtree/creator/runc_parent_test.go b/pkg/processtree/creator/runc_parent_test.go index 114884b059..fc4775a7a8 100644 --- a/pkg/processtree/creator/runc_parent_test.go +++ b/pkg/processtree/creator/runc_parent_test.go @@ -87,7 +87,7 @@ func TestExecRereadsAncestorWhenChildExecsFirst(t *testing.T) { // runc exits; the child's exec repairs its parent's chain. c.pidStartTimeNs[execPID] = execStartNs table[execPID] = [2]uint64{uint64(shim), execStartNs} - c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execChildPID, PPID: execPID, Comm: "sleep", Pcomm: "sh", ContainerID: ancestryContainerID}) + c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execChildPID, PPID: execPID, Comm: "sleep", Pcomm: "sh", ContainerID: ancestryContainerID, StartTimeNs: uint64(testWallOffsetNs) + execBootNs}) require.Equal(t, shim, c.processMap.Get(execPID).PPID) require.Equal(t, shimComm, c.processMap.Get(execPID).Pcomm, "the repaired ancestor must name its adopted parent") require.Equal(t, "sh", c.processMap.Get(execChildPID).Pcomm, "the child's own parent is unchanged") @@ -161,6 +161,24 @@ func TestExecRereadRejectsRecycledExecTarget(t *testing.T) { }) } +// A delayed parent exec caches its recycled pid's replacement start time. A +// queued child exec stamped before the replacement existed must not trust that +// cached value and attach the old ancestry to the replacement's parent. +func TestExecRereadRejectsAncestorCreatedAfterChildExec(t *testing.T) { + c, _, shim := newRuncTestCreator(t) + calls := 0 + c.readParent = stubParentReader(map[uint32][2]uint64{execPID: {uint64(shim), recycledStartNs}, shim: {1, shimStartNs}}, &calls) + c.readStartTime = func(uint32) (uint64, time.Time) { return recycledStartNs, time.Time{} } + execUnderRunc(c, execPID, runcPID, "sh") + require.Equal(t, runcPID, c.processMap.Get(execPID).PPID) + require.Equal(t, recycledStartNs, c.pidStartTimeNs[execPID], "the parent exec caches the replacement's start time") + + childExecBootNs := recycledStartNs - 1_000_000_000 + c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execChildPID, PPID: execPID, Comm: "sleep", Pcomm: "sh", ContainerID: ancestryContainerID, StartTimeNs: uint64(testWallOffsetNs) + childExecBootNs}) + require.Equal(t, runcPID, c.processMap.Get(execPID).PPID, "an ancestor created after the child's exec must not be repaired") + require.Equal(t, "runc", c.processMap.Get(execPID).Pcomm) +} + // Two queued execs for the same pid both report runc. The first repairs the // parent; the second keeps it and must not relabel Pcomm back to runc. func TestExecRereadKeepsRepairedPcommOnSuccessiveExecs(t *testing.T) { From 8c6f618f54692f3189bbf45645b8a741cf5a39d2 Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Fri, 30 Oct 2026 12:06:41 +0100 Subject: [PATCH 5/5] test(processtree): give the child a valid exec time so identity-guard tests isolate their guard Address Copilot review comment 4422945671 on #1063: TestExecRereadRejectsRecycledAncestor fed the child exec with a zero StartTimeNs, so createdBeforeExec rejected the repair on its own and both subcases passed even with the ancestor-identity guards removed. The child now execs after the ancestor's /proc start time, so each subcase fails when only its guard is removed. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Matthias Bertschy --- pkg/processtree/creator/runc_parent_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/processtree/creator/runc_parent_test.go b/pkg/processtree/creator/runc_parent_test.go index fc4775a7a8..c759a51a6a 100644 --- a/pkg/processtree/creator/runc_parent_test.go +++ b/pkg/processtree/creator/runc_parent_test.go @@ -114,7 +114,9 @@ func TestExecRereadRejectsRecycledAncestor(t *testing.T) { c.pidStartTimeNs[execPID] = tc.recorded } table[execPID] = [2]uint64{uint64(shim), tc.readStart} - c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execChildPID, PPID: execPID, ContainerID: ancestryContainerID}) + // The child execs after the ancestor's /proc start time, so only the + // identity guard under test can reject the repair. + c.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: execChildPID, PPID: execPID, ContainerID: ancestryContainerID, StartTimeNs: uint64(testWallOffsetNs) + tc.readStart + 1_000_000_000}) require.Equal(t, runcPID, c.processMap.Get(execPID).PPID) }) }