From: Alexandr Nedvedicky <sashan@fastmail.net>
Subject: Re: pf: refcount struct pf_rule lifetime
To: Warnier Pierre <pierre@warnier.net>
Cc: tech@openbsd.org
Date: Wed, 15 Apr 2026 09:55:23 +0200

Hello,

On Tue, Apr 14, 2026 at 07:18:08PM +0200, Warnier Pierre wrote:
> 
> You're right that the diff on its own brings no current benefit.
> It's pure scaffolding; no caller takes an extra reference yet, so
> every refcnt_rele() sees count 1 and the existing pool_put() runs
> unchanged. I should have said this more plainly in the cover.
> 
> The follow-up I had in mind is refcnt_take(&r->ref) at rule
> selection in pf_test_rule() and refcnt_rele() on the pf_test()
> exit path, so a packet thread holds its rule reference explicitly
> across evaluation. The reason I didn't think anchor refcount alone
> was sufficient is pf_rm_rule(): it TAILQ_REMOVEs individual rules
> from a ruleset and pool_puts them, while the anchor stays alive.
> Anchor refcount protects the ruleset as a container, but not an
> individual rule pointer a thread has already picked up from it.
> 
> With the current NET_LOCK() + PF_LOCK() regime this is moot. Both
> pf_test_rule() and the pf_rm_rule() callers run serialised, so
> there is no race today. The rule-level refcount only becomes
> load-bearing once the rule-tree walk moves off PF_LOCK() exclusive
> (SMR or shared mode), at which point anchor-level alone doesn't
> close the gap between "rule removed from TAILQ" and "last packet
> thread using it drops its reference".
> 
> Is that the direction you want? If you'd rather unlock the read
> side via SMR on the rule tree ??? with pf_rm_rule() deferring
> pool_put() to a grace period instead of per-rule refcounting ??? I
> am happy to park this diff and start there instead. I don't want
> to execute the comment at the end of pf_test() in a shape that's
> no longer the one you have in mind.
> 

    it all makes sense however the pf(4) is not there yet. PF_LOCK()
    is not going away anytime soon. The lock protects those tables
    packet may modify on its journey through pf_test():

	- IP address tables (see overload action in pf.conf(5))

	- source IP address tables (see max-src-nddes, max-src-states,
	    source limiter in pf.conf(5))

	- and state table, however this can be in theory moved out
	  from PF_LOCK() scope, because it has its own RW-lock.

    The good time to land your diff will be once there will be a plan
    how to split the PF_LOCK() to 'smaller locks'. And here IMO are
    the complications which come from pf_ioctl().

If you are really interested to dive into this I can provide more
details. Just let me know.

thanks and
regards
sashan