Restore counter values on chain update

doc/usage/bfcli.rst

@@ -206,7 +206,7 @@ See below for a list of available hook options.
 ``chain update``
 ~~~~~~~~~~~~~~~~
 
-Update an existing chain. The new chain will atomically update the existing one. Hook options are ignored. The new chain will replace the existing chain with the same name.
+Update an existing chain. The new chain will atomically update the existing one. Hook options are ignored. The new chain will replace the existing chain with the same name. Counters are reset to zero.
 
 If you want to modify the hook options, use ``bfcli chain set`` instead.
 
@@ -242,7 +242,7 @@ If you want to modify the hook options, use ``bfcli chain set`` instead.
 ``chain update-set``
 ~~~~~~~~~~~~~~~~~~~~
 
-Atomically update the content of a named set in a chain using delta operations. This is more efficient than replacing the entire chain when you only need to modify set membership.
+Atomically update the content of a named set in a chain using delta operations. This is more efficient than replacing the entire chain when you only need to modify set membership. Counters are preserved across the update.
 
 **Options**
   - ``--name NAME``: name of the chain containing the set.

src/bpfilter/cgen/cgen.c

@@ -27,6 +27,7 @@
 #include "cgen/dump.h"
 #include "cgen/handle.h"
 #include "cgen/prog/link.h"
+#include "cgen/prog/map.h"
 #include "cgen/program.h"
 #include "ctx.h"
 #include "opts.h"
@@ -334,7 +335,51 @@ int bf_cgen_attach(struct bf_cgen *cgen, const struct bf_ns *ns,
     return r;
 }
 
