From 049eb0dc75024c026a13b3a33f492b1eeb924f13 Mon Sep 17 00:00:00 2001 From: yaakov-stein Date: Thu, 15 Oct 2026 15:40:51 -0700 Subject: [PATCH 1/4] lib: chain: validate rules before code generation Check REDIRECT support, matcher operators and verdicts for every rule, including disabled rules. Unreachable rules must not bypass validation. --- doc/usage/reference/bfcli.rst | 2 +- src/libbpfilter/cgen/xdp.c | 3 +- src/libbpfilter/chain.c | 48 ++++++++++--- tests/unit/libbpfilter/chain.c | 127 +++++++++++++++++++++++++++++++++ 4 files changed, 167 insertions(+), 13 deletions(-) diff --git a/doc/usage/reference/bfcli.rst b/doc/usage/reference/bfcli.rst index eb0c8792a..0501da902 100644 --- a/doc/usage/reference/bfcli.rst +++ b/doc/usage/reference/bfcli.rst @@ -21,7 +21,7 @@ Commands ``bfcli`` commands are structured as ``bfcli OBJECT ACTION``. The commands and actions supported by ``bfcli`` are described below. -For the commands supporting it, ``--dry-run`` will parse the chains and check that their matchers, sets, ``log``, and ``mark`` are supported by their hook, then stop: nothing is generated, loaded, or attached, so you don't need to be root. A successful dry run doesn't mean the chain will load: for example, bpfilter only checks if the hook supports ``REDIRECT`` when it generates the program. +For the commands supporting it, ``--dry-run`` will parse the chains and check that their matchers, sets, ``log``, ``mark``, and ``REDIRECT`` are supported by their hook, then stop: nothing is generated, loaded, or attached, so you don't need to be root. A successful dry run doesn't guarantee that the chain will load. ``ruleset set`` ~~~~~~~~~~~~~~~ diff --git a/src/libbpfilter/cgen/xdp.c b/src/libbpfilter/cgen/xdp.c index c66c06eaf..32e3851c9 100644 --- a/src/libbpfilter/cgen/xdp.c +++ b/src/libbpfilter/cgen/xdp.c @@ -95,8 +95,7 @@ static int _bf_xdp_gen_inline_redirect(struct bf_program *program, { assert(program); - if (dir != BF_REDIRECT_EGRESS) - return bf_err_r(-ENOTSUP, "XDP redirect only supports 'out' direction"); + (void)dir; // bpf_redirect(ifindex, flags) - flags are ignored for XDP EMIT(program, BPF_MOV64_IMM(BPF_REG_1, ifindex)); diff --git a/src/libbpfilter/chain.c b/src/libbpfilter/chain.c index 65b4cb05d..e402483e4 100644 --- a/src/libbpfilter/chain.c +++ b/src/libbpfilter/chain.c @@ -163,6 +163,26 @@ static int _bf_chain_check_rule(struct bf_chain *chain, struct bf_rule *rule) assert(rule); + if (rule->verdict < 0 || rule->verdict >= _BF_VERDICT_MAX) { + return bf_err_r(-EINVAL, "unknown verdict %d in rule %u", rule->verdict, + rule->index); + } + + if (rule->verdict == BF_VERDICT_REDIRECT) { + enum bf_flavor flavor = bf_hook_to_flavor(chain->hook); + + if (flavor != BF_FLAVOR_XDP && flavor != BF_FLAVOR_TC) { + return bf_err_r(-ENOTSUP, "redirect is not supported by %s hook", + bf_hook_to_str(chain->hook)); + } + + if (flavor == BF_FLAVOR_XDP && + rule->redirect_dir != BF_REDIRECT_EGRESS) { + return bf_err_r(-ENOTSUP, + "XDP redirect only supports 'out' direction"); + } + } + r = _bf_rule_has_unmatchable_set_matcher(chain, rule); if (r < 0) return r; @@ -206,28 +226,36 @@ static int _bf_chain_check_rule(struct bf_chain *chain, struct bf_rule *rule) bf_list_foreach (&rule->matchers, matcher_node) { struct bf_matcher *matcher = bf_list_node_get_data(matcher_node); + enum bf_matcher_type type = bf_matcher_get_type(matcher); + enum bf_matcher_op op = bf_matcher_get_op(matcher); const struct bf_matcher_meta *meta; // Track if the chain uses IPv6 nexthdr matcher. - if (bf_matcher_get_type(matcher) == BF_MATCHER_IP6_NEXTHDR && - !rule->disabled) + if (type == BF_MATCHER_IP6_NEXTHDR && !rule->disabled) chain->flags |= BF_FLAG(BF_CHAIN_STORE_NEXTHDR); // Ensure the matcher is compatible with the chain's hook. - meta = bf_matcher_get_meta(bf_matcher_get_type(matcher)); + meta = bf_matcher_get_meta(type); if (!meta) { - return bf_err_r(-EINVAL, "unknown matcher type %d in rule", - bf_matcher_get_type(matcher)); + return bf_err_r(-EINVAL, "unknown matcher type %d in rule", type); + } + + if (op < 0 || op >= _BF_MATCHER_OP_MAX || + (type == BF_MATCHER_SET && op != BF_MATCHER_IN) || + (type != BF_MATCHER_SET && + (op == BF_MATCHER_IN || !bf_matcher_get_ops(type, op)))) { + return bf_err_r(-EINVAL, + "unsupported operator %d for matcher %s in rule %u", + op, bf_matcher_type_to_str(type), rule->index); } if (meta->unsupported_hooks & BF_FLAG(chain->hook)) { - return bf_err_r( - -ENOTSUP, "matcher %s is not compatible with %s", - bf_matcher_type_to_str(bf_matcher_get_type(matcher)), - bf_hook_to_str(chain->hook)); + return bf_err_r(-ENOTSUP, "matcher %s is not compatible with %s", + bf_matcher_type_to_str(type), + bf_hook_to_str(chain->hook)); } - if (bf_matcher_get_type(matcher) == BF_MATCHER_SET) { + if (type == BF_MATCHER_SET) { const struct bf_set *set = bf_chain_get_set_for_matcher(chain, matcher); diff --git a/tests/unit/libbpfilter/chain.c b/tests/unit/libbpfilter/chain.c index bf82acfdb..4813f6016 100644 --- a/tests/unit/libbpfilter/chain.c +++ b/tests/unit/libbpfilter/chain.c @@ -735,6 +735,130 @@ static void get_set_by_name(void **state) assert_null(bf_chain_get_set_by_name(chain, "bft_set_missing")); } +static void redirect_validation(void **state) +{ + _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; + + (void)state; + + // NF does not support REDIRECT. + assert_ok(bf_rule_new(&rule)); + rule->verdict = BF_VERDICT_REDIRECT; + bf_rule_set_redirect(rule, 1, BF_REDIRECT_EGRESS); + assert_ok(bf_list_add_tail(&rules, rule)); + assert_err(bf_chain_new(&chain, "test", BF_HOOK_NF_LOCAL_IN, + BF_VERDICT_ACCEPT, NULL, &rules)); + bf_list_clean(&rules); + + // XDP does not support ingress redirects. + assert_ok(bf_rule_new(&rule)); + rule->verdict = BF_VERDICT_REDIRECT; + bf_rule_set_redirect(rule, 1, BF_REDIRECT_INGRESS); + assert_ok(bf_list_add_tail(&rules, rule)); + assert_err(bf_chain_new(&chain, "test", BF_HOOK_XDP, BF_VERDICT_ACCEPT, + NULL, &rules)); + bf_list_clean(&rules); + + // XDP supports egress redirects. + assert_ok(bf_rule_new(&rule)); + rule->verdict = BF_VERDICT_REDIRECT; + bf_rule_set_redirect(rule, 1, BF_REDIRECT_EGRESS); + assert_ok(bf_list_add_tail(&rules, rule)); + assert_ok(bf_chain_new(&chain, "test", BF_HOOK_XDP, BF_VERDICT_ACCEPT, NULL, + &rules)); + bf_chain_free(&chain); + + // TC supports ingress redirects. + assert_ok(bf_rule_new(&rule)); + rule->verdict = BF_VERDICT_REDIRECT; + bf_rule_set_redirect(rule, 1, BF_REDIRECT_INGRESS); + assert_ok(bf_list_add_tail(&rules, rule)); + assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_INGRESS, + BF_VERDICT_ACCEPT, NULL, &rules)); +} + +static void matcher_operator_validation(void **state) +{ + _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 addr = 0; + uint8_t flags = 1; + + (void)state; + + // ANY is not supported for IPv4 addresses. + assert_ok(bf_rule_new(&rule)); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP4_SADDR, BF_MATCHER_ANY, + &addr, sizeof(addr), false)); + assert_ok(bf_list_add_tail(&rules, rule)); + assert_err(bf_chain_new(&chain, "test", BF_HOOK_XDP, BF_VERDICT_ACCEPT, + NULL, &rules)); + bf_list_clean(&rules); + + // Membership requires a SET matcher, not a scalar IN matcher. + assert_ok(bf_rule_new(&rule)); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP4_SADDR, BF_MATCHER_IN, + &addr, sizeof(addr), false)); + assert_ok(bf_list_add_tail(&rules, rule)); + assert_err(bf_chain_new(&chain, "test", BF_HOOK_XDP, BF_VERDICT_ACCEPT, + NULL, &rules)); + bf_list_clean(&rules); + + // Reject an operator past the enum bounds. + assert_ok(bf_rule_new(&rule)); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP4_SADDR, + _BF_MATCHER_OP_MAX, &addr, sizeof(addr), + false)); + assert_ok(bf_list_add_tail(&rules, rule)); + assert_err(bf_chain_new(&chain, "test", BF_HOOK_XDP, BF_VERDICT_ACCEPT, + NULL, &rules)); + bf_list_clean(&rules); + + // Reject a negative operator. + assert_ok(bf_rule_new(&rule)); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP4_SADDR, -1, &addr, + sizeof(addr), false)); + assert_ok(bf_list_add_tail(&rules, rule)); + assert_err(bf_chain_new(&chain, "test", BF_HOOK_XDP, BF_VERDICT_ACCEPT, + NULL, &rules)); + bf_list_clean(&rules); + + // ANY is supported for TCP flags, unlike IPv4 addresses. + assert_ok(bf_rule_new(&rule)); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_TCP_FLAGS, BF_MATCHER_ANY, + &flags, sizeof(flags), false)); + assert_ok(bf_list_add_tail(&rules, rule)); + assert_ok(bf_chain_new(&chain, "test", BF_HOOK_XDP, BF_VERDICT_ACCEPT, NULL, + &rules)); +} + +static void verdict_validation(void **state) +{ + _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; + + (void)state; + + // Reject a verdict past the enum bounds. + assert_ok(bf_rule_new(&rule)); + rule->verdict = _BF_VERDICT_MAX; + assert_ok(bf_list_add_tail(&rules, rule)); + assert_err(bf_chain_new(&chain, "test", BF_HOOK_XDP, BF_VERDICT_ACCEPT, + NULL, &rules)); + bf_list_clean(&rules); + + // Reject a negative verdict. + assert_ok(bf_rule_new(&rule)); + rule->verdict = -1; + assert_ok(bf_list_add_tail(&rules, rule)); + assert_err(bf_chain_new(&chain, "test", BF_HOOK_XDP, BF_VERDICT_ACCEPT, + NULL, &rules)); +} + int main(void) { const struct CMUnitTest tests[] = { @@ -760,6 +884,9 @@ int main(void) cmocka_unit_test(get_set_by_name), cmocka_unit_test(named_reference_stability), cmocka_unit_test(unpack_duplicate_sets), + cmocka_unit_test(redirect_validation), + cmocka_unit_test(matcher_operator_validation), + cmocka_unit_test(verdict_validation), }; return cmocka_run_group_tests(tests, NULL, NULL); From 4fd4cb94bf2deb03dc4cf9c6bd3e3713f0a0314a Mon Sep 17 00:00:00 2001 From: yaakov-stein Date: Thu, 15 Oct 2026 06:32:19 -0700 Subject: [PATCH 2/4] lib: cgen: omit unreachable code after unconditional verdicts Omit subsequent rules and default-policy code after unconditional terminal verdicts, avoiding verifier rejection of unreachable instructions. Document support for unconditional rules in the C API. --- src/libbpfilter/cgen/program.c | 74 +++++++++++++++--------- src/libbpfilter/include/bpfilter/chain.h | 2 +- tests/unit/libbpfilter/chain.c | 10 +++- 3 files changed, 55 insertions(+), 31 deletions(-) diff --git a/src/libbpfilter/cgen/program.c b/src/libbpfilter/cgen/program.c index eb53e6c64..de2a55ab6 100644 --- a/src/libbpfilter/cgen/program.c +++ b/src/libbpfilter/cgen/program.c @@ -591,14 +591,15 @@ static int _bf_program_generate_log(struct bf_program *program, } /** - * @brief Check whether a matcher needs no bytecode. + * @brief Check whether matcher code generation can be skipped. * * A matcher needs no bytecode if it matches every packet the rule applies - * to. The protocol checks derived from the matcher are still generated. + * to. Protocol checks emitted before the matcher are still generated, but + * guards inside the matcher code are skipped. * * @param program Program to generate the matcher for. Can't be NULL. * @param matcher Matcher to check. Can't be NULL. - * @return True if no bytecode is needed for the matcher, false otherwise. + * @return True if matcher code generation can be skipped, false otherwise. */ static bool _bf_program_matcher_is_noop(const struct bf_program *program, const struct bf_matcher *matcher) @@ -631,9 +632,6 @@ static int _bf_program_generate_rule(struct bf_program *program, assert(program->runtime.ops->gen_inline_matcher); assert(program->runtime.ops->gen_inline_log); - if (rule->disabled) - return 0; - bf_list_foreach (&rule->matchers, matcher_node) { struct bf_matcher *matcher = bf_list_node_get_data(matcher_node); @@ -726,10 +724,6 @@ static int _bf_program_generate_rule(struct bf_program *program, break; } - r = _bf_program_fixup(program, BF_FIXUP_TYPE_JMP_NEXT_RULE); - if (r) - return bf_err_r(r, "failed to generate next rule fixups"); - return 0; } @@ -884,6 +878,7 @@ int bf_program_emit_fixup_elfstub(struct bf_program *program, int bf_program_generate(struct bf_program *program) { const struct bf_chain *chain = program->runtime.chain; + bool next_reachable = true; int ret_code; int r; @@ -899,6 +894,8 @@ int bf_program_generate(struct bf_program *program) EMIT(program, BPF_MOV64_IMM(BPF_REG_7, 0)); EMIT(program, BPF_MOV64_IMM(BPF_REG_8, 0)); + // @todo Avoid feature setup and maps needed only by unreachable rules. + // If at least one rule logs the matched packets, populate ctx->log_map if (program->runtime.chain->flags & BF_FLAG(BF_CHAIN_LOG)) { EMIT_LOAD_LOG_FD_FIXUP(program, BPF_REG_2); @@ -931,30 +928,51 @@ int bf_program_generate(struct bf_program *program) } bf_list_foreach (&chain->rules, rule_node) { - r = _bf_program_generate_rule(program, - bf_list_node_get_data(rule_node)); + struct bf_rule *rule = bf_list_node_get_data(rule_node); + size_t n_fixups; + + if (rule->disabled) + continue; + + r = _bf_program_generate_rule(program, rule); if (r) return r; + + n_fixups = bf_list_size(&program->fixups); + r = _bf_program_fixup(program, BF_FIXUP_TYPE_JMP_NEXT_RULE); + if (r) + return bf_err_r(r, "failed to generate next rule fixups"); + + /* Resolving next-rule jumps removes their fixups and leaves all other + * fixups unchanged. If the list shrinks, the rule has a jump to the + * next rule. CONTINUE also reaches the next rule by falling through. */ + next_reachable = rule->verdict == BF_VERDICT_CONTINUE || + bf_list_size(&program->fixups) < n_fixups; + + if (!next_reachable) + break; } - r = program->runtime.ops->gen_inline_epilogue(program); - if (r) - return r; + if (next_reachable) { + r = program->runtime.ops->gen_inline_epilogue(program); + if (r) + return r; - // Call the update counters function - /// @todo Allow chains to have no counters at all. - EMIT(program, BPF_MOV64_REG(BPF_REG_1, BPF_REG_10)); - EMIT(program, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, BF_PROG_CTX_OFF(arg))); - EMIT_LOAD_COUNTERS_FD_FIXUP(program, BPF_REG_2); - EMIT(program, - BPF_MOV32_IMM(BPF_REG_3, bf_program_chain_counter_idx(program))); - EMIT_FIXUP_ELFSTUB(program, BF_ELFSTUB_UPDATE_COUNTERS); + // Call the update counters function + /// @todo Allow chains to have no counters at all. + EMIT(program, BPF_MOV64_REG(BPF_REG_1, BPF_REG_10)); + EMIT(program, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, BF_PROG_CTX_OFF(arg))); + EMIT_LOAD_COUNTERS_FD_FIXUP(program, BPF_REG_2); + EMIT(program, + BPF_MOV32_IMM(BPF_REG_3, bf_program_chain_counter_idx(program))); + EMIT_FIXUP_ELFSTUB(program, BF_ELFSTUB_UPDATE_COUNTERS); - r = program->runtime.ops->get_verdict(chain->policy, &ret_code); - if (r) - return r; - EMIT(program, BPF_MOV64_IMM(BPF_REG_0, ret_code)); - EMIT(program, BPF_EXIT_INSN()); + r = program->runtime.ops->get_verdict(chain->policy, &ret_code); + if (r) + return r; + EMIT(program, BPF_MOV64_IMM(BPF_REG_0, ret_code)); + EMIT(program, BPF_EXIT_INSN()); + } r = _bf_program_generate_elfstubs(program); if (r) diff --git a/src/libbpfilter/include/bpfilter/chain.h b/src/libbpfilter/include/bpfilter/chain.h index f0a60a376..2bf4c9225 100644 --- a/src/libbpfilter/include/bpfilter/chain.h +++ b/src/libbpfilter/include/bpfilter/chain.h @@ -127,7 +127,7 @@ void bf_chain_dump(const struct bf_chain *chain, prefix_t *prefix); * The chain will own the rule and is responsible for freeing it. The rule's * index will automatically be updated. * - * @todo Rules without any matcher should be rejected. + * A rule without any matcher matches unconditionally. * * @param chain Chain to insert the rule into. Can't be NULL. * @param rule Rule to insert into the chain. Can't be NULL. diff --git a/tests/unit/libbpfilter/chain.c b/tests/unit/libbpfilter/chain.c index 4813f6016..01c734ca8 100644 --- a/tests/unit/libbpfilter/chain.c +++ b/tests/unit/libbpfilter/chain.c @@ -743,7 +743,10 @@ static void redirect_validation(void **state) (void)state; - // NF does not support REDIRECT. + // NF does not support REDIRECT, even after an unconditional DROP. + assert_ok(bf_rule_new(&rule)); + rule->verdict = BF_VERDICT_DROP; + assert_ok(bf_list_add_tail(&rules, rule)); assert_ok(bf_rule_new(&rule)); rule->verdict = BF_VERDICT_REDIRECT; bf_rule_set_redirect(rule, 1, BF_REDIRECT_EGRESS); @@ -789,7 +792,10 @@ static void matcher_operator_validation(void **state) (void)state; - // ANY is not supported for IPv4 addresses. + // An unconditional DROP must not hide an unsupported scalar operator. + assert_ok(bf_rule_new(&rule)); + rule->verdict = BF_VERDICT_DROP; + assert_ok(bf_list_add_tail(&rules, rule)); assert_ok(bf_rule_new(&rule)); assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP4_SADDR, BF_MATCHER_ANY, &addr, sizeof(addr), false)); From 0d4e958b7fbd229ecfeb70be232e9f4dea2e8c17 Mon Sep 17 00:00:00 2001 From: yaakov-stein Date: Thu, 15 Oct 2026 07:52:22 -0700 Subject: [PATCH 3/4] cli: parser: allow unconditional rules Allow matcherless rules in bfcli and document the syntax, exposing the library's unconditional-rule support. Add XDP regressions for unconditional rule behavior. --- doc/usage/explanation/rule-evaluation.rst | 4 +- doc/usage/reference/rules.rst | 4 +- doc/usage/reference/ruleset.rst | 2 +- src/bfcli/parser.y | 6 +-- tests/e2e/CMakeLists.txt | 1 + tests/e2e/rules/unconditional.sh | 51 +++++++++++++++++++++++ 6 files changed, 59 insertions(+), 9 deletions(-) create mode 100755 tests/e2e/rules/unconditional.sh diff --git a/doc/usage/explanation/rule-evaluation.rst b/doc/usage/explanation/rule-evaluation.rst index 4a95050c7..833ef4685 100644 --- a/doc/usage/explanation/rule-evaluation.rst +++ b/doc/usage/explanation/rule-evaluation.rst @@ -12,7 +12,9 @@ Let's see how they work together. How does a rule match? ---------------------- -A rule contains one or more matchers, and the packet has to match **all** of them. For example, a rule defined with an IPv4 source address matcher and a TCP destination port matcher will only match the TCP packets coming from this address and sent to this port. +A rule contains zero or more matchers, and the packet has to match **all** of them. For example, a rule defined with an IPv4 source address matcher and a TCP destination port matcher will only match the TCP packets coming from this address and sent to this port. + +A rule without matchers matches every packet or socket operation that reaches it. What if you want to match one address or another? Adding two ``ip4.saddr`` matchers to the same rule won't work: a packet only has one source address, so the rule would never match. Use two rules, or a set. A set contains multiple elements of the same type, and the rule will match if the packet's data is one of them. A named set is also defined outside of the rule: you can use it from multiple rules, and change its content without writing the whole chain again. diff --git a/doc/usage/reference/rules.rst b/doc/usage/reference/rules.rst index a71e05277..87590f4ad 100644 --- a/doc/usage/reference/rules.rst +++ b/doc/usage/reference/rules.rst @@ -6,14 +6,14 @@ Rules are defined such as: .. code-block:: text rule - $MATCHER... + [$MATCHER]... [log [$HEADERS] [every $FREQUENCY]] [counter] [mark $MARK] $VERDICT With: - - ``$MATCHER``: one or more matchers: the packet has to match all of them. A matcher will either compare the packet's data to a value (see :doc:`matchers`), or check if it's in a set (see :doc:`sets`). Both can be mixed, in any order. + - ``$MATCHER``: zero or more matchers: the packet has to match all of them. A matcher will either compare the packet's data to a value (see :doc:`matchers`), or check if it's in a set (see :doc:`sets`). Both can be mixed, in any order. A rule without matchers matches every packet or socket operation that reaches it. - ``log``: optional. Three forms are supported: - ``log $HEADERS``: log specific packet headers. ``$HEADERS`` is a comma-separated list of ``link`` (layer 2), ``internet`` (layer 3), and/or ``transport`` (layer 4). Only supported by packet-based hooks (XDP, TC, NF, cgroup_skb). diff --git a/doc/usage/reference/ruleset.rst b/doc/usage/reference/ruleset.rst index c67010d68..2e5be4e48 100644 --- a/doc/usage/reference/ruleset.rst +++ b/doc/usage/reference/ruleset.rst @@ -8,7 +8,7 @@ A ruleset is composed of one or more chains. A chain is defined by its name, its chain $NAME $HOOK[{$OPTIONS}] $POLICY [set $SET_NAME $KEY [min-size=$MIN_SIZE] in { $ELEMENTS }]... [rule - $MATCHER... + [$MATCHER]... [log [$HEADERS] [every $FREQUENCY]] [counter] [mark $MARK] diff --git a/src/bfcli/parser.y b/src/bfcli/parser.y index dde21918d..0e4504d1d 100644 --- a/src/bfcli/parser.y +++ b/src/bfcli/parser.y @@ -494,17 +494,13 @@ log_headers : STRING } ; -matchers : matcher +matchers : %empty { _free_bf_list_ bf_list *list = NULL; if (bf_list_new(&list, (bf_list_ops[]){{.free = (bf_list_ops_free)bf_matcher_free, .pack = (bf_list_ops_pack)bf_matcher_pack}}) < 0) bf_parse_err("failed to allocate a new bf_list for bf_matcher\n"); - if (bf_list_add_tail(list, $1) < 0) - bf_parse_err("failed to insert matcher into bf_list\n"); - - TAKE_PTR($1); $$ = TAKE_PTR(list); } | matchers matcher diff --git a/tests/e2e/CMakeLists.txt b/tests/e2e/CMakeLists.txt index 2569f5092..59132e5a0 100644 --- a/tests/e2e/CMakeLists.txt +++ b/tests/e2e/CMakeLists.txt @@ -157,6 +157,7 @@ bf_add_e2e_shell_test(e2e rules/redirect.sh ROOT) bf_add_e2e_shell_test(e2e rules/set_grouping.sh ROOT) bf_add_e2e_shell_test(e2e rules/set_min_size.sh ROOT) bf_add_e2e_shell_test(e2e rules/set_port_range.sh ROOT) +bf_add_e2e_shell_test(e2e rules/unconditional.sh ROOT) bf_add_e2e_shell_test(e2e hooks/cgroup_skb_dual_stack.sh ROOT) bf_add_e2e_shell_test(e2e hooks/cgroup_sock_addr_connect4.sh ROOT) diff --git a/tests/e2e/rules/unconditional.sh b/tests/e2e/rules/unconditional.sh new file mode 100755 index 000000000..b32b22cc2 --- /dev/null +++ b/tests/e2e/rules/unconditional.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +# Copyright (c) Meta Platforms, Inc. and affiliates. + +. "$(dirname "$0")"/../e2e_test_util.sh + +make_sandbox +# Confirm connectivity and populate neighbor entries before testing DROP. +ping -c 1 -W 0.1 ${NS_IP_ADDR} + +# Set-map, logging and counter fixups remain valid when the suffix is omitted. +${FROM_NS} ${BFCLI} chain set --from-str \ + "chain c BF_HOOK_XDP{ifindex=${NS_IFINDEX}} NEXT \ + set s (ip4.daddr) in {} \ + rule meta.l3_proto not ipv4 NEXT \ + rule (ip4.daddr) in s counter DROP \ + rule (tcp.dport) not in {} (ip4.saddr) in { ${HOST_IP_ADDR} } NEXT \ + rule counter CONTINUE \ + rule counter log DROP \ + rule counter CONTINUE" +(! ping -c 1 -W 0.1 ${NS_IP_ADDR}) +test "$(get_counter c 3)" -gt 0 +test "$(get_counter c 4)" -gt 0 +test "$(get_counter c 5)" = "0" +# Counter 6 belongs to the chain policy, after the six rules. +test "$(get_counter c 6)" = "0" + +# Adding a condition to DROP restores the original suffix and policy. +${FROM_NS} ${BFCLI} chain update --from-str \ + "chain c BF_HOOK_XDP{ifindex=${NS_IFINDEX}} NEXT \ + set s (ip4.daddr) in {} \ + rule meta.l3_proto not ipv4 NEXT \ + rule (ip4.daddr) in s counter DROP \ + rule (tcp.dport) not in {} (ip4.saddr) in { ${HOST_IP_ADDR} } NEXT \ + rule counter CONTINUE \ + rule ip4.proto tcp counter log DROP \ + rule counter CONTINUE" +ping -c 1 -W 0.1 ${NS_IP_ADDR} +test "$(get_counter c 5)" -gt 0 +test "$(get_counter c 6)" -gt 0 + +# Comparison no-ops also terminate without emitting the suffix or policy. +for matcher in '(meta.sport) not in {}'; do + ${FROM_NS} ${BFCLI} chain set --from-str \ + "chain c BF_HOOK_XDP{ifindex=${NS_IFINDEX}} NEXT \ + rule meta.l3_proto not ipv4 NEXT \ + rule $matcher counter DROP \ + rule NEXT" + (! ping -c 1 -W 0.1 ${NS_IP_ADDR}) +done + +${FROM_NS} ${BFCLI} ruleset flush From 334148c33fe590b85156152bfd0fec2a19218925 Mon Sep 17 00:00:00 2001 From: yaakov-stein Date: Thu, 15 Oct 2026 09:42:19 -0700 Subject: [PATCH 4/4] lib: cgen: omit comparisons for 100% probability Treat non-negated meta.probability 100 as a no-op, avoiding unnecessary random sampling and allowing terminal rules to omit unreachable code. Cover it in the existing XDP regression. --- src/libbpfilter/cgen/program.c | 6 ++++++ tests/e2e/rules/unconditional.sh | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/libbpfilter/cgen/program.c b/src/libbpfilter/cgen/program.c index de2a55ab6..92249fa22 100644 --- a/src/libbpfilter/cgen/program.c +++ b/src/libbpfilter/cgen/program.c @@ -607,6 +607,12 @@ static bool _bf_program_matcher_is_noop(const struct bf_program *program, assert(program); assert(matcher); + if (bf_matcher_get_type(matcher) == BF_MATCHER_META_PROBABILITY) { + const float proba = *(const float *)bf_matcher_payload(matcher); + + return proba == 100.0F && !bf_matcher_get_negate(matcher); + } + if (bf_matcher_get_type(matcher) == BF_MATCHER_SET) { const struct bf_set *set = bf_chain_get_set_for_matcher(program->runtime.chain, matcher); diff --git a/tests/e2e/rules/unconditional.sh b/tests/e2e/rules/unconditional.sh index b32b22cc2..bb15b24b0 100755 --- a/tests/e2e/rules/unconditional.sh +++ b/tests/e2e/rules/unconditional.sh @@ -39,7 +39,7 @@ test "$(get_counter c 5)" -gt 0 test "$(get_counter c 6)" -gt 0 # Comparison no-ops also terminate without emitting the suffix or policy. -for matcher in '(meta.sport) not in {}'; do +for matcher in '(meta.sport) not in {}' 'meta.probability eq 100%'; do ${FROM_NS} ${BFCLI} chain set --from-str \ "chain c BF_HOOK_XDP{ifindex=${NS_IFINDEX}} NEXT \ rule meta.l3_proto not ipv4 NEXT \