[yaakov-stein]

Summary

Currently, chains like these:

chain c BF_HOOK_XDP NEXT 
    rule log NEXT
 
chain c BF_HOOK_XDP NEXT
    rule (meta.sport) not in {} DROP
    rule ... NEXT

will fail to load (referring to chains like these being used through libbpfilter, not via bfcli), as the verifier will complain that BPF instructions are unreachable. Even when no rules follow an unconditional terminal rule, the default-policy block for the chain will still cause this problem.

I see two paths we can take to fix this:

1. libbpfilter can check if rules are unconditional and, if so, reject the ruleset with a clear error (as opposed to a verifier splat + error).
2. libbpfilter can check if rules are unconditional and prevent any unreachable code from being added to the program.

As more users use bpfilter programmatically (via C API usage of libbpfilter), I believe the second approach is the correct one. When programmatically defining a ruleset or populating a set, having zero matchers or having an empty set can easily occur. Requiring the user to ensure those cases don't happen passes unnecessary work onto callers of libbpfilter.

Also, the third commit adds the no-op case of a non-negated meta.probability 100, as that will always be true.

Related issue

#561 - Allow a rule to match all the traffic

Testing

New testing file (unconditional.sh) + some manual testing

Notes for the reviewer

• This is more of an RFC or proposal that happens to have an implementation with it. I'm interested to hear if you think this is the right approach or if we should require the user to guard against these cases.

AI disclosure

Worked with AI for planning and coding

Checklist

• I understand every line in this PR and can explain why it is correct
• I built the project and ran the tests covering this change
• make -C $BUILD test_bin test passes and the code follows the style guide
• Commits are formatted as component: subcomponent: short description
• I have disclosed any AI usage above

[yaakov-stein]

@claude review

[github-actions]

Claude review of PR #607 (8cfc64c)

Suggestions

• Unreachable rules skip codegen-time validation — src/libbpfilter/cgen/program.c:963 — after the break, later rules no longer hit the REDIRECT-per-hook and XDP in checks, so chains that were rejected before now load, and a later chain update can fail on a rule nobody touched
• Read reachability from the emitted code instead of re-deriving it — src/libbpfilter/cgen/program.c:615 — _bf_program_rule_is_unconditional() duplicates _bf_program_matcher_is_noop() and _bf_program_check_proto(); checking for pending BF_FIXUP_TYPE_JMP_NEXT_RULE fixups gives the exact answer by construction
• (meta.sport) not in {} drops the TCP/UDP guard and now ends the chain — src/libbpfilter/cgen/program.c:633 — the sport/dport L4 guard is only emitted in _bf_matcher_pkt_prepare_header(), which no-op sets skip, so ICMP matches and later rules and the policy are dropped
• rule-evaluation.rst still says "one or more matchers" — doc/usage/reference/rules.rst:16 — doc/usage/explanation/rule-evaluation.rst:15 now contradicts the reference
• Compare fixup list sizes instead of scanning — src/libbpfilter/cgen/program.c:949 — _bf_program_has_fixup() walks every pending fixup for each rule; bf_list_size() before/after _bf_program_fixup() gives the same answer in O(1)
• Document that all-no-op rules are unconditional too — doc/usage/explanation/rule-evaluation.rst:36 — ... not in {} on an empty set and meta.probability eq 100% also hide later rules and the policy
• Test pins (meta.sport) not in {} matching ICMP — tests/e2e/rules/unconditional.sh:10 — the guard gap from the earlier item is still there; the new test now expects ICMP to hit this rule and end the chain
• Only XDP is covered by the new e2e test — tests/e2e/rules/unconditional.sh:13 — dropping the epilogue and policy block affects every flavor; load-only checks on TC/NF/cgroup_skb/cgroup_sock_addr would catch flavor-specific verifier rejections
• No unprivileged parsing test for the grammar change — tests/e2e/CMakeLists.txt:150 — rule DROP, rule with no verdict, and REDIRECT on NF under --dry-run could go in tests/e2e/parsing/
• New checks now reject disabled rules that used to load — src/libbpfilter/chain.c:170 — the REDIRECT/verdict/operator checks also run on rules disabled by an empty-set or incompatible matcher, which codegen skipped before; this also affects restoring pinned chains through bf_chain_new_from_pack()
• Unit tests rely on bf_chain_new() consuming rules on failure — tests/unit/libbpfilter/chain.c:623 — this goes against the chain.h contract ("both list are unchanged"); bf_list_clean(&rules) between cases would keep the tests independent of it
• Document what happens after an unconditional terminal rule — doc/usage/explanation/rule-evaluation.rst:17 — later rules and the policy are never evaluated, so their counters stay at 0
• Matcher payload length is not validated — src/libbpfilter/chain.c:242 — _bf_program_matcher_is_noop() adds another *(const float *) payload read; the payload size could be checked against ops->ref_payload_size here (gap predates the PR, follow-up is fine)

