[0/3] Enable flavor specific matching

[yaakov-stein]

Stacked PRs:

• -> [0/3] Enable flavor specific matching #456
• [1/3] Separate comparison logic from data loading in matchers #464
• [2/3] Add CGROUP_SOCK_ADDR initial chain support #459
• [3/3] Add CGROUP_SOCK_ADDR matcher support #465

Currently, matcher codegen is hardcoded in a flavor-agnostic switch. With this refactor, each flavor controls its own matcher codegen through a callback, allowing a new flavor to intercept matchers and generate context-appropriate bytecode.

The gen_inline_matcher callback generalizes the ad-hoc gen_inline_get_mark and gen_inline_get_skb callbacks. Instead of having dedicated callbacks for each flavor-sensitive value, every flavor controls its full matcher codegen through a single dispatch point. Packet-based flavors delegate most matchers to a shared bf_matcher_generate_packet() function (the extracted switch), overriding only the matchers that need flavor-specific context access.

Here is an overview of the architectural changes:

Current Architecture

  bf_program_generate()
  |
  |-- ops->gen_inline_prologue()             // Flavor-specific: parse raw
  |       |                                  // packet into l3_hdr/l4_hdr
  |       |                                  // scratch buffers, set R7/R8
  |       +-- bf_stub_parse_l3_hdr()
  |       +-- bf_stub_parse_l4_hdr()
  |
  |-- for each rule:
  |     _bf_program_generate_rule()
  |     |
  |     |-- for each matcher:
  |     |     switch(matcher_type)           // Hardcoded switch - same for
  |     |       |                            // ALL flavors (flavor-agnostic)
  |     |       +-- META_* --> bf_matcher_generate_meta()
  |     |       |                 |
  |     |       |                 +-- META_MARK:
  |     |       |                 |     ops->gen_inline_get_mark()  <-- Ad-hoc
  |     |       |                 |                                     flavor callback
  |     |       |                 +-- META_FLOW_HASH:
  |     |       |                       ops->gen_inline_get_skb()   <-- Ad-hoc
  |     |       |                                                       flavor callback
  |     |       +-- IP4_*  --> bf_matcher_generate_ip4()    // Reads l3_hdr
  |     |       +-- IP6_*  --> bf_matcher_generate_ip6()    // Reads l3_hdr
  |     |       +-- TCP_*  --> bf_matcher_generate_tcp()    // Reads l4_hdr
  |     |       +-- UDP_*  --> bf_matcher_generate_udp()    // Reads l4_hdr
  |     |       +-- ICMP_* --> bf_matcher_generate_icmp()   // Reads l4_hdr
  |     |       +-- SET    --> bf_matcher_generate_set()    // Reads l3/l4_hdr
  |     |
  |     |-- ops->gen_inline_set_mark()       // Rule action (if mark set)
  |     +-- ops->get_verdict()               // Emit verdict
  |
  |-- ops->gen_inline_epilogue()             // No-op in all 4 flavors
  +-- ops->get_verdict(chain->policy)        // Default policy

bf_flavor_ops (7 callbacks):

  gen_inline_prologue   -- Flavor-specific header parsing
  gen_inline_epilogue   -- No-op in all 4 flavors (dead code)
  gen_inline_set_mark   -- Write mark to sk_buff (rule action)
  gen_inline_get_mark   -- Read mark from sk_buff (used by meta.mark matcher only)
  gen_inline_get_skb    -- Get sk_buff pointer (used by meta.flow_hash matcher only)
  gen_inline_redirect   -- Redirect packet to another interface
  get_verdict           -- Convert ACCEPT/DROP to BPF return code

Problem: There is no simple mechanism for a flavor to provide alternative bytecode for the same matcher type. The only two flavor-aware matchers (meta.mark and meta.flow_hash) use ad-hoc callbacks rather than a general dispatch mechanism.

