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 && >