Allow Unconditional Terminal Rules

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.
 

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``
 ~~~~~~~~~~~~~~~

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).

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]

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

src/libbpfilter/cgen/program.c

@@ -591,21 +591,28 @@ 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)
 {
     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);
@@ -631,9 +638,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 +730,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 +884,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 +900,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 +934,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)

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));

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);
 

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.

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)

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 {}' '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 \
+         rule $matcher counter DROP \
+         rule NEXT"
+    (! ping -c 1 -W 0.1 ${NS_IP_ADDR})
+done
+
+${FROM_NS} ${BFCLI} ruleset flush