From: Stuart Henderson Subject: Re: wrong reference to anchor/rule may appear in pflog (or state) To: Alexandr Nedvedicky Cc: tech@openbsd.org Date: Thu, 20 Jun 2024 12:42:20 +0100 From my understanding of the description, I think this diff makes sense. It certainly fixes quite an annoying problem that makes debugging rulesets very tricky since 7.3 if you use anchors (e.g. for ftp-proxy). I don't have full understanding of the anchor code, but OK sthen FWIW. On 2024/06/17 10:43, Alexandr Nedvedicky wrote: > ping! OK from anyone? > > thanks and > regards > sashan > > > On Fri, May 24, 2024 at 09:23:54AM +0200, Alexandr Nedvedicky wrote: > > Hello, > > > > The issue has been brought up and analyzed by Giannis Kapetanakis here [1]. > > Giannis went back in history and found a culprit: > > > > pf.c (2023/01/05) 1.1169 > > > > The change in 1.1169 fixes stack overflow which may be triggered with deeply > > nested anchors. Unfortunately that fix introduces issue reported by Giannis. > > To trigger it one has to use rule anchors like for example here: > > > > @0 match in all scrub (no-df random-id) > > @1 pass out log proto tcp from self to any port 12345 > > @2 anchor "relayd/*" > > @3 anchor "test" { > > @0 pass out log proto tcp from self to any port 12346 > > @1 anchor "foo" { > > @0 pass out log proto tcp from self to any port 12348 > > } > > @2 pass out log proto tcp from self to any port 12349 > > } > > @4 pass out log proto tcp from self to any port 12347 > > > > Rules above use the same numbering style which is also used by command > > 'pfctl -sr -v' > > > > if packet is sent to remote port 12349 it matches the rule @2 > > in anchor @3 ('test'). pflog (and also state shown by 'pfctl -ss -vv') > > should report that in form: > > anchor 3, rule 2 > > however the pf in current reports this: > > anchor 1, rule 2 > > > > The diff below is based on idea of simple one-liner tweak > > proposed by Giannis off-list. His idea is to just forget > > the reference to anchor as pf backtraces back towards root. > > Such fix would work if there would be no rule @2 in anchor @3. > > For general case we need to save/restore reference to anchor > > as we traverse the rulesets. > > > > Note: I think pf never recorded a full path from root to rule, > > pf just reports leaf anchor where the rule belongs to. To record > > the full path a follow-up change is needed. > > > > OK to commit diff below? > > > > thanks and > > regards > > sashan > > > > [1] https://marc.info/?t=171611737500001&r=1&w=2 > > > > --------8<---------------8<---------------8<------------------8<-------- > > diff --git a/sys/net/pf.c b/sys/net/pf.c > > index 8591b044e43..007a00c6bc6 100644 > > --- a/sys/net/pf.c > > +++ b/sys/net/pf.c > > @@ -3666,8 +3666,8 @@ pf_anchor_stack_top(void) > > } > > > > int > > -pf_anchor_stack_push(struct pf_ruleset *rs, struct pf_rule *r, > > - struct pf_anchor *child, int jump_target) > > +pf_anchor_stack_push(struct pf_ruleset *rs, struct pf_rule *anchor, > > + struct pf_rule *r, struct pf_anchor *child, int jump_target) > > { > > struct pf_anchor_stackframe *stack; > > struct pf_anchor_stackframe *top_sf = pf_anchor_stack_top(); > > @@ -3677,6 +3677,7 @@ pf_anchor_stack_push(struct pf_ruleset *rs, struct pf_rule *r, > > return (-1); > > > > top_sf->sf_rs = rs; > > + top_sf->sf_anchor = anchor; > > top_sf->sf_r = r; > > top_sf->sf_child = child; > > top_sf->sf_jump_target = jump_target; > > @@ -3693,8 +3694,8 @@ pf_anchor_stack_push(struct pf_ruleset *rs, struct pf_rule *r, > > } > > > > int > > -pf_anchor_stack_pop(struct pf_ruleset **rs, struct pf_rule **r, > > - struct pf_anchor **child, int *jump_target) > > +pf_anchor_stack_pop(struct pf_ruleset **rs, struct pf_rule **anchor, > > + struct pf_rule **r, struct pf_anchor **child, int *jump_target) > > { > > struct pf_anchor_stackframe *top_sf = pf_anchor_stack_top(); > > struct pf_anchor_stackframe *stack; > > @@ -3710,6 +3711,7 @@ pf_anchor_stack_pop(struct pf_ruleset **rs, struct pf_rule **r, > > __func__); > > > > *rs = top_sf->sf_rs; > > + *anchor = top_sf->sf_anchor; > > *r = top_sf->sf_r; > > *child = top_sf->sf_child; > > *jump_target = top_sf->sf_jump_target; > > @@ -4306,25 +4308,27 @@ enter_ruleset: > > if (r->quick) > > return (PF_TEST_QUICK); > > } else { > > - ctx->a = r; > > ctx->aruleset = &r->anchor->ruleset; > > if (r->anchor_wildcard) { > > RB_FOREACH(child, pf_anchor_node, > > &r->anchor->children) { > > - if (pf_anchor_stack_push(ruleset, r, > > - child, PF_NEXT_CHILD) != 0) > > + if (pf_anchor_stack_push(ruleset, > > + ctx->a, r, child, > > + PF_NEXT_CHILD) != 0) > > return (PF_TEST_FAIL); > > > > + ctx->a = r; > > ruleset = &child->ruleset; > > goto enter_ruleset; > > next_child: > > continue; /* with RB_FOREACH() */ > > } > > } else { > > - if (pf_anchor_stack_push(ruleset, r, child, > > - PF_NEXT_RULE) != 0) > > + if (pf_anchor_stack_push(ruleset, ctx->a, > > + r, child, PF_NEXT_RULE) != 0) > > return (PF_TEST_FAIL); > > > > + ctx->a = r; > > ruleset = &r->anchor->ruleset; > > child = NULL; > > goto enter_ruleset; > > @@ -4335,7 +4339,9 @@ next_rule: > > r = TAILQ_NEXT(r, entries); > > } > > > > - if (pf_anchor_stack_pop(&ruleset, &r, &child, &target) == 0) { > > + if (pf_anchor_stack_pop(&ruleset, &ctx->a, &r, &child, > > + &target) == 0) { > > + > > /* stop if any rule matched within quick anchors. */ > > if (r->quick == PF_TEST_QUICK && *ctx->am == r) > > return (PF_TEST_QUICK); > > diff --git a/sys/net/pfvar_priv.h b/sys/net/pfvar_priv.h > > index 7a420bfa308..559273046ee 100644 > > --- a/sys/net/pfvar_priv.h > > +++ b/sys/net/pfvar_priv.h > > @@ -321,6 +321,7 @@ struct pf_pdesc { > > > > struct pf_anchor_stackframe { > > struct pf_ruleset *sf_rs; > > + struct pf_rule *sf_anchor; > > union { > > struct pf_rule *u_r; > > struct pf_anchor_stackframe *u_stack_top; > > >