New Architecture (After)

  bf_program_generate()
  |
  |-- ops->gen_inline_prologue()             // Unchanged
  |
  |-- for each rule:
  |     _bf_program_generate_rule()
  |     |
  |     |-- for each matcher:
  |     |     ops->gen_inline_matcher(program, matcher)    // NEW: flavor dispatch
  |     |       |
  |     |       +-- [XDP]  bf_matcher_generate_packet() directly
  |     |       |            (no mark/flow_hash support)
  |     |       |
  |     |       +-- [TC]   _bf_tc_gen_inline_matcher()
  |     |       |     +-- META_MARK, META_FLOW_HASH: flavor-specific loading
  |     |       |     +-- default:        bf_matcher_generate_packet()
  |     |       |
  |     |       +-- [NF]   _bf_nf_gen_inline_matcher()
  |     |       |     +-- META_MARK, META_FLOW_HASH: flavor-specific loading
  |     |       |     +-- default:        bf_matcher_generate_packet()
  |     |       |
  |     |       +-- [CGROUP_SKB] _bf_cgroup_gen_inline_matcher()
  |     |       |     +-- META_MARK, META_FLOW_HASH: flavor-specific loading
  |     |       |     +-- default:        bf_matcher_generate_packet()
  |     |
  |     |-- ops->gen_inline_set_mark()       // Unchanged
  |     +-- ops->get_verdict()               // Unchanged
  |
  |-- ops->gen_inline_epilogue()             // Unchanged
  +-- ops->get_verdict(chain->policy)        // Unchanged

bf_flavor_ops (6 callbacks):

  gen_inline_prologue   -- KEPT (unchanged)
  gen_inline_epilogue   -- KEPT (unchanged)
  gen_inline_set_mark   -- KEPT (rule action, unchanged)
  gen_inline_redirect   -- KEPT (unchanged)
  get_verdict           -- KEPT (unchanged)
  gen_inline_matcher    -- NEW: flavor-specific matcher dispatch

  gen_inline_get_mark   -- REMOVED (subsumed by gen_inline_matcher)
  gen_inline_get_skb    -- REMOVED (subsumed by gen_inline_matcher)

Notes:

• Commit a65aca1 adds a missing load instruction that is moved as part of this PR.
• While this enables flavor-specific matching in a more intuitive way, an even better approach would be a callback-centric approach that let's flavors define their own table of callbacks per matcher. The plan is to add this eventually.

[yaakov-stein]

