[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 (3ca48eb)

Suggestions

• Uninitialized local variables in prologue — src/libbpfilter/cgen/cgroup_sock_addr.c:175 — saddr_off and daddr_off declared without initialization may trigger -Wmaybe-uninitialized despite being structurally safe
• ABI break in bf_log layout undocumented — src/libbpfilter/include/bpfilter/runtime.h:178 — struct bf_log layout changed significantly (fields moved into tagged union, bf_pkthdr renamed); worth noting the intentional break in the commit description
• Prologue pre-reads all fields unconditionally — src/libbpfilter/cgen/cgroup_sock_addr.c:180 — Context field copies (dport, saddr, daddr) execute whenever any rule has logging, even for rules requesting only pid,comm
• Struct naming diverges from convention — src/libbpfilter/include/bpfilter/runtime.h:110 — _bf_log_pkt, _bf_log_sock_addr, and _bf_runtime_sock_addr use leading-underscore for struct names, but project convention reserves this for static functions/variables
• E2E logging tests only cover connect4 — tests/e2e/hooks/cgroup_sock_addr_connect4.sh — No logging tests for connect6, sendmsg4, or sendmsg6; the has_saddr code path is untested

Nits

• Duplicate register setup code — src/libbpfilter/cgen/packet.c:367 — bf_packet_gen_inline_log and _bf_cgroup_sock_addr_gen_inline_log share identical R1-R5 setup, differing only in the ELF stub ID
• Kernel UAPI include in public header — src/libbpfilter/include/bpfilter/runtime.h:15 — first <linux/...> include in any include/bpfilter/ header, added solely for sizeof(struct in6_addr); the literal 16 is already used in the sibling struct
• Error message missing offending bits — src/libbpfilter/chain.c:144 — "invalid log options for %s" doesn't indicate which options are invalid; including the bitmask would improve debuggability
• Hardcoded %.16s for comm buffer — src/bfcli/print.c:664 — Format width matches BF_COMM_LEN but uses a literal; consider the symbolic constant via %.*s

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.

[github-actions]

Claude: suggestion: struct _bf_log_pkt, struct _bf_log_sock_addr (here in runtime.h), and struct _bf_runtime_sock_addr (in cgen/runtime.h) use a leading-underscore prefix for struct names. The project style guide reserves leading underscores for static functions and variables (e.g., _bf_ctx_free()), and all existing struct types in the codebase use the plain bf_ prefix (e.g., struct bf_chain, struct bf_runtime, struct bf_log).

Consider struct bf_log_pkt, struct bf_log_sock_addr, and struct bf_runtime_sock_addr for consistency.

[github-actions]

Claude: nit: The format specifier %.16s hardcodes the comm buffer size. BF_COMM_LEN is defined to 16 in this PR and could be used here via %.*s to stay in sync if the buffer size ever changes:

(void)fprintf(stdout, "  Process   : %s%.*s%s\n",
              bf_logger_get_color(BF_COLOR_LIGHT_GREEN, BF_STYLE_BOLD),
              BF_COMM_LEN, log->payload.sock_addr.comm,
              bf_logger_get_color(BF_COLOR_RESET, BF_STYLE_RESET));