lib: matcher: make set ownership explicit

src/bfcli/parser.y

@@ -531,7 +531,6 @@ matcher         : matcher_type negate matcher_op RAW_PAYLOAD
                 | SET_TYPE negate matcher_op SET_RAW_PAYLOAD
                 {
                      _free_bf_matcher_ struct bf_matcher *matcher = NULL;
-                     _free_bf_set_ struct bf_set *set = NULL;
                     _cleanup_free_ const char *raw_key = $1;
                     _cleanup_free_ const char *payload = $4;
                     enum bf_matcher_op op = $3;
@@ -540,18 +539,10 @@ matcher         : matcher_type negate matcher_op RAW_PAYLOAD
                     if (op != BF_MATCHER_IN)
                         bf_parse_err("only the 'in' operator is supported for sets");
 
-                    r = bf_set_new_from_raw(&set, NULL, raw_key, payload);
-                    if (r)
-                        bf_parse_err("failed to create new set");
-
-                    r = bf_matcher_new(&matcher, BF_MATCHER_SET, BF_MATCHER_IN, set, sizeof(*set), $2);
+                    r = bf_matcher_new_for_anon_set_from_raw(&matcher, raw_key, payload, $2);
                     if (r)
                         bf_parse_err("failed to create a new matcher");
 
-                    r = bf_hashset_add(&ruleset->sets, (void **)&set);
-                    if (r)
-                        bf_parse_err("failed to add new set to the ruleset");
-
                      $$ = TAKE_PTR(matcher);
                 }
                 | SET_TYPE negate matcher_op STRING
@@ -578,7 +569,7 @@ matcher         : matcher_type negate matcher_op RAW_PAYLOAD
                     if (found_set->n_comps != test_key->n_comps || memcmp(found_set->key, test_key->key, found_set->n_comps * sizeof(enum bf_matcher_type)))
                         bf_parse_err("using named set '%s', but key doesn't match", name);
 
-                    r = bf_matcher_new(&matcher, BF_MATCHER_SET, BF_MATCHER_IN, found_set, sizeof(*found_set), $2);
+                    r = bf_matcher_new_for_named_set(&matcher, found_set, $2);
                     if (r)
                         bf_parse_err("failed to create a new matcher");
 

src/libbpfilter/cgen/cgroup_sock_addr.c

