fix(libpman): only append errno to error logs - #3070
Open
aroberts87 wants to merge 1 commit into
Open
Conversation
libbpf_probe_bpf_helper() and libbpf_find_vmlinux_btf_id() signal their result via the return value but leave errno set (e.g. EACCES from the verifier rejecting the probe program). log_msg_v() appended any non-zero errno to every message, so debug success lines wrongly showed "(errno: 13 | message: Permission denied)". Gate the errno append on FALCOSECURITY_LOG_SEV_ERROR, where errno is meaningful, instead of clearing errno at each probe call site. Also set errno=ENXIO before its log_errorf so the error line carries the right code. Add a libpman_log unit suite covering the gate: stale errno is not appended to debug/info/warning lines, error lines still carry it, and no bogus errno is appended when errno is 0. Signed-off-by: Adam Roberts <adam.roberts@sysdig.com>
poiana
added
release-note
kind/bug
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aroberts87 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3070 +/- ##
==========================================
+ Coverage 75.94% 76.27% +0.32%
==========================================
Files 300 300
Lines 33276 33397 +121
Branches 5057 5032 -25
==========================================
+ Hits 25271 25473 +202
+ Misses 8005 7924 -81
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
Hi @aroberts87, I think your PR is a step towards the right direction and I agree this should be fixed. I was checking the cases where |
Contributor
|
That certainly is an approach :D Personally, I'd prefer passing errno explicitly where we want it, and zero where we don't (regardless of log level), but I see that would require more extensive surgery (changing the signature of the pman log callback). Still, I'm not going to block it so I'm happy to ✔️ if you want to leave it as is. |
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
4 participants
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.
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area libpman
/area libscap-engine-modern-bpf
/area tests
What this PR does / why we need it:
libpman'slog_msg_v()appends the currenterrnoto every log message it formats, regardless of severity. But several libpman code paths run libbpf feature probes right before logging a non-error line:libbpf_probe_bpf_helper()inpman_prepare_progs_before_loading()libbpf_find_vmlinux_btf_id()inis_kernel_symbol_available()These probes signal their result through the return value and leave a stale
errnobehind (e.g.EACCESwhen the verifier rejects the probe program,ENOENTfor a missing kernel symbol). The subsequentFALCOSECURITY_LOG_SEV_DEBUG"satisfied required feature" success line then wrongly rendered as:which is confusing: nothing failed, the feature is supported.
This PR fixes the leak at its root instead of clearing
errnoat each probe call site:log_msg_v()now appends theerrnodetail only forFALCOSECURITY_LOG_SEV_ERROR, whereerrnois meaningful. Lower severities (debug/info/warning) report their result via return values, noterrno.errno = ENXIOinpman_prepare_progs_before_loading()is now set before itslog_errorf(), so the error line carries the right code.Special notes for your reviewer:
Adds a
libpman_logunit suite (test/libscap/test_suites/engines/modern_bpf/libpman_log.cpp) that exercises the gate directly, without root or a loaded BPF probe: it installs a capturing log callback viapman_init_state()and asserts that a staleerrnois not appended to debug/info/warning lines, that error lines still carry it, and that no boguserrnois appended whenerrnois 0. Reverting the gate turns the debug/info/warning assertions red while the error-log assertions stay green.The bug only reproduces on a kernel where a feature probe actually fails (leaving
EACCES/ENOENTinerrno); on a fully-supported kernel every probe passes clean, so the unit suite — which injectserrnodeterministically — is the reliable regression guard.Does this PR introduce a user-facing change?: