symbolize: require CAP_CHECKPOINT_RESTORE instead of degrading around it - #41
Merged
Merged
Conversation
blazesym's process source reaches the file behind every mapping through /proc/<pid>/map_files/, and the kernel's proc_map_files_get_link() refuses to follow those magic symlinks without CAP_CHECKPOINT_RESTORE (or CAP_SYS_ADMIN). To tolerate a test binary setcap'd with only cap_bpf,cap_perfmon, symbolize/local.go had grown a permanent second resolution path and the machinery to steer it: a process-lifetime latch, a compare-and-swap so the log fired once, a startup CapEff probe, five LocalStats fields, a test seam to inject the two failure kinds, and a string match on blazesym's "permission denied" text because its C API drops the errno. None of that bought anything a supported deployment needs. CAP_CHECKPOINT_RESTORE is already in the required set documented by README.md, SECURITY.md and perfagent/agent.go, and unwind/procmap, unwind/dwarfagent and symbolize/debuginfod already depend on it directly. So: map_files is the single path, and NewLocalSymbolizer refuses rather than degrades. It probes once by open()ing a real map_files entry for itself, which exercises the exact kernel gate (not a CapEff bitmask, which is wrong about non-initial user namespaces and about Permitted-not-Effective) and yields a typed errno, which is what removes the dependency on blazesym's error text. Only a definite EPERM is a verdict; an inconclusive probe never blocks startup. The error names the missing capability and the setcap line. A log plus a Stats flag was the alternative, and it is silent degradation with extra steps: no caller in this repository reads LocalStats, and a startup log is long gone by the time a sixty-second run writes a profile.pb.gz full of 0x7f addresses. A profile whose every user frame is a bare address is not degraded, it is useless. LocalStats.RawAddrBatches stays: a pid that exits before its /proc entry can be read is a genuine per-process failure and real signal. That path still returns hex frames with Reason == FailureMissingSymbols and err == nil, which gpuprobe.Stats.StacksUnresolved consumes. gpuprobe's gate now requires CAP_CHECKPOINT_RESTORE too, so a run that would produce hex frames skips with a clear reason instead of passing green - which is exactly what it did while 63 sampled stacks resolved to nothing. CAP_SYS_ADMIN is untouched; the uprobe_multi attach path stays as it is. symbolize/local.go 357 -> 241 lines; 414 deletions against 245 insertions across the branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
169 lines removed.
symbolize/local.gogoes 357 → 241.What this deletes, and why it existed
blazesym resolves mappings through
/proc/<pid>/map_files/, whose magic symlinks the kernel refuses withoutCAP_CHECKPOINT_RESTORE. When a test binary was setcap'd with onlycap_bpf,cap_perfmon, symbolization silently resolved nothing — every user frame came back as a bare hex address while reporting zero failures.The fix at the time added a fallback path: retry with
no_map_files, latch that setting, log once via compare-and-swap, probe capabilities at startup, and detect permission errors by string-matching blazesym's message because the C API drops errno. Plus fiveLocalStatsfields to manage it.All of that existed to tolerate a capability configuration this project does not support.
CAP_CHECKPOINT_RESTOREis already in the documented required set (CLAUDE.md, spec §11, which states it covers/proc/<pid>/map_files), and three other code paths already depend on it directly —unwind/procmap/parse.go,unwind/dwarfagent/miss_drainer.go,symbolize/debuginfod/buildid.go.A test-harness constraint had been absorbed into the product.
Gone: the latch, the CAS log, the startup capability probe, the string match, the
no_map_filesretry, the test seam, and five stats fields. Kept:RawAddrBatches, which reports genuine per-process resolution failures — real signal.Failing loudly instead
NewLocalSymbolizernow refuses with a typed error rather than degrading:A hard error rather than a logged flag, because nothing in the repo read
LocalStats— a log plus a flag would have been silent degradation with extra steps. Detection is an actualopen()of a realmap_filesentry: the exact kernel gate, a typed errno, immune to user-namespace quirks and to Permitted-not-yet-Effective. Only a definiteEPERMis treated as a verdict.Fixing the cause, not just the symptom
Test provisioning is corrected so the same pressure does not return: the gate's capability check and every documented
setcapline now requirecap_bpf,cap_perfmon,cap_checkpoint_restore— the set the product actually needs. A run that would emit hex frames now skips with a reason rather than passing green with degraded output.For the phase gate, a human runs:
Note on
CAP_SYS_ADMINNot used, and not needed. It grants
map_filestoo, butCAP_CHECKPOINT_RESTOREis narrower and already required. The one placeCAP_SYS_ADMINwould apply —perf_uprobeinstead ofuprobe_multi— costs more code (one link per probe rather than one link with cookies) and buys only the removal of the Linux 6.6 floor. That is a deployment-reach decision, not a simplification.Verification
go build,go vet, all listed packages,-race,golangci-lint0 issues. Three pre-existinggofmtoffenders (symbolize/hist.go,symbolize/debuginfod/stats.go, and a test helper) are untouched by this branch — a go1.26 alignment change already onmain.Also spotted and deliberately left alone:
perfagent/agent.go:327callscaps.SetFlagwithout a followingcaps.SetProc(), so that raise is a no-op. Harmless for+epbinaries, unrelated to this change.mainis currently red onTestKernelStackResolution(no user-side function in profile), which predates this branch and touches none of this code — reruns in flight to confirm flakiness.