Nits

• Doxygen for _bf_program_rule_is_unconditional() — src/libbpfilter/cgen/program.c:615 — its sibling has one; the contract (ignores the verdict, expects sets to exist) isn't obvious from the signature
• Log when rules or the policy become unreachable — src/libbpfilter/cgen/program.c:965 — chain.c already uses bf_warn() when it disables rules
• Move the parser-only checks to the parsing/ suite — tests/e2e/rules/unconditional.sh:7 — the --dry-run grammar checks don't need ROOT or a sandbox
• Paragraph is hard-wrapped — doc/usage/explanation/rule-evaluation.rst:36 — the rest of the file uses one line per paragraph
• Reuse the new type local — src/libbpfilter/chain.c:229 — the loop body still calls bf_matcher_get_type(matcher) several times
• REDIRECT uses assert(), mark still returns -ENOTSUP — src/libbpfilter/cgen/program.c:723 — both are validated in chain.c; is the different handling intended?
• Keep the XDP direction invariant visible — src/libbpfilter/cgen/xdp.c:98 — the doc comment still says dir must be egress; an assert() would document it
• _bf_program_matcher_is_noop() doc comment is out of date — src/libbpfilter/cgen/program.c:618 — "protocol checks are still generated" is not true for meta.sport/meta.dport, and doesn't apply to meta.probability
• Unreachable rules still set chain flags — src/libbpfilter/cgen/program.c:959 — BF_CHAIN_LOG* / BF_CHAIN_STORE_NEXTHDR still come from rules that are never emitted
• "Disabled rules" is not defined in the user docs — doc/usage/reference/bfcli.rst:24 — users don't know this internal term
• Doc sentence lands before the feature exists — doc/usage/reference/bfcli.rst:24 — "rules that follow an unconditional terminal rule" is added in the first commit, before codegen/parser support
• n_fixups declared mid-block — src/libbpfilter/cgen/program.c:933 — locals usually go at the top of the block; policy_reachable also controls whether the next rule is emitted
• Unrelated blank line — src/libbpfilter/cgen/program.c:705 — the REDIRECT case is otherwise untouched
• Counter index 6 is the policy counter — tests/e2e/rules/unconditional.sh:24 — a short comment would make that clear (also line 38)
• "Comparison no-ops" comment — tests/e2e/rules/unconditional.sh:40 — (meta.sport) not in {} is a set matcher, not a comparison
• Codegen workflow comment still says the policy always applies — src/libbpfilter/cgen/program.h — the @file overview doesn't mention that the program can end at the last reachable rule

Errors

• mcp__github__get_pull_request_comments isn't available in this environment, so I used mcp__github__get_issue_comments for top-level comments instead.

Workflow run

[yaakov-stein]

@claude review

[github-actions]

Claude: nit: Rules after an unconditional terminal rule are never emitted, but _bf_chain_check_rule() still uses them to set BF_CHAIN_LOG, BF_CHAIN_LOG_RATELIMIT and BF_CHAIN_STORE_NEXTHDR. For example, rule DROP rule log every 1s DROP still creates the log and state maps and runs the state-map lookup in the prologue for every packet. An unreachable ip6.nexthdr rule still turns on IPv6 extension-header parsing. The program still works, but is this extra per-packet work intended? Or should unreachable rules stop contributing to the chain flags?

[yaakov-stein]

@claude review

[prashantpcm94]

(meta.sport) not in {} isn't a no-op: meta.sport not 9 and (meta.sport) not in {9} never match ICMP (ports exist only for TCP/UDP), so an empty set shouldn't either.
Treating it as unconditional makes this rule drop ping and skip the rest of the chain.

[qdeslandes]

Few nits. Also, in the last commit "Cover it in the existing XDP regression." is probably not necessary, we usually don't mention the tests in the commit messages unless the commit is only about the tests.

[qdeslandes]

This comment is not very clear to me, do you mind rephrasing it?

[qdeslandes]

All good for now, but we'll eventually need to define a better model to handle flavor-level and matcher-level requirements and constraints.

[qdeslandes]

Make the check more explicit, e.g. _bf_program_has_fixup(program, BF_FIXUP_TYPE_JMP_NEXT_RULE), with _bf_program_has_fixup properly documented.

[qdeslandes]

I had to think hard about this, it definitely doesn't behave as I thought it would!

[qdeslandes]

Newline below.

[qdeslandes]

"suffix" here is the rules after the unconditional one? It's confusing IMO

[qdeslandes]

meta.probability not eq 0% should also be handled here. Currently, 0% has a probability of 1 in 2^32 to match because we use BPF_JLE for the comparison. This is a good opportunity to fix one of those two legs (the other one being that meta.probability eq 0% should never match, but that's another thing).