Repository navigation
fix(http): preserve verifier-visible bounds for iovec reads - #1012
Conversation
Keep aggregate offsets and read sizes in 64-bit scalars and preserve explicit per-read and remaining-capacity checks across LLVM optimization. Add native bounds assertions and a privileged live TCP capture regression runner. Refs kubescape#1009 Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 6.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 5 files. (2 skipped: 2… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: preserving verifier-visible bounds for iovec reads in the HTTP gadget. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 6.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 5 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Verdict: needs clarification before approval for d448cce61d763ed4bccd537d15fb8c1248bed166, against main at 67e455e040520a3f1f5a4ac20fbf5fe25f676d43. No confirmed production-code blockers found.
The fix is needed: #1009 documents rejection of the exact released HTTP ELF on COS 121 / Linux 6.6.157+. The problematic narrowed-sum sequence remains on the target branch. Disabling HTTP tracing sacrifices coverage rather than fixing it.
The implementation appears sound. At program.bpf.c:611–627, the barrier retains direct 64-bit checks on the final helper size after both minima. My Clang 18.1.8 amd64/arm64 builds retain if r8 > 16384 and if r8 > r2 before passing that size to helper 112. Together with buffered < 16384, this supplies independent ranges within the unchanged 32 KiB scratch map; size <= capacity preserves the logical 16 KiB limit. Capture ABI, 256 KiB budget, descriptor bound, reservation cleanup and continuation handling remain unchanged. No new dependencies or unrelated production changes.
Remaining evidence gap: could you provide baseline rejection and patched load/capture results on COS 121 build 18867.624.2, kernel 6.6.157+, retaining ELF identity and verifier logs, then repeat the patched run for reattachment as described in tests/README.md:21–28? The PR explicitly reports this pending. Other-kernel success and native helper substitutes cannot establish that the reported failure is resolved.
Validation independently performed in credential-free, network-isolated sandboxes:
- Go 1.27:
go test -v -count=1 ./pkg/ebpf/gadgets/http/...,go test -race -count=1 ./pkg/ebpf/gadgets/http/..., andgo vet ./pkg/ebpf/gadgets/http/...passed with the C compiler present, without test skips. - The current tests also pass with target-branch production C substituted: useful behavior preservation, but no failing-before/passing-after kernel regression.
- Base and head BPF compile for amd64/arm64 with IG build flags (
-target bpf -Wall -g -O2, architecture headers/macros); patched generated assembly retains both checks. This was a compiler check, not a kernel load or reproduction of the released ELF digest. - Diff whitespace, changed Go formatting and Python syntax checks passed. All reported head CI checks succeeded, including cross-platform build and component tests. CodeRabbit's successful status accompanies a rate-limit comment, not a completed review. Full local repository/agent builds and privileged live scenarios were not run; this host is Linux
7.1.13+, not the affected COS kernel.
History: merged #992 introduced coalescing; #990 introduced bounded chunks, and #991 repaired continuation handling. They are predecessors, not superseding fixes. #991's counter-contention objection was addressed with per-CPU counters and does not recur here. Searches across open/closed/merged PRs and issues used HTTP/verifier/BPF/COS/6.6, iovec/coalescing, scratch-map/read-size terminology, emit_iov_step, affected filenames and #1009, capped at 100 results/query; the broad HTTP query reached that cap. No superseding fix surfaced; this is not an exhaustive proof. Closed #724 concerns a different TLS gadget, with no explicit maintainer rejection of this bounds approach; closed #802 was explicitly superseded by #818 for telemetry work, unrelated to this fix.
Independent correctness review: COMMENT; architecture: WATCH for target-kernel validation. Existing #1012 discussion had only the rate-limit comment and no reviews/inline findings. PR is open, non-draft and mergeable, with head/base unchanged on final recheck. Branch protection requires an approving review including last-push approval; I am withholding approval until the material validation gap is resolved.
Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Rechecked 454f49ba3a9ff25c4029581c61f04785f043530b against main at 493f1011f11be74d3b46477516eaaceb02de8871. Verdict: needs clarification before approval. No confirmed production-code blockers. Independent correctness review: COMMENT; architecture: WATCH.
The new COS report materially advances the earlier request: it records rejection of the exact baseline ELF from #1009 and two successful 42-scenario patched capture runs on the vendor COS 121 / 6.6.157+ kernel. The custom VM boot differences are clearly disclosed. The bounds repair remains appropriate: explicit 64-bit helper-size checks after both minima, with an optimizer barrier, preserve the existing map, event ABI and capture limits.
Two evidence items remain before approval:
- Please publish the evidence archive and its checksum. The report currently points to an author-local path (lines 118–123), so reviewers cannot inspect the verifier logs, ELF identities or raw capture results.
- Please rebuild the latest source and record COS load/capture plus detach/reattachment with its ELF hash. Lines 103–116 explicitly say the COS runs predate the upstream merge. I confirmed the only subsequent production HTTP change is the upstream local-port byte-order correction, and the bounds fix is unchanged; this is an artifact-validation gap, not a demonstrated regression. The fixture replay helper boots the archived gadget rather than rebuilding the current checkout.
Fresh independent validation in a credential-free, network-isolated bubblewrap sandbox with Go 1.27.0 and native C compiler: go test -v -mod=readonly -count=1 ./pkg/ebpf/gadgets/http/... passed with no skips; HTTP race tests and go vet -mod=readonly ./pkg/ebpf/gadgets/http/... passed. git diff --check passed. I did not independently run COS/KVM, privileged live capture, BPF compilation, the full repository suite or agent build this time. Native tests establish behavior preservation, not kernel verifier acceptance.
History remains consistent with the previous review: merged #990/#991/#992 are predecessors, not replacements. #991's counter-contention objection was addressed and does not recur here. Renewed all-state searches using HTTP/verifier, iovec, scratch, COS, 6.6, BPF verifier and the production filename found no superseding fix; queries were capped at 100 results, so this is not proof of absence. Closed #724 concerns a different TLS gadget, not an explicit rejection of this approach.
Immediately before submission, the PR was open, non-draft and mergeable with the reviewed head and target unchanged; no new reviews or inline findings required reconciliation. Cross-platform build, component-image build and benchmark CI remain in progress; completed checks show no failure. I am withholding approval for the evidence gaps above, rather than requesting code changes. No source changes or merge actions were made.
|
@matthyx Thanks for the recheck. Both evidence gaps are now addressed for PR head 1. Public evidence archive and checksum Archive SHA-256: The archive contains complete verifier diagnostics, raw capture events, exit statuses, ELF files and hashes, OCI identities, build/test logs, VM configuration and replay inputs. Anonymous downloads and both archive and extracted-file checksums were verified. 2. Fresh build and COS load/capture plus reattachment Rebuilt the current PR source using IG OCI index: The exported OCI ELF layers were verified byte-for-byte against the freshly built objects. On the local VM running vendor COS 121 build
The complete baseline verifier diagnostic is retained, including a decoded 5,416-line trace. The rebuilt gadget bundle, full Go The previously disclosed custom-initramfs and writable-overlay boot differences remain unchanged. These results establish acceptance on the exact vendor COS kernel; they do not claim normal GCE startup or full host-agent/backend integration. No production-code changes were needed. |
|
@matthyx can you please review and give feedback on this PR |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Rechecked head 454f49ba3a9ff25c4029581c61f04785f043530b against main at 493f1011f11be74d3b46477516eaaceb02de8871. APPROVE — no remaining code blockers. Independent correctness review: APPROVE; architecture: CLEAR.
The two evidence gaps from my previous review are resolved by the author’s current-head validation. I downloaded the public archive, verified its SHA-256 (bece18d4…f16758) and all per-file checksums, and matched its production C and executable test/helper sources to this head. The archive’s two Markdown reports contain newer documentation, explicitly separate from the checked-in versions.
Raw evidence corroborates the exact baseline ELF from #1009 rejecting the 262144-byte helper read into the 32768-byte map. Fresh patched ELF afd55eb6…dc0f27 loads on vendor COS 121 / 6.6.157+; initial and separate reattachment invocations each record exit 0 and 42/42 capture scenarios. Both architecture disassemblies retain size/capacity checks before helper 112. This establishes the need and directly exercises the repaired verifier boundary while preserving capture limits, map size and event ABI.
Fresh independent execution: compiled the production native C harness with cc -O2 and passed all 48 scalar/vector boundary cases in a credential-free, network-isolated bubblewrap sandbox. These checks preserve behavior; they do not reproduce kernel verification. Reviewed supplied full-suite/build/race/vet logs, but did not rerun those commands or privileged COS/KVM validation this time. All 39 reported CI checks/contexts succeeded. COS evidence uses a custom-initramfs/overlay VM and standalone IG, not full deployed-agent/backend integration; arm64 was compiled/inspected, not kernel-loaded.
History remains as previously reviewed: merged #990, #991, and #992 are predecessors. #991’s counter objection is unrelated to this scalar-bounds change and was addressed. Renewed all-state iovec and scratch/verifier PR searches, open HTTP/verifier search and COS issue search found no replacement; queries were limited to 100 results and are not proof of absence.
Nonblocking documentation suggestion: refresh tests/validation-cos121.md:99-123 and the PR description to link the public current-head report; they still describe the older evidence gap.
Immediately before submission: open, non-draft, mergeable; head and target unchanged, no new review/inline findings. Branch rules require approval, with no required status contexts; visible checks are green. No merge or source changes performed.
2 participants
Overview
The HTTP gadget bundled in v0.3.251 fails BPF verification on COS 121 / Linux 6.6.157. The verifier sees a possible 256 KiB user-memory read into the 32 KiB aggregation scratch map, preventing HTTP tracing from starting.
This change keeps aggregation offsets and read sizes in unsigned 64-bit scalars and uses a register barrier to preserve explicit bounds checks across LLVM optimization. Before the read helper runs, the final size must fit both the 16 KiB chunk limit and the remaining aggregate capacity.
The scratch-map size, event ABI, and 256 KiB syscall capture limit remain unchanged.
Additional Information
The baseline build reproduces the exact amd64 ELF digest reported in #1009. Patched amd64 and arm64 builds retain the explicit bounds checks in their generated instructions.
Local validation passed on Fedora / Linux 7.2.8. Exact COS 121 / Linux 6.6.157 validation remains pending because the affected host was unavailable.
How to Test
Run the native HTTP regression tests with Go 1.27:
On a disposable Linux host with root access, Docker, Python 3, and Inspektor Gadget v0.48.1:
The runner builds current source, requires an actual HTTP event after attachment, and verifies 42 live TCP capture scenarios across sendmsg, recvmsg, writev, and readv. Coverage includes chunk boundaries, split and empty vectors, partial receives, and the syscall capture limit.
Completed local validation:
Related issues/PRs
Refs #1009
Checklist before requesting a review
Summary by CodeRabbit