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