fix(processtree): re-read the parent from procfs for unknown host parents

docs/features/processtree-runc-parent-reread.md

@@ -0,0 +1,54 @@
+# 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/<pid>/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 (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. |
+
+## Cost and locking
+
+Each read is one `/proc/<pid>/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. 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, 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
+
+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.

pkg/processtree/creator/parent_reader.go

@@ -0,0 +1,53 @@
+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/<pid>/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 (
+		mu     sync.Mutex
+		fs     procfs.FS
+		ready  bool
+		warned bool
+	)
+	return func(pid uint32) (uint32, uint64, bool) {
+		mu.Lock()
+		if !ready {
+			f, err := procfs.NewDefaultFS()
+			if err != nil {
+				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, ready = f, true
+		}
+		mu.Unlock()
+		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
+	}
+}

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"
@@ -61,6 +62,20 @@ 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/<pid>/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 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
 }
 
 func NewProcessTreeCreator(containerTree containerprocesstree.ContainerProcessTree, config config.Config) ProcessTreeCreator {
@@ -79,7 +94,10 @@ func NewProcessTreeCreator(containerTree containerprocesstree.ContainerProcessTr
 		pidStartTimeNs:        make(map[uint32]uint64),
 		readStartTime:         newProcfsStartTimeReader(),
 		nowBootNs:             readBootTimeNs,
-		config:                config,
+		readParent:            newProcfsParentReader(),
+		// WallTimeFromBootTime(ts) is ts plus IG's offset for any non-zero ts.
+		bootToWallNs: int64(gadgets.WallTimeFromBootTime(1)) - 1,
+		config:       config,
 	}
 
 	return creator
@@ -216,6 +234,113 @@ 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, event conversion.ProcessEvent) {
+	containerID := event.ContainerID
+	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 || startNs == 0 {
+		return
+	}
+
+	// Pid-reuse guard: the /proc reading must describe the same incarnation the
+	// 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 && pt.pidStartTimeNs[anchor.PID] == 0 {
+		return
+	}
+	if !pt.createdBeforeExec(startNs, event.StartTimeNs) {
+		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
+	}
+	if _, exited := pt.pendingExits[ppid]; exited {
+		return
+	}
+	_, parentNs, ok := pt.readParent(ppid)
+	if !ok || parentNs == 0 || parentNs > startNs {
+		return
+	}
+	if known := pt.pidStartTimeNs[ppid]; known != 0 && known != parentNs {
+		return
+	}
+
+	pt.UpdatePPID(anchor, conversion.ProcessEvent{PID: anchor.PID, PPID: ppid, ContainerID: containerID})
+	if anchor.PPID != ppid {
+		return
+	}
+	anchor.Pcomm = parent.Comm
+}
+
+// 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
 // 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 +567,8 @@ func (pt *processTreeCreatorImpl) handleExecEvent(event conversion.ProcessEvent)
 	pt.UpdatePPID(proc, event)
 
 	if pt.config.KubernetesMode {
+		pt.rereadUnknownParent(proc, event)
+
 		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
@@ -460,7 +587,10 @@ 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. 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 != "" {