amadan
cgroup: say the 4 s placement-check bound assumes a prompt SIGKILL (#272)
Whole-branch review round 1, finding 4, the smallest fix: documentation. The shim's deadlines and VerifyPlacement's budget bound the protocol's waits. The reap of a SIGKILLed shim or probe (reap's Process.Wait, the status read to EOF after a kill, awaitProbe's wait) is outside them and ends only when the kernel ends the process, which a process in uninterruptible sleep delays. The comments now say so where the bound is claimed, and name what an unconditional bound would need.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+42
−17
internal/cgroup/shim_linux.go+22 −6internal/cgroup/verify_linux.go+20 −11
diff --git a/internal/cgroup/shim_linux.go b/internal/cgroup/shim_linux.go| index 928b14d82..77da9eb6f 100644 |
| --- a/internal/cgroup/shim_linux.go |
| +++ b/internal/cgroup/shim_linux.go |
| @@ -52,8 +52,10 @@ func (m *Manager) startViaShim(dir string, cmd *exec.Cmd, before func() error) ( |
| // startViaShimBy is startViaShim with neither of its two waits running past |
| // by. The zero time caps nothing: each wait is shimDeadline, as for every |
| // tenant start (Ruling 9). Only VerifyPlacement sets by, so the arm-time |
| -// check fits its budget whatever the shim does; past by, a start fails and |
| -// is reaped exactly as one past shimDeadline is. |
| +// check fits its budget whatever the shim does in user space; past by, a |
| +// start fails and is reaped exactly as one past shimDeadline is. The reap |
| +// of a SIGKILLed shim is outside both waits and assumes the kernel ends a |
| +// killed process promptly (reap). |
| func (m *Manager) startViaShimBy(dir string, cmd *exec.Cmd, before func() error, by time.Time) (refused bool, err error) { |
| // The hook itself, not shimAvailable: FakePlacementForTest's override |
| // must never send a binary without the hook to start itself as a shim. |
| @@ -271,6 +273,9 @@ func (m *Manager) startShimCore(dir string, dirFile *os.File, expected string, c |
| reap(cmd) |
| return false, err |
| } |
| + // EOF comes when the killed shim's status fd closes, which is when |
| + // the kernel has ended it: unbounded by the deadline, under reap's |
| + // assumption. |
| _ = cmd.Process.Kill() |
| _ = statusR.SetReadDeadline(time.Time{}) |
| more, _ := io.ReadAll(br) |
| @@ -285,10 +290,21 @@ func (m *Manager) startShimCore(dir string, dirFile *os.File, expected string, c |
| return false, err |
| } |
| -// reap ends a failed start's shim: kill, then wait, which reaps the PID and |
| -// is bounded because no tenant ran to hold the child's pipes open. exec.Cmd's |
| -// copying goroutines and pipes are released in the background, so the |
| -// caller's I/O never holds this call. |
| +// reap ends a failed start's shim: kill, then wait, which reaps the PID. |
| +// exec.Cmd's copying goroutines and pipes are released in the background, |
| +// so the caller's I/O never holds this call, and no tenant ran to hold the |
| +// child's pipes open. |
| +// |
| +// The wait is bounded only by the kernel finishing the SIGKILL. SIGKILL is |
| +// not proof of termination: a process in uninterruptible sleep (a stuck |
| +// cgroupfs write, a hung disk or network filesystem read of /proc/self/exe) |
| +// dies only when that sleep ends, and this call, and so the start's |
| +// answer, waits that long. Normally that is microseconds. Neither |
| +// shimDeadline nor VerifyPlacement's budget covers it: they bound the |
| +// protocol's waits, on the assumption that a killed process ends promptly |
| +// (whole-branch review, finding 4). Bounding the reap too would need the |
| +// start to return while still owning an unreaped shim and its in-flight |
| +// registration, and to report the target unproven until the reap is done. |
| func reap(cmd *exec.Cmd) { |
| _ = cmd.Process.Kill() |
| _, _ = cmd.Process.Wait() |
diff --git a/internal/cgroup/verify_linux.go b/internal/cgroup/verify_linux.go| index 6f11d5d04..5259daf63 100644 |
| --- a/internal/cgroup/verify_linux.go |
| +++ b/internal/cgroup/verify_linux.go |
| @@ -27,12 +27,17 @@ const placementCheckName = "carlos-placement-check" |
| // change it, to stand in a probe that misreports or never exits. |
| var placementProbeArgs = []string{ProbeVerb} |
| -// placementCheckBudget bounds the whole of VerifyPlacement, the shim's two |
| -// waits and the probe's run included, so a decline is decided in time for |
| -// the restarted edge to answer `carlos system update`'s 20 s gate |
| -// (systemGateWait) with the rest of its boot work. What runs after the |
| -// budget is the reap of a SIGKILLed shim or probe and an rmdir: milliseconds, |
| -// which the second of headroom under 5 s covers. |
| +// placementCheckBudget bounds VerifyPlacement's waits, the shim's two and |
| +// the probe's run, so a decline is decided in time for the restarted edge |
| +// to answer `carlos system update`'s 20 s gate (systemGateWait) with the |
| +// rest of its boot work. What runs after the budget is the reap of a |
| +// SIGKILLed shim or probe and an rmdir. That is milliseconds on the |
| +// assumption that the kernel ends a killed process promptly, which the |
| +// second of headroom under 5 s covers. It is an assumption, not a bound: |
| +// SIGKILL does not end a process in uninterruptible sleep (stuck kernel |
| +// I/O) until that sleep ends, and the reap waits for it (reap, in |
| +// shim_linux.go; awaitProbe). A box stalled that way can take longer than |
| +// 4 s to decline (whole-branch review, finding 4). |
| const placementCheckBudget = 4 * time.Second |
| // VerifyPlacement does one real placement, through the shim, into a scratch |
| @@ -43,10 +48,12 @@ const placementCheckBudget = 4 * time.Second |
| // because no placement was possible under carlos-edge.service's hardening. |
| // |
| // It calls the shim itself (startViaShimBy), never the placeStart seam, so a |
| -// test's FakePlacementForTest cannot make it pass. It ends at ctx's deadline |
| -// or after placementCheckBudget, whichever is sooner; the shim's waits are |
| -// capped at the same instant. The scratch cgroup is removed once the probe |
| -// is reaped; a removal failure is logged, not returned. |
| +// test's FakePlacementForTest cannot make it pass. Its waits end at ctx's |
| +// deadline or after placementCheckBudget, whichever is sooner; the shim's |
| +// waits are capped at the same instant. The reap of a process it killed |
| +// then follows, outside that budget (placementCheckBudget says why). The |
| +// scratch cgroup is removed once the probe is reaped; a removal failure is |
| +// logged, not returned. |
| func (m *Manager) VerifyPlacement(ctx context.Context) error { |
| if m == nil { |
| return ErrNoDelegation |
| @@ -126,7 +133,9 @@ func awaitProbe(ctx context.Context, cmd *exec.Cmd) error { |
| case werr = <-waited: |
| case <-ctx.Done(): |
| _ = cmd.Process.Kill() |
| - werr = <-waited // bounded: the probe is dead and holds no other process |
| + // Bounded once the kernel has ended the killed probe, which holds no |
| + // other process; not by ctx (placementCheckBudget). |
| + werr = <-waited |
| var ee *exec.ExitError |
| if errors.As(werr, &ee) { |
| if ws, ok := ee.Sys().(syscall.WaitStatus); ok && ws.Signaled() && ws.Signal() == syscall.SIGKILL { |