I am slightly torn between two approaches here (but I don't think it is too consequential, especially as bf_matcher_generate_meta_mark is no longer needed after #464):

1. The approach currently in this PR - make bf_matcher_generate_meta_mark/flow_hash public and have the various gen_inline_matcher functions (i.e. _bf_cgroup_skb_gen_inline_matcher) call them directly. Have bf_matcher_generate_packet error out on those meta matchers.
2. We can keep bf_matcher_generate_meta_mark/flow_hash private and the gen_inline_matcher callbacks EMIT the necessary instructions and then call bf_matcher_generate_packet -> bf_matcher_generate_meta -> _bf_matcher_generate_meta_mark/flow_hash.

Approach 2 would add less to the public API and tries to stay as similar to the current architecture, while approach 1 is more explicit in how it handles flavor-specific matchers. I'm currently leaning towards approach 1, especially given future plans to move to something along the lines of a table for per-matcher callbacks, but am open to thoughts. Thanks!

[qdeslandes]

The issue I have with the current suggestion is that bf_matcher_generate_meta_mark and bf_matcher_generate_meta_flow_hash expects the packet's mark to be in r1 already. It should be self contained and not expect specific register values (except for r6 to r9 registers which are callee saved).

[yaakov-stein]

This probably can use some further discussion - my current thoughts, as described in #464, are that matchers have three stages of codegen, two of which are important here:

1. data loading - each flavor will/can have it's own way to load the data into the proper registers
2. comparison - once the values are in the right registers, the comparisons are flavor-agnostic

Given this, my thought is it would make sense to generalize the comparison stage when we can and then have each flavor define it's own data loading step. The idea here is similar, but as mentioned, it does require that we assume previous instructions loaded the correct value into specific registers.

For now, I renamed the functions to be more explicit that they are only generating the comparison bytecode, but that still runs into the problem you've mentioned (the expectation that data is loaded into certain registers).

[qdeslandes]

The design is sound, my comment is mostly about the (private) API we define than the actual logic. I would like the code to be more expressive about the fact r1 is expected to contain the processed value. However, it should be fine in this specific PR and is probably something to refine in the next one (#1/3). I'll add a comment there for more details.

[yaakov-stein]

Drive-by fix here and same thing in nf.c

[github-actions]

Claude: review of facebook/bpfilter #456 (6af7c7f)

Must fix

• Missing copyright year in packet.c — src/bpfilter/cgen/matcher/packet.c:3 — Every file in the repo uses Copyright (c) YYYY Meta Platforms, Inc. and affiliates. with a year; both new files omit it.
• Missing copyright year in packet.h — src/bpfilter/cgen/matcher/packet.h:3 — Same issue as packet.c above.

Suggestions

• bf_flavor_ops defined in public header but used only internally (dismissed) — src/libbpfilter/include/bpfilter/flavor.h — Consider moving the definition to a daemon-private header so internal refactors don't appear as public ABI changes.
• bf_matcher_generate_meta silently mis-reports META_MARK/META_FLOW_HASH — src/bpfilter/cgen/matcher/meta.c — Now has explicit -ENOTSUP guard cases with a clear error message. Fixed.
• assert() on gen_inline_matcher inconsistent with gen_inline_set_mark guard — src/bpfilter/cgen/program.c:296 — gen_inline_set_mark has a runtime check returning -ENOTSUP, but gen_inline_matcher uses assert() which is a no-op in release builds.

Nits

• Inconsistent @brief scope in meta.h — src/bpfilter/cgen/matcher/meta.h:30 — Fixed: precondition now in its own paragraph.
• Commit 1 fix immediately superseded by commit 2 — src/bpfilter/cgen/nf.c — Commit 1 adds the missing BPF_LDX_MEM to _bf_nf_gen_inline_get_skb, but commit 2 deletes the entire function. The fix keeps the series bisectable (good), but consider noting in the commit 1 message that it is a prerequisite fix for the refactor.
• Commit 1 has no body explaining why — The commit daemon: cgen: emit missing skb load in _bf_nf_gen_inline_get_skb has no body. The style guide says the description should explain "why". Even a one-liner such as "Without this load, the skb pointer is not dereferenced before use, producing invalid BPF bytecode" would help reviewers.
• gen_inline_matcher field placed after get_verdict in bf_flavor_ops — src/libbpfilter/include/bpfilter/flavor.h — The new callback is appended after get_verdict, separating it from the other gen_inline_* codegen callbacks. Consider placing it where gen_inline_get_mark/gen_inline_get_skb were to keep codegen callbacks grouped.
• Commit 2 subject says daemon: cgen: but also modifies lib component — The commit modifies src/libbpfilter/include/bpfilter/flavor.h (part of the lib public API), yet the subject only references daemon: cgen:. When a commit spans components, the subject should acknowledge the broader scope or the public API change could be split into its own commit.

CLAUDE.md improvements

• @return phrasing: the PR uses @return 0 on success, negative errno on error. in four new Doxygen blocks, while the dominant codebase convention (49 occurrences across 24 files) is @return 0 on success, or a negative errno value on failure. Consider documenting the preferred phrasing in CLAUDE.md or the style guide.

[qdeslandes]

The issue I have with the current suggestion is that bf_matcher_generate_meta_mark and bf_matcher_generate_meta_flow_hash expects the packet's mark to be in r1 already. It should be self contained and not expect specific register values (except for r6 to r9 registers which are callee saved).

[github-actions]

Claude: suggestion — gen_inline_set_mark is guarded with a runtime check (if (!program->runtime.ops->gen_inline_set_mark) return bf_err_r(-ENOTSUP, ...)) a few lines below, but gen_inline_matcher uses assert() which is compiled out in release builds. If a new flavor is added without setting this callback, the assert won't fire in release and this becomes a NULL dereference.

Since gen_inline_matcher is documented as "Required for all flavors", the assert is defensible as a debug-time invariant check. But for consistency with how gen_inline_set_mark is guarded, consider a runtime check:

if (!program->runtime.ops->gen_inline_matcher)
    return bf_err_r(-ENOTSUP, "gen_inline_matcher not set for %s",
                    program->runtime.chain->name);

bpfilter/src/bpfilter/cgen/program.c

Line 296 in 6af7c7f

assert(program->runtime.ops->gen_inline_matcher);

[qdeslandes]

LGTM, thanks!