To my point of view this deserves an errata.

Apart from pflog wrong numbering, you also have a broken pfctl -ss -vv

You see states belonging to different (rule) numbers than they really are.

It puzzled me a lot trying to understand why I had certain states.
I though something was wrong with my firewall rules.

G

On 20/06/2024 14:42, Stuart Henderson wrote:
>  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;
>>>