Hello,
Matthias Pitzl discovered a regression introduced by my earlier commit [1].
Matthias has noticed the pflogd output changes for ruleset here:
--------8<---------------8<---------------8<------------------8<--------
block out log quick from any to 1.1.1.1
block out log quick from any to 1.1.1.2
anchor log_test {
block out log quick from any to 2.1.1.1
block out log quick from any to 2.1.1.2
}
block out log quick from any to 3.1.1.1
block out log quick from any to 3.1.1.2
--------8<---------------8<---------------8<------------------8<--------
pinging addresses used in rules above Matthias noticed the rule numbers
and anchors in log are incorrect:
Feb 7 16:34:47 sys pf: rule 0/(match) [usid 0, pid 95203] block out ... >
1.1.1.1
Feb 7 16:34:48 sys pf: rule 1/(match) [usid 0, pid 95203] block out ... >
1.1.1.2
Feb 7 16:34:50 sys pf: rule 2.log_test.0s/(match) [uid 0, pid 95203] block
out on ... > 2.1.1.1
Feb 7 16:34:52 sys pf: rule 2.log_test.1s/(match) [uid 0, pid 95203] block
out on ... > 2.1.1.2
Feb 7 16:34:55 sys pf: rule 2/(match) [usid 0, pid 95203] block out on ...
> 3.1.1.1
Feb 7 16:34:57 sys pf: rule 2/(match) [uid 0, pid 95203] block out on ...
> 3.1.1.2
in output above the entries for 3.1.1.1 and 3.1.1.2 have a wrong rule number.
we should see rule number 3 for 3.1.1.1 and 4 for 3.1.1.2
With joint effort we could identify two problems in my earlier change.
1) pf_match_rule() must remember anchor rule and its ruleset
kept in ctx, before it updates ctx for descent:
3689 ctx->a = r; /* remember anchor */
3690 ctx->aruleset = ruleset; /* and its
ruleset */
3691 if (pf_step_into_anchor(ctx, r) != PF_TEST_OK)
3692 break;
once pf_step_into_anchor() returns and we are supposed to continue
with rule evaluation, we are better to restore ctx->a and ctx->aruleset,
which match our nesting level.
2) PFLOG_PACKET() called from pf_test_rule():
3789 #if NPFLOG > 0
3790 if (r->log)
3791 PFLOG_PACKET(pd, ctx.reason, r, ctx.a, ruleset, NULL);
3792 if (ctx.act.log & PF_LOG_MATCHES)
3793 pf_log_matches(pd, r, ctx.a, ruleset, &ctx.rules);
3794 #endif /* NPFLOG > 0 */
uses anchor kept in ctx, instead of local variable, which holds anchor for
matching rule.
OK?
thanks and
regards
sasha
[1]
https://github.com/openbsd/src/commit/e236f0fa7b23e94c7258b2055ec8e7c9804957c7#diff-9517dfce4e8db974781a4536fd38cfc1
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 51a91114c74..75d4e7158c2 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -3108,9 +3108,9 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct
pf_rule *r)
rv = pf_match_rule(ctx, &child->ruleset);
if ((rv == PF_TEST_QUICK) || (rv == PF_TEST_FAIL)) {
/*
- * we either hit a rule qith quick action
+ * we either hit a rule with quick action
* (more likely), or hit some runtime
- * error (e.g. pool_get() faillure).
+ * error (e.g. pool_get() failure).
*/
break;
}
@@ -3497,6 +3497,8 @@ enum pf_test_status
pf_match_rule(struct pf_test_ctx *ctx, struct pf_ruleset *ruleset)
{
struct pf_rule *r;
+ struct pf_rule *save_a;
+ struct pf_ruleset *save_aruleset;
r = TAILQ_FIRST(ruleset->rules.active.ptr);
while (r != NULL) {
@@ -3682,11 +3684,18 @@ pf_match_rule(struct pf_test_ctx *ctx, struct
pf_ruleset *ruleset)
break;
}
} else {
+ save_a = ctx->a;
+ save_aruleset = ctx->aruleset;
ctx->a = r; /* remember anchor */
ctx->aruleset = ruleset; /* and its ruleset */
- if (pf_step_into_anchor(ctx, r) != PF_TEST_OK) {
+ /*
+ * Note: we don't need to restore if we are not going
+ * to continue with ruleset evaluation.
+ */
+ if (pf_step_into_anchor(ctx, r) != PF_TEST_OK)
break;
- }
+ ctx->a = save_a;
+ ctx->aruleset = save_aruleset;
}
r = TAILQ_NEXT(r, entries);
}
@@ -3768,8 +3777,6 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
struct pf_state **sm,
ruleset = *ctx.rsm;/* ruleset of the anchor defined by the rule 'a' */
ctx.aruleset = ctx.arsm;/* ruleset of the 'a' rule itself */
-
-
/* apply actions for last matching pass/block rule */
pf_rule_to_actions(r, &ctx.act);
if (r->rule_flag & PFRULE_AFTO)
@@ -3782,9 +3789,9 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
struct pf_state **sm,
#if NPFLOG > 0
if (r->log)
- PFLOG_PACKET(pd, ctx.reason, r, ctx.a, ruleset, NULL);
+ PFLOG_PACKET(pd, ctx.reason, r, a, ruleset, NULL);
if (ctx.act.log & PF_LOG_MATCHES)
- pf_log_matches(pd, r, ctx.a, ruleset, &ctx.rules);
+ pf_log_matches(pd, r, a, ruleset, &ctx.rules);
#endif /* NPFLOG > 0 */
if (pd->virtual_proto != PF_VPROTO_FRAGMENT &&
