Hello Klemens.
I took a closer look at your change and related area. Below is an alternate
way to fix the bug you've found.
there are few details worth to note:
ptr_array matters to main ruleset only, because it is the only
ruleset covered by MD5 sum for pfsync.
it looks like we should also recompute the MD5sum if we remove the rule
from the main ruleset
my diff introduces a new member of pr_ruleset structure, which keeps the
array size so we can pass it to free(9f) later.
I have no pfsync deployment readily available for testing. Will be glad
if you can give my diff a try.
thanks a lot
regards
sashan
--------8<--------------8<--------------8<-----------------8<-------
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index a486745bc1c..00a4f70e7db 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -332,8 +332,32 @@ pf_purge_rule(struct pf_rule *rule)
pf_rm_rule(ruleset->rules.active.ptr, rule);
ruleset->rules.active.rcount--;
- TAILQ_FOREACH(rule, ruleset->rules.active.ptr, entries)
- rule->nr = nr++;
+
+ /*
+ * pf_chksum calculation is copied and modified from
+ * pf_setup_pfsync_matching(), which is part of ruleset commit
+ * operation.
+ */
+ if (ruleset == &pf_main_ruleset) {
+ MD5_CTX ctx;
+ u_int8_t digest[PF_MD5_DIGEST_LENGTH];
+
+ MD5Init(&ctx);
+
+ TAILQ_FOREACH(rule, ruleset->rules.active.ptr, entries) {
+ rule->nr = nr++;
+ pf_hash_rule(&ctx, rule);
+ ruleset->rules.active.ptr_array[rule->nr] = rule;
+ }
+
+ MD5Final(digest, &ctx);
+ memcpy(pf_status.pf_chksum, digest,
sizeof(pf_status.pf_chksum));
+ } else {
+ TAILQ_FOREACH(rule, ruleset->rules.active.ptr, entries) {
+ rule->nr = nr++;
+ }
+ }
+
ruleset->rules.active.ticket++;
pf_calc_skip_steps(ruleset->rules.active.ptr);
pf_remove_if_empty_ruleset(ruleset);
@@ -804,6 +828,7 @@ pf_commit_rules(u_int32_t ticket, char *anchor)
{
struct pf_ruleset *rs;
struct pf_rule *rule, **old_array;
+ size_t old_array_sz;
struct pf_rulequeue *old_rules;
int error;
u_int32_t old_rcount;
@@ -827,13 +852,16 @@ pf_commit_rules(u_int32_t ticket, char *anchor)
old_rules = rs->rules.active.ptr;
old_rcount = rs->rules.active.rcount;
old_array = rs->rules.active.ptr_array;
+ old_array_sz = rs->rules.active.array_sz;
rs->rules.active.ptr = rs->rules.inactive.ptr;
rs->rules.active.ptr_array = rs->rules.inactive.ptr_array;
rs->rules.active.rcount = rs->rules.inactive.rcount;
+ rs->rules.active.array_sz = rs->rules.inactive.array_sz;
rs->rules.inactive.ptr = old_rules;
rs->rules.inactive.ptr_array = old_array;
rs->rules.inactive.rcount = old_rcount;
+ rs->rules.inactive.array_sz = old_array_sz;
rs->rules.active.ticket = rs->rules.inactive.ticket;
pf_calc_skip_steps(rs->rules.active.ptr);
@@ -843,10 +871,12 @@ pf_commit_rules(u_int32_t ticket, char *anchor)
while ((rule = TAILQ_FIRST(old_rules)) != NULL)
pf_rm_rule(old_rules, rule);
if (rs->rules.inactive.ptr_array)
- free(rs->rules.inactive.ptr_array, M_TEMP, 0);
+ free(rs->rules.inactive.ptr_array, M_TEMP,
+ rs->rules.inactive.array_sz);
rs->rules.inactive.ptr_array = NULL;
rs->rules.inactive.rcount = 0;
rs->rules.inactive.open = 0;
+ rs->rules.inactive.array_sz = 0;
pf_remove_if_empty_ruleset(rs);
/* queue defs only in the main ruleset */
@@ -864,10 +894,14 @@ pf_setup_pfsync_matching(struct pf_ruleset *rs)
MD5Init(&ctx);
if (rs->rules.inactive.ptr_array)
- free(rs->rules.inactive.ptr_array, M_TEMP, 0);
+ free(rs->rules.inactive.ptr_array, M_TEMP,
+ rs->rules.inactive.array_sz);
rs->rules.inactive.ptr_array = NULL;
+ rs->rules.inactive.array_sz = 0;
if (rs->rules.inactive.rcount) {
+ rs->rules.inactive.array_sz =
+ rs->rules.inactive.rcount * sizeof(caddr_t);
rs->rules.inactive.ptr_array =
mallocarray(rs->rules.inactive.rcount, sizeof(caddr_t),
M_TEMP, M_NOWAIT);
diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index f06a1fa0e07..acdafcbc971 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -926,6 +926,7 @@ struct pf_ruleset {
struct pf_rule **ptr_array;
u_int32_t rcount;
u_int32_t ticket;
+ size_t array_sz;
int open;
} active, inactive;
} rules;
