Skip to content

[1/2][CGROUP_SOCK_ADDR Logging] Add flavor-specific log codegen callback#486

Open
yaakov-stein wants to merge 5 commits intofacebook:mainfrom
yaakov-stein:extract-log-codegen-callback
Open

[1/2][CGROUP_SOCK_ADDR Logging] Add flavor-specific log codegen callback#486
yaakov-stein wants to merge 5 commits intofacebook:mainfrom
yaakov-stein:extract-log-codegen-callback

Conversation

@yaakov-stein
Copy link
Copy Markdown
Contributor

Relies on #485 - gave stack-pr a try, but it doesn't allow for multi-commit PR's :(

Summary

Move log codegen into a per-flavor callback so each flavor controls its own register setup and ELF stub selection:

  • Move cgen/matcher/packet.{c,h} to cgen/packet.{c,h}. It was a mistake to put this here in the first place as packet-based flavors can share more than matcher codegen. We correct the placement now.
  • Add gen_inline_log to bf_flavor_ops, with bf_packet_gen_inline_log() as the shared implementation for all packet flavors.

@meta-cla meta-cla bot added the cla signed label Mar 20, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 20, 2026

Claude review of PR #486 (e014ec7)

Claude is re-reviewing this PR… (workflow run)

Must fix

  • assert(str) in bf_log_opt_from_str will crash unit testssrc/libbpfilter/rule.c:39 — The old bf_pkthdr_from_str did not assert on str, but the new code adds assert(str). The test harness assert_enum_to_from_str at tests/harness/test.h:54 explicitly calls from_str(NULL, &back) to test graceful NULL handling. This assert will abort in debug builds during unit tests

Suggestions

  • Missing guard for gen_inline_log dispatchsrc/libbpfilter/cgen/program.c:295 — Fixed: assert(program->runtime.ops->gen_inline_log) added at function entry
  • Missing bf_log_type string mappingsrc/libbpfilter/include/bpfilter/runtime.h:91 — Fixed: _bf_log_type_strs[], static_assert_enum_mapping, and bf_log_type_{to,from}_str now present in rule.c
  • <linux/in6.h> include widens BPF stub dependenciessrc/libbpfilter/include/bpfilter/runtime.h:15runtime.h is included by all BPF stubs. Adding <linux/in6.h> for sizeof(struct in6_addr) in the sock_addr variant pulls a kernel header into every stub's compilation. Consider using __u8 saddr[16] / __u8 daddr[16] instead
  • req_headers 4-bit bitfield inconsistent with 8-option limitsrc/libbpfilter/include/bpfilter/runtime.h:131 — Fixed: req_headers and headers are now full __u8 fields
  • cgroup_sock_addr log stub errors at codegen timesrc/libbpfilter/cgen/cgroup_sock_addr.c:284 — The stub returns -ENOTSUP during program generation. Chain validation already rejects this combination, so this is defense-in-depth
  • Missing unit tests for bf_log_type_* functionstests/unit/libbpfilter/rule.c:20 — Fixed: tests now present
  • Add static_assert for struct bf_log sizesrc/libbpfilter/include/bpfilter/runtime.h:167 — Both BPF stubs and userspace readers must agree on the struct layout. A size assertion would catch unintentional layout drift

Nits

  • Doxygen refers to sock variantsrc/libbpfilter/include/bpfilter/runtime.h:96 — Fixed: now says sock_addr
  • gen_inline_log required/optional not documentedsrc/libbpfilter/include/bpfilter/flavor.h:143 — Fixed: now says "Required for all flavors."
  • static_assert relaxed from < 8 to <= 8src/libbpfilter/include/bpfilter/rule.h:75 — The original _BF_PKTHDR_MAX < 8 rejects exactly 8 values; _BF_LOG_OPT_MAX <= 8 permits them. Both are correct for a uint8_t bitmask, but the semantic change should be intentional
  • bfc_print_log silently drops non-packet logssrc/bfcli/print.c:577 — When log->log_type is not BF_LOG_TYPE_PACKET, the function returns silently. Since this is part 1/2, consider a bf_dbg() trace or comment noting sock_addr printing is deferred

Workflow run

@yaakov-stein yaakov-stein changed the title [1/2][`CGROUP_SOCK_ADDR Logging] Extract log codegen callback [1/2][CGROUP_SOCK_ADDR Logging] Extract log codegen callback Mar 20, 2026
@yaakov-stein yaakov-stein changed the title [1/2][CGROUP_SOCK_ADDR Logging] Extract log codegen callback [1/2][CGROUP_SOCK_ADDR Logging] Add flavor-specific log codegen callback Mar 20, 2026
Rename the packet logging ELF stub from `log` to `pkt_log` to
establish naming symmetry with the upcoming `sock_log` stub for
socket-based hooks.
@yaakov-stein yaakov-stein force-pushed the extract-log-codegen-callback branch from 86913cd to 219c17e Compare March 27, 2026 19:10
@yaakov-stein yaakov-stein force-pushed the extract-log-codegen-callback branch from 219c17e to 527d9cd Compare March 27, 2026 19:33
@yaakov-stein yaakov-stein force-pushed the extract-log-codegen-callback branch from 527d9cd to 6fa1882 Compare March 27, 2026 20:35
Add a bf_log_type discriminator and restructure bf_log as a tagged
union with packet and socket variants. Packet-specific fields
(pkt_size, headers, l2hdr/l3hdr/l4hdr) move into the pkt variant.
The sock variant (pid, comm) is defined but not yet populated.

The struct sizes to the larger packet variant, so total size is
unchanged for ring buffer reservations.
Move packet.c/h from cgen/matcher/ to cgen/ and rename
bf_matcher_generate_packet() to bf_packet_gen_inline_matcher(). This
file provides shared codegen utilities for all packet-based flavors,
not a matcher implementation, and will host additional shared packet
codegen like bf_packet_gen_inline_log() in subsequent commits.
@yaakov-stein yaakov-stein force-pushed the extract-log-codegen-callback branch from 6fa1882 to e014ec7 Compare March 27, 2026 21:01
@yaakov-stein yaakov-stein marked this pull request as ready for review March 27, 2026 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant