Hello sasha-san,

On Fri, 15 Aug 2025 10:58:13 +0200
Alexandr Nedvedicky <sashan@fastmail.net> wrote:
> 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.

In my understanding, pf.conf(5) says "nat-to is usually applied
outbound" and "rdr-to is usually applied inbound", but actually pf
accepts "in nat-to" and "out rdr-to" and it works.

So I prefer refusing the config when being parsed or writing the
behavior on the man page clearer if we don't handle such the non usual
cases.

Aslo, if we can assume the state can have only rdrpl or natpl, we can
suppose it's natrule.ptr->nat or natrule.ptr->rdr then there is no
need to iterate the match_rule.

What do you think?