From: Alexandr Nedvedicky <sashan@fastmail.net>
Subject: Re: diff: pf least-states
To: YASUOKA Masahiko <yasuoka@openbsd.org>
Cc: sashan@openbsd.org, tech@openbsd.org
Date: Fri, 15 Aug 2025 10:58:13 +0200

Hello Yasuoka-san,

thank you for a good work. I like your change. I just have some
minor suggestion to clean up a logic in pf_postprocess_addr().
see further below.

On Thu, Aug 14, 2025 at 08:08:45PM +0900, YASUOKA Masahiko wrote:
</snip>
> @@ -879,70 +893,102 @@ pf_get_transaddr_af(struct pf_rule *r, s
>  int
>  pf_postprocess_addr(struct pf_state *cur)
>  {
</snip>
> +	struct pf_addr		*addr;
> +	struct pf_pool		*rpool, *natpl = NULL, *rdrpl = NULL;
> +	struct pf_rule_item	*ri;
> +	int			 ret = 0;
> +
> +	/* decrease the states counter in pool for "least-states" */
> +	if (cur->natrule.ptr != NULL) { /* this is the final */
> +		if (cur->natrule.ptr->nat.addr.type != PF_ADDR_NONE)
> +			natpl = &cur->natrule.ptr->nat;
> +		if (cur->natrule.ptr->rdr.addr.type != PF_ADDR_NONE)
> +			rdrpl = &cur->natrule.ptr->rdr;
> +	}
> +	if (natpl == NULL || rdrpl == NULL) {
> +		/* nat or rdr might be done by a previous "match" */
> +		SLIST_FOREACH(ri, &cur->match_rules, entry) {
> +			/* first match since the list order is reversed */
> +			if (natpl == NULL &&
> +			    ri->r->nat.addr.type != PF_ADDR_NONE)
> +				natpl = &ri->r->nat;
    If I'm not mistaken, then we can break from the loop right here as soon as
    we find the pool.

> +			if (rdrpl == NULL &&
> +			    ri->r->rdr.addr.type != PF_ADDR_NONE)
> +				rdrpl = &ri->r->rdr;
    same here.    
> +			if (natpl != NULL && rdrpl != NULL)
> +				break;
    the condition and break above can be removed then.
> +		}
> +	}

The thing is that rdr-to rules always imply match on inbound packets,
and similarly nat-to rules always imply match on outbound packets.
So the natrule we are post-processing here will either have rdrpl
or natpl, but never both at the same time. So this the only change
I can suggest to improve in your diff.

thanks and
regards
sashan