-int bf_cgen_update(struct bf_cgen *cgen, struct bf_chain **new_chain)
+/**
+ * @brief Transfer all counters from old handle to new handle.
+ *
+ * Copies counter values 1:1 for all rule counters plus policy and error
+ * counters. The old and new chains must have the same number of rules.
+ * Both handles must be loaded.
+ *
+ * @param old_handle Handle with the source counter map. Can't be NULL.
+ * @param new_handle Handle with the destination counter map. Can't be NULL.
+ * @param n_rules Number of rules in the chain.
+ * @return 0 on success, or a negative errno value on failure.
+ */
+static int _bf_cgen_transfer_counters(const struct bf_handle *old_handle,
+                                      struct bf_handle *new_handle,
+                                      size_t n_rules)
+{
+    int r;
+
+    assert(old_handle);
+    assert(new_handle);
+
+    if (!old_handle->cmap || !new_handle->cmap)
+        return bf_err_r(-ENOENT, "missing counter map for counter transfer");
+
+    // n_rules entries for rules, +1 for policy, +1 for errors.
+    for (uint32_t i = 0; i < n_rules + 2; ++i) {
+        struct bf_counter counter;
+
+        r = bf_handle_get_counter(old_handle, i, &counter);
+        if (r)
+            return bf_err_r(r, "failed to read counter %u", i);
+
+        if (!counter.count && !counter.size)
+            continue;
+
+        r = bf_map_set_elem(new_handle->cmap, &i, &counter);
+        if (r)
+            return bf_err_r(r, "failed to write counter %u", i);
+    }
+
+    return 0;
+}
+
+int bf_cgen_update(struct bf_cgen *cgen, struct bf_chain **new_chain,
+                   uint32_t flags)
 {
     _free_bf_program_ struct bf_program *new_prog = NULL;
     _free_bf_handle_ struct bf_handle *new_handle = NULL;
@@ -345,6 +390,9 @@ int bf_cgen_update(struct bf_cgen *cgen, struct bf_chain **new_chain)
     assert(cgen);
     assert(new_chain);
 
+    if (flags & ~BF_FLAGS_MASK(_BF_CGEN_UPDATE_MAX))
+        return bf_err_r(-EINVAL, "unknown update flags: 0x%x", flags);
+
     old_handle = cgen->handle;
 
     if (bf_opts_persist()) {
@@ -371,6 +419,19 @@ int bf_cgen_update(struct bf_cgen *cgen, struct bf_chain **new_chain)
     if (r)
         return bf_err_r(r, "failed to load new program");
 
+    if (flags & BF_FLAG(BF_CGEN_UPDATE_PRESERVE_COUNTERS)) {
+        if (bf_list_size(&cgen->chain->rules) !=
+            bf_list_size(&(*new_chain)->rules)) {
+            return bf_err_r(-EINVAL,
+                            "rule count mismatch for counter transfer");
+        }
+
+        r = _bf_cgen_transfer_counters(old_handle, new_handle,
+                                       bf_list_size(&(*new_chain)->rules));
+        if (r)
+            return bf_err_r(r, "failed to transfer counters");
+    }
+
     if (bf_opts_persist())
         bf_handle_unpin(old_handle, pindir_fd);
 

src/bpfilter/cgen/cgen.h

@@ -129,6 +129,17 @@ int bf_cgen_load(struct bf_cgen *cgen);
 int bf_cgen_attach(struct bf_cgen *cgen, const struct bf_ns *ns,
                    struct bf_hookopts **hookopts);
 
+/**
+ * Flags to control the behavior of `bf_cgen_update`.
+ */
+enum bf_cgen_update_flags
+{
+    /** Preserve counter values from the old program. */
+    BF_CGEN_UPDATE_PRESERVE_COUNTERS,
+
+    _BF_CGEN_UPDATE_MAX,
+};
+
 /**
  * Update the program attached to the hook.
  *
@@ -142,9 +153,11 @@ int bf_cgen_attach(struct bf_cgen *cgen, const struct bf_ns *ns,
  * @param cgen Codegen to update. Can't be NULL.
  * @param new_chain Chain containing the new rules, sets, and policy.
  *        Can't be NULL.
+ * @param flags Flags to control update behavior. 0 if no flags.
  * @return 0 on success, or negative errno value on failure.
  */
-int bf_cgen_update(struct bf_cgen *cgen, struct bf_chain **new_chain);
+int bf_cgen_update(struct bf_cgen *cgen, struct bf_chain **new_chain,
+                   uint32_t flags);
 
 /**
  * Detach a program from the kernel.

src/bpfilter/xlate/cli.c

@@ -509,7 +509,7 @@ int _bf_cli_chain_update(const struct bf_request *request,
     if (!cgen)
         return -ENOENT;
 
-    r = bf_cgen_update(cgen, &chain);
+    r = bf_cgen_update(cgen, &chain, 0);
     if (r)
         return -EINVAL;
 
@@ -607,7 +607,8 @@ int _bf_cli_chain_update_set(const struct bf_request *request,
     if (r)
         return bf_err_r(r, "failed to calculate set difference");
 
-    r = bf_cgen_update(cgen, &new_chain);
+    r = bf_cgen_update(cgen, &new_chain,
+                       BF_FLAG(BF_CGEN_UPDATE_PRESERVE_COUNTERS));
     if (r)
         return bf_err_r(r, "failed to update chain with new set data");
 

src/bpfilter/xlate/ipt/ipt.c

@@ -718,7 +718,7 @@ static int _bf_ipt_ruleset_set(const struct bf_request *req)
 
             TAKE_PTR(cgen);
         } else {
-            r = bf_cgen_update(cgen, &chain);
+            r = bf_cgen_update(cgen, &chain, 0);
             if (r) {
                 TAKE_PTR(cgen);
                 bf_err_r(

src/libbpfilter/include/bpfilter/helper.h

@@ -116,6 +116,13 @@ extern const char *strerrordesc_np(int errnum);
  */
 #define BF_FLAG(n) (1 << (n))
 
+/**
+ * @brief Generate a bitmask covering all valid flags below `max_value`.
+ *
+ * @return Mask with bits 0 through max_value-1 set.
+ */
+#define BF_FLAGS_MASK(max_value) ((1ULL << (max_value)) - 1)
+
 #define bf_packed __attribute__((packed))
 #define bf_aligned(x) __attribute__((aligned(x)))
 #define bf_unused __attribute__((unused))

tests/e2e/cli/chain_update.sh

@@ -23,3 +23,17 @@ ${FROM_NS} bfcli chain update --from-str "chain chain_load_xdp_3 BF_HOOK_XDP ACC
 ${FROM_NS} bfcli chain update --name chain_load_xdp_3 --from-str "chain chain_load_xdp_3 BF_HOOK_XDP{ifindex=${NS_IFINDEX}} ACCEPT"
 ping -c 1 -W 0.1 ${NS_IP_ADDR}
 ${FROM_NS} bfcli chain flush --name chain_load_xdp_3
+
+# Counters are reset after chain update
+${FROM_NS} bfcli chain set --from-str "chain chain_load_xdp_4 BF_HOOK_XDP{ifindex=${NS_IFINDEX}} ACCEPT
+    rule ip4.proto icmp counter DROP
+"
+(! ping -c 1 -W 0.1 ${NS_IP_ADDR})
+counter=$(${FROM_NS} bpftool map dump pinned ${WORKDIR}/bpf/bpfilter/chain_load_xdp_4/bf_cmap | jq '.[0].value.count')
+test "$counter" = "1"
+${FROM_NS} bfcli chain update --from-str "chain chain_load_xdp_4 BF_HOOK_XDP ACCEPT
+    rule ip4.proto icmp counter DROP
+"
+counter=$(${FROM_NS} bpftool map dump pinned ${WORKDIR}/bpf/bpfilter/chain_load_xdp_4/bf_cmap | jq '.[0].value.count')
+test "$counter" = "0"
+${FROM_NS} bfcli chain flush --name chain_load_xdp_4

tests/e2e/cli/chain_update_set.sh

@@ -202,3 +202,36 @@ chain_output=$(${FROM_NS} bfcli chain get --name test_xdp)
 echo "$chain_output"
 echo "$chain_output" | grep -q '10.0.0.1'
 echo "$chain_output" | grep -q '10.0.0.2'
+
+# Counters are preserved after update-set for both set and non-set rules
+${FROM_NS} bfcli chain set --from-str "chain test_xdp BF_HOOK_XDP{ifindex=${NS_IFINDEX}} ACCEPT
+    set blocked_ips (ip4.saddr) in {
+        10.0.0.1
+    }
+    rule
+        (ip4.saddr) in blocked_ips
+        counter
+        DROP
+    rule
+        ip4.proto icmp
+        counter
+        DROP
+"
+
+(! ping -c 1 -W 0.1 ${NS_IP_ADDR})
+${FROM_NS} bfcli chain update-set \
+    --name test_xdp \
+    --set-name blocked_ips \
+    --add ${HOST_IP_ADDR}
+
+(! ping -c 1 -W 0.1 ${NS_IP_ADDR})
+${FROM_NS} bfcli chain update-set \
+    --name test_xdp \
+    --set-name blocked_ips \
+    --add 10.0.0.2
+
+counter=$(${FROM_NS} bpftool map dump pinned ${WORKDIR}/bpf/bpfilter/test_xdp/bf_cmap | jq '.[0].value.count')
+test "$counter" = "1"
+counter=$(${FROM_NS} bpftool map dump pinned ${WORKDIR}/bpf/bpfilter/test_xdp/bf_cmap | jq '.[1].value.count')
+test "$counter" = "1"
+${FROM_NS} bfcli chain flush --name test_xdp