Switch host isolation eBPF probes to use skeleton headers#233
Switch host isolation eBPF probes to use skeleton headers#233stanek-michal merged 3 commits intomainfrom
Conversation
Skeleton headers are already used by other non-HostIsolation related probes. Migrate the two host isolation probes so that we do not have to ship eBPF binaries separately and rely on the binaries being at a fixed path. Note that the eBPF maps used by the probes are still pinned at our bpf sysfs path. Another change in this patch is adding a prefix "el-endpo_" to TCA_BPF_NAME of the tc filter. This will help us identify our own tc filter on the system without having to store state (after restart, etc).
Require the network interface name to be provided on the command line
nicholasberlin
left a comment
There was a problem hiding this comment.
LGTM, there are formatting issues though.
Here's what my bot thinks. None of the suggested changes look necessary to me, as it suggests.
PR Review: #233 — Switch host isolation eBPF probes to use skeleton headers
Author: stanek-michal
Branch: pr11704-modern-skeleton-ebpf -> main
Commits: 2 | Files changed: 7 | +108 -120
URL: #233
Summary
This PR migrates the Host Isolation eBPF probes (KprobeConnectHook and TcFilter) from runtime .o file loading to compile-time skeleton headers, aligning them with the Events probes. It also parameterizes the TcLoaderDemo interface name (previously hardcoded to ens33) and changes the TCA_BPF_NAME to a structured format (el-endpo_<name>:[<id>]) for easier identification when enumerating tc probes. The changes are clean and well-structured.
Findings
Significant
-
Skeleton retry loop does not reset
ctxbetween attempts (KprobeConnectHookDemo.c)In
main(), the retry loop callstry_load_ebpf_kprobe(load_method, &ctx)repeatedly. On failure,try_load_ebpf_kprobedestroys the local skeleton and does not write to*ctx_out, soctxinmainstays at its initializedNULL— this is correct. However, the pattern is subtle and relies on an implicit contract that the function never modifies*ctx_outon failure. A defensive*ctx_out = NULLon the error path insidetry_load_ebpf_kprobewould make this contract explicit and guard against future regressions. -
bpf_obj_get_info_by_fdsetsrv = 0on success path, masking intent (TcLoader.c:489)After
rv = bpf_obj_get_info_by_fd(fd, &info, &info_len),rvis 0 on success. This is fine functionally, but later at line 518rv = 0is set again redundantly. The dual assignment isn't a bug, but it makes the success flow slightly harder to follow — the original pattern of initializingrv = -1and only settingrv = 0at the single exit point was cleaner. Consider assigning thebpf_obj_get_info_by_fdresult to a separate variable (or just checking< 0with an inline expression) to preserve the single-exit-pointrv = 0pattern used elsewhere in this file.
Minor
-
TCA_BPF_NAMEbuffer sizing is safe but tight for future changes (TcLoader.c:498)The format
"el-endpo_%s:[%u]"withinfo.name(max 15 chars fromBPF_OBJ_NAME_LEN) andinfo.id(max ~10 digits) fits comfortably in the 128-byte buffer (~38 bytes worst case). The truncation check is correctly in place. No action needed — just noting this is well-handled. -
Docs update correctly reflects the new CLI (hostisolation.md)
The documentation now shows
<iface>as a required argument and updates the unload command accordingly. Looks good.
Verdict
APPROVE — The migration to skeleton headers is straightforward, the removed ebpf_open_object_file and ebpf_load_and_attach_kprobe helpers are cleanly replaced by skeleton APIs, and the TcLoaderDemo interface parameterization is a nice usability improvement. The findings above are minor polish suggestions, not blockers.
Two main changes:
upside: no need to ship per-arch.o objects, no need to rely on fixed path with the objects