On Mon, Jul 20, 2020 at 05:07:03PM +0200, Klemens Nanni wrote:
> On Mon, Jul 20, 2020 at 01:14:00PM +0200, Alexandr Nedvedicky wrote:
> > I took a closer look at your change and related area. Below is an alternate
> > way to fix the bug you've found.
> Thanks for bringing it up again, I forgot to reply earlier.
>
> > 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.
> Correct, as is directly seen by the only caller pf_commit_rules():
>
> 819 /* Calculate checksum for the main ruleset */
> 820 if (rs == &pf_main_ruleset) {
> 821 error = pf_setup_pfsync_matching(rs);
> 822 if (error != 0)
> 823 return (error);
> 824 }
>
> > it looks like we should also recompute the MD5sum if we remove the rule
> > from the main ruleset
> Yes, but that is a separate bug, regardless of how we handle checksum
> calculation and rule lookups.
>
> My plan was to tackle this next, but reusing existing code instead:
>
> After my initial diff, pf_setup_pfsync_matching() would merely update
> the checksum, so we could rename the function to something more
> appropiate and call it from pf_purge_rule() instead of duplicating it
> there.
Here is a new diff that amends the previous with the following:
- rename pf_setup_pfsync_matching() to pf_calc_chksum()
- make it void and simplify callees
- rehash in pf_purge_rule() to account for removed `once' rules
> > 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 persued this first (as done in other free() size diffs) but eventually
> opted for the removal of ptr_array for the sake of simplicity.
>
> > I have no pfsync deployment readily available for testing. Will be glad
> > if you can give my diff a try.
> Works as expected in both regards: pfsync picks the right rule and the
> checksum updates after rules expired.
I have tested this with my usual setup where states keep syncing and
`once' rules disappear from the main ruleset on the machine I hit,
compared to -CURRENT however watching `pfctl -vsi | grep Check' no shows
the checksum updating accordingly.
Admins using `once' rules are hopefully aware of this caveat already,
but now the checksum actually indicates out-of-sync rulesets and does
no longer present the same checksum for different rulesets.
Feedback? OK?
Index: if_pfsync.c
===================================================================
RCS file: /cvs/src/sys/net/if_pfsync.c,v
retrieving revision 1.277
diff -u -p -r1.277 if_pfsync.c
--- if_pfsync.c 21 Aug 2020 22:59:27 -0000 1.277
+++ if_pfsync.c 23 Aug 2020 12:09:39 -0000
@@ -500,6 +500,7 @@ pfsync_state_import(struct pfsync_state
struct pfi_kif *kif;
int pool_flags;
int error = ENOMEM;
+ int n = 0;
if (sp->creatorid == 0) {
DPFPRINTF(LOG_NOTICE, "pfsync_state_import: "
@@ -524,9 +525,11 @@ pfsync_state_import(struct pfsync_state
*/
if (sp->rule != htonl(-1) && sp->anchor == htonl(-1) &&
(flags & (PFSYNC_SI_IOCTL | PFSYNC_SI_CKSUM)) && ntohl(sp->rule) <
- pf_main_ruleset.rules.active.rcount)
- r = pf_main_ruleset.rules.active.ptr_array[ntohl(sp->rule)];
- else
+ pf_main_ruleset.rules.active.rcount) {
+ TAILQ_FOREACH(r, pf_main_ruleset.rules.active.ptr, entries)
+ if (ntohl(sp->rule) == n++)
+ break;
+ } else
r = &pf_default_rule;
if ((r->max_states && r->states_cur >= r->max_states))
Index: pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.495
diff -u -p -r1.495 pfvar.h
--- pfvar.h 28 Jul 2020 16:47:42 -0000 1.495
+++ pfvar.h 23 Aug 2020 12:09:39 -0000
@@ -924,7 +924,6 @@ struct pf_ruleset {
struct pf_rulequeue queues[2];
struct {
struct pf_rulequeue *ptr;
- struct pf_rule **ptr_array;
u_int32_t rcount;
u_int32_t ticket;
int open;
Index: pf_ioctl.c
===================================================================
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.354
diff -u -p -r1.354 pf_ioctl.c
--- pf_ioctl.c 21 Jul 2020 14:13:17 -0000 1.354
+++ pf_ioctl.c 23 Aug 2020 13:50:53 -0000
@@ -97,7 +97,7 @@ int pf_rollback_rules(u_int32_t, char
void pf_remove_queues(void);
int pf_commit_queues(void);
void pf_free_queues(struct pf_queuehead *);
-int pf_setup_pfsync_matching(struct pf_ruleset *);
+void pf_calc_chksum(struct pf_ruleset *);
void pf_hash_rule(MD5_CTX *, struct pf_rule *);
void pf_hash_rule_addr(MD5_CTX *, struct pf_rule_addr *);
int pf_commit_rules(u_int32_t, char *);
@@ -337,6 +337,9 @@ pf_purge_rule(struct pf_rule *rule)
ruleset->rules.active.ticket++;
pf_calc_skip_steps(ruleset->rules.active.ptr);
pf_remove_if_empty_ruleset(ruleset);
+
+ if (ruleset == &pf_main_ruleset)
+ pf_calc_chksum(ruleset);
}
u_int16_t
@@ -805,9 +808,8 @@ int
pf_commit_rules(u_int32_t ticket, char *anchor)
{
struct pf_ruleset *rs;
- struct pf_rule *rule, **old_array;
+ struct pf_rule *rule;
struct pf_rulequeue *old_rules;
- int error;
u_int32_t old_rcount;
/* Make sure any expired rules get removed from active rules first. */
@@ -818,23 +820,16 @@ pf_commit_rules(u_int32_t ticket, char *
ticket != rs->rules.inactive.ticket)
return (EBUSY);
- /* Calculate checksum for the main ruleset */
- if (rs == &pf_main_ruleset) {
- error = pf_setup_pfsync_matching(rs);
- if (error != 0)
- return (error);
- }
+ if (rs == &pf_main_ruleset)
+ pf_calc_chksum(rs);
/* Swap rules, keep the old. */
old_rules = rs->rules.active.ptr;
old_rcount = rs->rules.active.rcount;
- old_array = rs->rules.active.ptr_array;
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.inactive.ptr = old_rules;
- rs->rules.inactive.ptr_array = old_array;
rs->rules.inactive.rcount = old_rcount;
rs->rules.active.ticket = rs->rules.inactive.ticket;
@@ -844,9 +839,6 @@ pf_commit_rules(u_int32_t ticket, char *
/* Purge the old rule list. */
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);
- rs->rules.inactive.ptr_array = NULL;
rs->rules.inactive.rcount = 0;
rs->rules.inactive.open = 0;
pf_remove_if_empty_ruleset(rs);
@@ -857,35 +849,23 @@ pf_commit_rules(u_int32_t ticket, char *
return (pf_commit_queues());
}
-int
-pf_setup_pfsync_matching(struct pf_ruleset *rs)
+void
+pf_calc_chksum(struct pf_ruleset *rs)
{
MD5_CTX ctx;
struct pf_rule *rule;
u_int8_t digest[PF_MD5_DIGEST_LENGTH];
MD5Init(&ctx);
- if (rs->rules.inactive.ptr_array)
- free(rs->rules.inactive.ptr_array, M_TEMP, 0);
- rs->rules.inactive.ptr_array = NULL;
if (rs->rules.inactive.rcount) {
- rs->rules.inactive.ptr_array =
- mallocarray(rs->rules.inactive.rcount, sizeof(caddr_t),
- M_TEMP, M_NOWAIT);
-
- if (!rs->rules.inactive.ptr_array)
- return (ENOMEM);
-
TAILQ_FOREACH(rule, rs->rules.inactive.ptr, entries) {
pf_hash_rule(&ctx, rule);
- (rs->rules.inactive.ptr_array)[rule->nr] = rule;
}
}
MD5Final(digest, &ctx);
memcpy(pf_status.pf_chksum, digest, sizeof(pf_status.pf_chksum));
- return (0);
}
int