@@ -294,8 +294,7 @@ static int _bf_cgroup_sock_addr_generate_set(struct bf_program *program,
 
     set = bf_chain_get_set_for_matcher(program->runtime.chain, matcher);
     if (!set) {
-        return bf_err_r(-ENOENT, "set #%u not found in %s",
-                        *(uint32_t *)bf_matcher_payload(matcher),
+        return bf_err_r(-ENOENT, "matcher set not found in %s",
                         program->runtime.chain->name);
     }
 

src/libbpfilter/cgen/matcher/set.c

@@ -28,8 +28,7 @@ int bf_set_generate_map_lookup(struct bf_program *program,
 
     set = bf_chain_get_set_for_matcher(program->runtime.chain, matcher);
     if (!set) {
-        return bf_err_r(-ENOENT, "set '%s' not found in %s",
-                        (const char *)bf_matcher_payload(matcher),
+        return bf_err_r(-ENOENT, "matcher set not found in %s",
                         program->runtime.chain->name);
     }
 

src/libbpfilter/cgen/packet.c

@@ -338,8 +338,7 @@ static int _bf_matcher_pkt_generate_set(struct bf_program *program,
 
     set = bf_chain_get_set_for_matcher(program->runtime.chain, matcher);
     if (!set) {
-        return bf_err_r(-ENOENT, "set #%u not found in %s",
-                        *(uint32_t *)bf_matcher_payload(matcher),
+        return bf_err_r(-ENOENT, "matcher set not found in %s",
                         program->runtime.chain->name);
     }
 

src/libbpfilter/cgen/program.c

@@ -164,6 +164,52 @@ _bf_program_find_set_group(const struct bf_program *program,
     return NULL;
 }
 
+static int _bf_program_add_set_to_group(struct bf_program *program,
+                                        const struct bf_set *set)
+{
+    _free_bf_set_group_ struct bf_set_group *group = NULL;
+    int r;
+
+    assert(program);
+    assert(set);
+
+    if (bf_hashset_is_empty(&set->elems))
+        return 0;
+
+    /* If a group with a matching key already exists, reuse it. For LPM trie,
+     * sets can't be grouped (as we can't define which rule matches on which
+     * prefix), so they always are the sole member of their group. */
+    if (!set->use_trie) {
+        bf_list_foreach (&program->set_groups, group_node) {
+            struct bf_set_group *group = bf_list_node_get_data(group_node);
+            const struct bf_set *head =
+                bf_list_node_get_data(bf_list_get_head(&group->sets));
+
+            if (bf_set_same_key(set, head)) {
+                r = bf_list_add_tail(&group->sets, (void *)set);
+                if (r)
+                    return bf_err_r(r, "failed to add set to existing group");
+
+                return 0;
+            }
+        }
+    }
+
+    r = _bf_set_group_new(&group);
+    if (r)
+        return bf_err_r(r, "failed to allocate set group");
+
+    r = bf_list_add_tail(&group->sets, (void *)set);
+    if (r)
+        return bf_err_r(r, "failed to seed set group");
+
+    r = bf_list_push(&program->set_groups, (void **)&group);
+    if (r)
+        return bf_err_r(r, "failed to register set group");
+
+    return 0;
+}
+
 static int _bf_program_build_set_groups(struct bf_program *program)
 {
     assert(program);
@@ -172,48 +218,30 @@ static int _bf_program_build_set_groups(struct bf_program *program)
     program->set_groups = bf_list_default(_bf_set_group_free, NULL);
 
     bf_hashset_foreach (&program->runtime.chain->sets, set_node) {
-        struct bf_set *set = set_node->data;
-        struct bf_set_group *match = NULL;
-        int r;
-
-        if (bf_hashset_is_empty(&set->elems))
-            continue;
+        int r = _bf_program_add_set_to_group(program, set_node->data);
+        if (r)
+            return r;
+    }
 
-        /* LPM trie sets are not grouped: BPF LPM trie lookup always
-         * returns the longest-prefix match, so the read-modify-write
-         * step in _bf_program_load_sets_maps() can't preserve the
-         * per-set bitmask when prefixes overlap. */
-        if (!set->use_trie) {
-            bf_list_foreach (&program->set_groups, group_node) {
-                struct bf_set_group *group = bf_list_node_get_data(group_node);
-                const struct bf_set *head =
-                    bf_list_node_get_data(bf_list_get_head(&group->sets));
-
-                if (bf_set_same_key(set, head)) {
-                    match = group;
-                    break;
-                }
-            }
-        }
+    bf_list_foreach (&program->runtime.chain->rules, rule_node) {
+        const struct bf_rule *rule = bf_list_node_get_data(rule_node);
 
-        if (match) {
-            r = bf_list_add_tail(&match->sets, set);
-            if (r)
-                return bf_err_r(r, "failed to add set to existing group");
-        } else {
-            _free_bf_set_group_ struct bf_set_group *new_group = NULL;
+        bf_list_foreach (&rule->matchers, matcher_node) {
+            const struct bf_matcher *matcher =
+                bf_list_node_get_data(matcher_node);
+            const struct bf_set *set;
+            int r;
 
-            r = _bf_set_group_new(&new_group);
-            if (r)
-                return bf_err_r(r, "failed to allocate set group");
+            if (bf_matcher_get_type(matcher) != BF_MATCHER_SET)
+                continue;
 
-            r = bf_list_add_tail(&new_group->sets, set);
-            if (r)
-                return bf_err_r(r, "failed to seed set group");
+            memcpy(&set, bf_matcher_payload(matcher), sizeof(struct bf_set *));
+            if (!bf_set_is_anonymous(set))
+                continue;
 
-            r = bf_list_push(&program->set_groups, (void **)&new_group);
+            r = _bf_program_add_set_to_group(program, set);
             if (r)
-                return bf_err_r(r, "failed to register set group");
+                return r;
         }
     }
 

src/libbpfilter/chain.c

@@ -292,6 +292,15 @@ int bf_chain_new(struct bf_chain **chain, const char *name, enum bf_hook hook,
         return bf_err_r(-EINVAL, "invalid policy '%s'",
                         bf_verdict_to_str(policy));
 
+    if (sets) {
+        bf_hashset_foreach (sets, set_node) {
+            const struct bf_set *set = set_node->data;
+
+            if (!bf_set_is_valid_name(set->name))
+                return bf_err_r(-EINVAL, "chain sets must have valid names");
+        }
+    }
+
     _chain = calloc(1, sizeof(*_chain));
     if (!_chain)
         return -ENOMEM;
@@ -317,8 +326,13 @@ int bf_chain_new(struct bf_chain **chain, const char *name, enum bf_hook hook,
     }
 
     r = _bf_chain_validate_rules(_chain);
-    if (r)
+    if (r) {
+        if (rules)
+            *rules = bf_list_move(&_chain->rules);
+        if (sets)
+            *sets = bf_hashset_move(&_chain->sets);
         return r;
+    }
 
     *chain = TAKE_PTR(_chain);
 
@@ -332,8 +346,8 @@ int bf_chain_new_from_pack(struct bf_chain **chain, bf_rpack_node_t node)
     enum bf_hook hook;
     enum bf_verdict policy;
     bf_rpack_node_t array, array_node;
-    _clean_bf_list_ bf_list rules = bf_list_default(bf_rule_free, bf_rule_pack);
     _clean_bf_hashset_ bf_hashset sets = bf_set_hashset_default();
+    _clean_bf_list_ bf_list rules = bf_list_default(bf_rule_free, bf_rule_pack);
     int r;
 
     r = bf_rpack_kv_str(node, "name", &name);
@@ -358,6 +372,9 @@ int bf_chain_new_from_pack(struct bf_chain **chain, bf_rpack_node_t node)
         if (r)
             return bf_err_r(r, "failed to unpack bf_set into bf_chain.sets");
 
+        if (bf_set_is_anonymous(set))
+            return bf_err_r(-EINVAL, "anonymous set outside a matcher");
+
         r = bf_hashset_add(&sets, (void **)&set);
         if (r)
             return bf_err_r(r, "failed to add unpacked set to chain");
@@ -369,7 +386,8 @@ int bf_chain_new_from_pack(struct bf_chain **chain, bf_rpack_node_t node)
     bf_rpack_array_foreach (array, array_node) {
         _free_bf_rule_ struct bf_rule *rule = NULL;
 
-        r = bf_list_emplace(&rules, bf_rule_new_from_pack, rule, array_node);
+        r = bf_list_emplace(&rules, bf_rule_new_from_pack, rule, array_node,
+                            &sets);
         if (r) {
             return bf_err_r(r, "failed to unpack bf_rule into bf_chain.rules");
         }
@@ -391,8 +409,8 @@ void bf_chain_free(struct bf_chain **chain)
     if (!*chain)
         return;
 
-    bf_hashset_clean(&(*chain)->sets);
     bf_list_clean(&(*chain)->rules);
+    bf_hashset_clean(&(*chain)->sets);
     BF_FREEP(&(*chain)->name);
     BF_FREEP(chain);
 }
@@ -491,20 +509,29 @@ int bf_chain_add_set(struct bf_chain *chain, struct bf_set *set)
 {
     assert(chain && set);
 
+    if (!bf_set_is_valid_name(set->name))
+        return bf_err_r(-EINVAL, "only named sets can be added to a chain");
+
     return bf_hashset_add(&chain->sets, (void **)&set);
 }
 
 const struct bf_set *
 bf_chain_get_set_for_matcher(const struct bf_chain *chain,
                              const struct bf_matcher *matcher)
 {
+    const struct bf_set *set;
+
     assert(chain);
     assert(matcher);
 
     if (bf_matcher_get_type(matcher) != BF_MATCHER_SET)
         return NULL;
 
-    return bf_chain_get_set_by_name(chain, bf_matcher_payload(matcher));
+    memcpy(&set, bf_matcher_payload(matcher), sizeof(struct bf_set *));
+    if (bf_set_is_anonymous(set))
+        return set;
+
+    return bf_hashset_get(&chain->sets, set) == set ? set : NULL;
 }
 
 static struct bf_set *_bf_chain_get_set_by_name(struct bf_chain *chain,
@@ -612,6 +639,29 @@ int bf_chain_new_from_copy(struct bf_chain **dest, const struct bf_chain *src)
         if (r)
             return r;
 
+        bf_list_foreach (&rule->matchers, matcher_node) {
+            struct bf_matcher *matcher = bf_list_node_get_data(matcher_node);
+            const struct bf_set *set;
+
+            if (bf_matcher_get_type(matcher) != BF_MATCHER_SET)
+                continue;
+
+            set = bf_chain_get_set_for_matcher(src, matcher);
+            if (!set)
+                return bf_err_r(-ENOENT,
+                                "matcher set not found in source chain");
+            if (!set->name)
+                continue;
+
+            set = bf_chain_get_set_by_name(copy, set->name);
+            if (!set)
+                return bf_err_r(-ENOENT,
+                                "matcher set not found in copied chain");
+
+            memcpy((void *)bf_matcher_payload(matcher), &set,
+                   sizeof(struct bf_set *));
+        }
+
         r = bf_chain_add_rule(copy, rule);
         if (r)
             return r;

src/libbpfilter/include/bpfilter/chain.h

@@ -55,6 +55,8 @@ struct bf_chain
     uint8_t flags;
     enum bf_hook hook;
     enum bf_verdict policy;
+
+    /// Owning collection of named sets. Anonymous sets belong to matchers.
     bf_hashset sets;
     bf_list rules;
 
@@ -77,7 +79,7 @@ struct bf_chain
  * @param name Name of the chain. Can't be NULL.
  * @param hook Expected hook to attach the chain to.
  * @param policy Default action of the chain if no rule matched.
- * @param sets Owning collection of sets used by `rules`, initialized with
+ * @param sets Owning collection of named sets, initialized with
  *        bf_set_hashset_default(). Names must be unique within the chain.
  * @param rules List of rules.
  * @return 0 on success, or negative errno value on failure.
@@ -136,11 +138,10 @@ void bf_chain_dump(const struct bf_chain *chain, prefix_t *prefix);
 int bf_chain_add_rule(struct bf_chain *chain, struct bf_rule *rule);
 
 /**
- * @brief Insert a set into a chain.
+ * @brief Insert a named set into a chain.
  *
  * The chain will own the set and is responsible for freeing it. To reference
- * the set in a matcher, pass the set pointer and sizeof(*set) to
- * bf_matcher_new(). The matcher stores a copy of the set's name.
+ * the set in a matcher, use `bf_matcher_new_for_named_set`.
  *
  * @param chain Chain to insert the rule into. Can't be NULL.
  * @param set Set to insert into the chain. Can't be NULL.
@@ -155,6 +156,7 @@ int bf_chain_add_set(struct bf_chain *chain, struct bf_set *set);
  * @param matcher Matching filtering on a set. Can't be NULL.
  * @return The set `matcher` filters on, or NULL if the set can't be found or
  *         if `matcher->type` is not `BF_MATCHER_SET`.
+ *         The pointer is borrowed. Anonymous sets belong to their matcher.
  */
 const struct bf_set *
 bf_chain_get_set_for_matcher(const struct bf_chain *chain,