[2/2] Add CGROUP_SOCK_ADDR Logging Infrastructure

[yaakov-stein]

Stack:

• [0/2] Refactor Logging Infrastructure #485
• [1/2] Add flavor-specific log codegen callback #486
• -> [2/2] Add CGROUP_SOCK_ADDR Logging Infrastructure #491

Summary

This adds the remaining infrastructure to go from the hard-coded packet-specific logging to a flavor-specific logging approach that makes it easy to enable logging for CGROUP_SOCK_ADDR:

BEFORE                                  AFTER

program.c (hardcoded log emission)      program.c (callback dispatch)
──────────────────────────────────      ────────────────────────────────  
if rule->log:                           if rule->log:
  setup R1-R5 for packet headers          ops->gen_inline_log(program, rule)
  EMIT_FIXUP_ELFSTUB(PKT_LOG)

chain.c:                                Packet flavors (XDP, TC, NF, cgroup_skb)
  cgroup_sock_addr? → -ENOTSUP          ────────────────────────────────  
                                        bf_packet_gen_inline_log():
                                          R1=ctx, R2=rule_id, R3=log_opts,
                                          R4=verdict, R5=packed_protos
                                          → ELFSTUB_PKT_LOG

                                        cgroup_sock_addr.c
                                        ──────────────────────────────── 
                                        gen_inline_log():
                                          R1=ctx, R2=rule_id, R3=verdict,
                                          R4=packed_protos
                                          → ELFSTUB_SOCK_ADDR_LOG

We add a new sock_addr_log ELF stub, a gen_inline_log callback (see #486), CLI formatting for socket log entries, and per-hook log option validation (e.g. BF_LOG_OPT_LINK is rejected on socket hooks, BF_LOG_OPT_PID is rejected on packet hooks). It also introduces two new log options, BF_LOG_OPT_PID and BF_LOG_OPT_COMM, which use the bpf_get_current_pid_tgid() and bpf_get_current_comm() helper calls in the stub.

One of the challenges here is that the BPF verifier restricts which bpf_sock_addr context fields a program can access based on its attach type. For example, msg_src_ip4 is only valid for SENDMSG4 and user_ip4 is only valid for IPv4 hooks. The verifier checks all code paths statically regardless of runtime reachability, so a shared ELF stub compiled once and linked into all hook types cannot read these fields directly. To solve this, the per-hook inline codegen in the prologue copies the permitted fields into a new _bf_runtime_sock_addr struct on the BPF stack, and the stub copies from there into the log entry. The copying is done once per program invocation, guarded by the existing BF_CHAIN_LOG flag, so programs without logging rules pay zero cost.

Adding _bf_runtime_sock_addr to bf_runtime increases the stack frame for all flavors by 40 bytes (with alignment), but the struct remains well within the BPF 512-byte stack limit.

Testing

• Manual testing, example sendmsg4 log:

Note

• As mentioned above, the verifier doesn't allow accessing certain paths (ex. connect4 reading msg_src_ip6), so we'll need to add it to the stack frame, similar to how we do with packets. This leads to a new question - we really should have different bf_runtime for packet flavors and CGROUP_SOCK_ADDR given how different they are and the different data they use (can be done in a similar type of way we did it in bf_log, sharing common fields and then using a union). However, I don't think know is the right time to make this invasive change when a less invasive approach works, albeit in a less clean/proper way. Imo we can add this as a follow-up.

[github-actions]

Claude review of PR #491 (6cd0236)

Suggestions

• Missing e2e tests for other sock_addr hooks with logging — tests/e2e/hooks/cgroup_sock_addr_connect4.sh — only connect4 is tested with logging; connect6, sendmsg4, and sendmsg6 hook-specific codegen paths (IPv6 addresses, source address presence) are not exercised
• Add static_assert for bf_runtime size vs BPF stack limit — src/libbpfilter/cgen/runtime.h:153 — the new sock_addr field grows bf_runtime for all flavors; a compile-time guard against the 512-byte BPF stack limit would catch future regressions

Nits

• System include ordering — src/libbpfilter/include/bpfilter/runtime.h:26 — blank line between <linux/in6.h> and <asm/types.h> splits one system-header group into two
• "receive timestamp" wording — doc/usage/bfcli.rst:138 — for CGROUP_SOCK_ADDR hooks, "event timestamp" would be more accurate than "receive timestamp"

Workflow run

[github-actions]

Claude: must-fix: For IPv4 hooks the prologue writes only 4 bytes into ctx->sock_addr.saddr and ctx->sock_addr.daddr, but here sizeof(ctx->sock_addr.saddr) is 16. The __builtin_memcpy unconditionally copies all 16 bytes, so for IPv4 the remaining 12 bytes per address are uninitialized BPF stack data copied into the ring buffer entry.

For CONNECT4/CONNECT6 hooks, has_saddr is false so ctx->sock_addr.saddr is never written at all — the entire 16 bytes are uninitialized. This is either a BPF verifier rejection (if the verifier tracks the uninitialized read) or uninitialized stack data submitted to userspace via bpf_ringbuf_submit.

Consider either: (a) zero-initializing the sock_addr region in the prologue before writing the partial address, or (b) passing the address size to the stub so it copies only the correct number of bytes.

[yaakov-stein]

Currently, the packet logging does the same thing for headers that are not requested. This is not a verifier rejection in either case, but we do submit uninitialized stack data in both cases. As long as this doesn't become a verifier issue, I think it can be left as is given that both option add some amount of BPF instructions. Happy to make a change though if others think it makes sense.

[github-actions]

Claude: nit: This is the first <linux/...> include in any public header under include/bpfilter/. It is only needed for sizeof(struct in6_addr) in _bf_log_sock_addr.saddr and .daddr. The sibling struct _bf_runtime_sock_addr in cgen/runtime.h already uses the literal 16 for the same purpose. Using 16 here (with a comment referencing sizeof(struct in6_addr)) would avoid introducing a kernel UAPI header dependency into the public API surface.

[github-actions]

Claude: suggestion: The context field copies (dport, saddr, daddr) are guarded by BF_CHAIN_LOG, which is set if any rule in the chain has logging enabled. This means every connection through the chain pays the cost of the BPF loads and stores, even if the matched rule does not have logging or only requests pid,comm.

For chains where logging is rare (e.g., one log rule among many), consider computing the union of all rule->log options at chain creation time and only emitting copies for actually-needed fields. Not a correctness issue, but the number of instructions per invocation could be reduced.

[qdeslandes]

Few comments. Also, can you add a benchmark for logging from sock_addr?

[qdeslandes]

I don't think we need to discriminate logged data for socket hooks (c.f. the 1st PR in the stack).

[qdeslandes]

Both these paragraph skip over shared information in bf_log, e.g. the rule's verdict. Please document the logged info fully: shared data and hook-specific data.

[qdeslandes]

While I understand the issue here, this forces pre-fetching data that we might not use. Especially when it's done in the prologue, we'll pay the cost of it for rules that might not even be hit. Having a dedicated elfstub for each sock_addr hook would be better than prefetching this data.