Skip to content

lib: chain: disable rules with incompatible same-layer matchers #471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
qdeslandes wants to merge 1 commit into
base: main
Choose a base branch
from
+209 −0
Open

lib: chain: disable rules with incompatible same-layer matchers #471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions src/libbpfilter/chain.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,53 @@ static int _bf_rule_references_empty_set(const struct bf_chain *chain,
return 0;
}

/**
* @brief Check if a rule has matchers on the same layer but different
* protocols.
*
* A rule matching on e.g. both IPv4 source address and IPv6 destination
* address can never match a packet (a packet is either IPv4 or IPv6, not
* both). Same for layer 4 (e.g. TCP vs UDP). Such rules should be disabled.
*
* @param rule Rule to check.
* @return 0 if no conflict, 1 if the rule contains incompatible matchers.
*/
static int _bf_rule_has_incompatible_matchers(const struct bf_rule *rule)
{
uint32_t layer_hdr_id[_BF_MATCHER_LAYER_MAX];

assert(rule);

for (int i = 0; i < _BF_MATCHER_LAYER_MAX; ++i)
layer_hdr_id[i] = (uint32_t)-1;

bf_list_foreach (&rule->matchers, matcher_node) {
struct bf_matcher *matcher = bf_list_node_get_data(matcher_node);
const struct bf_matcher_meta *meta;

if (bf_matcher_get_type(matcher) == BF_MATCHER_SET)
continue;

meta = bf_matcher_get_meta(bf_matcher_get_type(matcher));
if (!meta || meta->layer <= BF_MATCHER_NO_LAYER)
continue;

if (layer_hdr_id[meta->layer] == (uint32_t)-1) {
layer_hdr_id[meta->layer] = meta->hdr_id;
continue;
}

if (layer_hdr_id[meta->layer] != meta->hdr_id) {
bf_warn(
"rule %u has incompatible matchers on the same layer, rule will be disabled",
rule->index);
return 1;
}
}

return 0;
}

int _bf_chain_check_rule(struct bf_chain *chain, struct bf_rule *rule)
{
int r;
Expand All @@ -70,6 +117,9 @@ int _bf_chain_check_rule(struct bf_chain *chain, struct bf_rule *rule)

rule->disabled = r;

if (!rule->disabled)
rule->disabled = _bf_rule_has_incompatible_matchers(rule);
Comment on lines 118 to +121
Copy link
Contributor

@yaakov-stein yaakov-stein Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (rule->log && !rule->disabled)
chain->flags |= BF_FLAG(BF_CHAIN_LOG);

Expand Down
1 change: 1 addition & 0 deletions tests/e2e/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ bf_add_e2e_test(e2e matchers/udp_sport.sh)
bf_add_e2e_test(e2e rules/action_order.sh ROOT)
bf_add_e2e_test(e2e rules/icmp_tc.sh ROOT)
bf_add_e2e_test(e2e rules/icmp_xdp.sh ROOT)
bf_add_e2e_test(e2e rules/incompatible_matchers.sh ROOT)
bf_add_e2e_test(e2e rules/log.sh ROOT)
bf_add_e2e_test(e2e rules/mark.sh ROOT)
bf_add_e2e_test(e2e rules/redirect.sh ROOT)
Expand Down
26 changes: 26 additions & 0 deletions tests/e2e/rules/incompatible_matchers.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#!/usr/bin/env bash
Copy link
Contributor

@yaakov-stein yaakov-stein Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


# Rules with matchers on the same layer but different protocols (e.g. ip4 + ip6)
# should be disabled: the chain loads successfully but the rule has no effect.

. "$(dirname "$0")"/../e2e_test_util.sh

make_sandbox
start_bpfilter

# Baseline: ping works before any chain is loaded.
ping -c 1 -W 0.1 ${NS_IP_ADDR}

# L3 conflict: ip4.saddr + ip6.daddr in the same rule. The rule can never match
# a packet (IPv4 and IPv6 are mutually exclusive), so it should be disabled.
${FROM_NS} bfcli chain set --from-str "chain xdp BF_HOOK_XDP{ifindex=${NS_IFINDEX}} ACCEPT rule ip4.saddr eq ${HOST_IP_ADDR} ip6.daddr eq ::1 counter DROP"

# Ping must still succeed: the DROP rule is disabled, default policy is ACCEPT.
ping -c 1 -W 0.1 ${NS_IP_ADDR}

# L4 conflict: tcp.sport + udp.dport in the same rule.
${FROM_NS} bfcli chain set --from-str "chain xdp BF_HOOK_XDP{ifindex=${NS_IFINDEX}} ACCEPT rule tcp.sport eq 80 udp.dport eq 53 counter DROP"

ping -c 1 -W 0.1 ${NS_IP_ADDR}

stop_bpfilter
Copy link

@github-actions github-actions bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
stop_bpfilter

132 changes: 132 additions & 0 deletions tests/unit/libbpfilter/chain.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,137 @@ static void mixed_enabled_disabled_log_flag(void **state)
assert_int_equal(chain->flags & BF_FLAG(BF_CHAIN_LOG), 0);
}

