lib: chain: disable rules with incompatible same-layer matchers#471
lib: chain: disable rules with incompatible same-layer matchers#471qdeslandes wants to merge 1 commit intofacebook:mainfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| ping -c 1 -W 0.1 ${NS_IP_ADDR} | ||
|
|
||
| stop_bpfilter |
There was a problem hiding this comment.
Claude: Explicit stop_bpfilter is unnecessary here — the EXIT trap in e2e_test_util.sh already calls cleanup() which invokes stop_bpfilter(). None of the other rules/ e2e tests (action_order.sh, icmp_tc.sh, icmp_xdp.sh, log.sh, mark.sh, redirect.sh) call it explicitly. Consider removing for consistency.
| stop_bpfilter |
|
|
||
| static void get_set_by_name(void **state) | ||
| { | ||
| _free_bf_chain_ struct bf_chain *chain = bft_chain_dummy(true); |
There was a problem hiding this comment.
Claude: The test covers L3/L4 conflicts and non-conflict cases well. Consider adding sub-cases for a couple of boundary inputs to _bf_rule_has_incompatible_matchers:
- Single matcher — a rule with only one matcher (e.g., just
BF_MATCHER_IP4_SADDR) can never conflict; verifying it isn't disabled guards against loop logic regressions. - Meta-layer matcher — matchers with
BF_MATCHER_NO_LAYER(e.g.,BF_MATCHER_META_IFACE) should be excluded from the conflict check. A rule mixing a meta matcher with an IPv4 matcher should not be flagged.
|
Claude: review of facebook/bpfilter #471 (564fce9) Suggestions
Nits
|
qdeslandes
force-pushed
the
disable_rules_incompatible_matchers
branch
from
4abcead to
5bac247
Compare
A rule combining matchers on the same network layer but targeting different protocols (e.g. ip4.saddr + ip6.daddr, or tcp.sport + udp.dport) can never match a packet. Detect these during chain validation and disable the rule instead of generating dead BPF code.
qdeslandes
force-pushed
the
disable_rules_incompatible_matchers
branch
from
5bac247 to
564fce9
Compare
| rule->disabled = r; | ||
|
|
||
| if (!rule->disabled) | ||
| rule->disabled = _bf_rule_has_incompatible_matchers(rule); |
There was a problem hiding this comment.
rule->disabled = r | _bf_rule_has_incompatible_matchers(rule);? Or probably || so we don't run _bf_rule_has_incompatible_matchers(rule) unless we need to
| @@ -0,0 +1,26 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Are the e2e tests valuable here? IIUC, the prior behavior would be the same as the behavior after the change, so what are we really testing? The only difference would be the codegen should be omitted and the rule should be disabled, which the e2e tests can't really cover, right?
2 participants
A rule combining matchers on the same network layer but targeting different protocols (e.g. ip4.saddr + ip6.daddr, or tcp.sport + udp.dport) can never match a packet. Detect these during chain validation and disable the rule instead of generating dead BPF code.