On Thu, February 8, 2018 11:24 am, Alexandr Nedvedicky wrote:
> 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
>
I ran into this as well and can at least confirm that this fixes it.
Tim.
> --------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 &&
>