static void incompatible_matchers_disable_rule(void **state)
{
(void)state;

// L3 conflict: IPv4 + IPv6 matchers.
{
_free_bf_chain_ struct bf_chain *chain = NULL;
_clean_bf_list_ bf_list rules =
bf_list_default(bf_rule_free, bf_rule_pack);
struct bf_rule *rule = NULL;
uint32_t ip4_addr = 0;
uint8_t ip6_addr[16] = {};

assert_ok(bf_rule_new(&rule));
assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP4_SADDR, BF_MATCHER_EQ,
&ip4_addr, sizeof(ip4_addr)));
assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP6_DADDR, BF_MATCHER_EQ,
ip6_addr, sizeof(ip6_addr)));
assert_ok(bf_list_add_tail(&rules, rule));

assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_EGRESS,
BF_VERDICT_ACCEPT, NULL, &rules));
assert_true(rule->disabled);
}

// L4 conflict: TCP + UDP matchers.
{
_free_bf_chain_ struct bf_chain *chain = NULL;
_clean_bf_list_ bf_list rules =
bf_list_default(bf_rule_free, bf_rule_pack);
struct bf_rule *rule = NULL;
uint16_t port = 0;

assert_ok(bf_rule_new(&rule));
assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_TCP_SPORT, BF_MATCHER_EQ,
&port, sizeof(port)));
assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_UDP_DPORT, BF_MATCHER_EQ,
&port, sizeof(port)));
assert_ok(bf_list_add_tail(&rules, rule));

assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_EGRESS,
BF_VERDICT_ACCEPT, NULL, &rules));
assert_true(rule->disabled);
}

// L4 conflict: ICMP + ICMPv6 matchers.
{
_free_bf_chain_ struct bf_chain *chain = NULL;
_clean_bf_list_ bf_list rules =
bf_list_default(bf_rule_free, bf_rule_pack);
struct bf_rule *rule = NULL;
uint8_t type_val = 0;

assert_ok(bf_rule_new(&rule));
assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_ICMP_TYPE, BF_MATCHER_EQ,
&type_val, sizeof(type_val)));
assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_ICMPV6_TYPE,
BF_MATCHER_EQ, &type_val,
sizeof(type_val)));
assert_ok(bf_list_add_tail(&rules, rule));

assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_EGRESS,
BF_VERDICT_ACCEPT, NULL, &rules));
assert_true(rule->disabled);
}

// No conflict: same protocol at same layer (TCP sport + TCP dport).
{
_free_bf_chain_ struct bf_chain *chain = NULL;
_clean_bf_list_ bf_list rules =
bf_list_default(bf_rule_free, bf_rule_pack);
struct bf_rule *rule = NULL;
uint16_t port = 0;

assert_ok(bf_rule_new(&rule));
assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_TCP_SPORT, BF_MATCHER_EQ,
&port, sizeof(port)));
assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_TCP_DPORT, BF_MATCHER_EQ,
&port, sizeof(port)));
assert_ok(bf_list_add_tail(&rules, rule));

assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_EGRESS,
BF_VERDICT_ACCEPT, NULL, &rules));
assert_false(rule->disabled);
}

// No conflict: different layers (IPv4 + TCP).
{
_free_bf_chain_ struct bf_chain *chain = NULL;
_clean_bf_list_ bf_list rules =
bf_list_default(bf_rule_free, bf_rule_pack);
struct bf_rule *rule = NULL;
uint32_t ip4_addr = 0;
uint16_t port = 0;

assert_ok(bf_rule_new(&rule));
assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP4_SADDR, BF_MATCHER_EQ,
&ip4_addr, sizeof(ip4_addr)));
assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_TCP_SPORT, BF_MATCHER_EQ,
&port, sizeof(port)));
assert_ok(bf_list_add_tail(&rules, rule));

assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_EGRESS,
BF_VERDICT_ACCEPT, NULL, &rules));
assert_false(rule->disabled);
}

// Disabled rule does not set BF_CHAIN_LOG.
{
_free_bf_chain_ struct bf_chain *chain = NULL;
_clean_bf_list_ bf_list rules =
bf_list_default(bf_rule_free, bf_rule_pack);
struct bf_rule *rule = NULL;
uint32_t ip4_addr = 0;
uint8_t ip6_addr[16] = {};

assert_ok(bf_rule_new(&rule));
rule->log = 1;
assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP4_SADDR, BF_MATCHER_EQ,
&ip4_addr, sizeof(ip4_addr)));
assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP6_DADDR, BF_MATCHER_EQ,
ip6_addr, sizeof(ip6_addr)));
assert_ok(bf_list_add_tail(&rules, rule));

assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_EGRESS,
BF_VERDICT_ACCEPT, NULL, &rules));
assert_true(rule->disabled);
assert_int_equal(chain->flags & BF_FLAG(BF_CHAIN_LOG), 0);
}
}

static void get_set_by_name(void **state)
{
_free_bf_chain_ struct bf_chain *chain = bft_chain_dummy(true);
Copy link

@github-actions github-actions bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.

Expand All @@ -159,6 +290,7 @@ int main(void)
cmocka_unit_test(dump),
cmocka_unit_test(get_set_from_matcher),
cmocka_unit_test(mixed_enabled_disabled_log_flag),
cmocka_unit_test(incompatible_matchers_disable_rule),
cmocka_unit_test(get_set_by_name),
};

Expand Down
Loading