From: Warnier Pierre <pierre@warnier.net>
Subject: pf: refcount struct pf_rule lifetime
To: tech@openbsd.org
Date: Tue, 14 Apr 2026 18:18:28 +0200

pf_test() ends with this comment:

        /*
         * At the moment, we rely on NET_LOCK() to prevent removal of items
         * we've collected above ('r', 'anchor' and 'ruleset').  They'll have
         * to be refcounted when NET_LOCK() is gone.
         */

struct pf_anchor already has that lifetime refcount (struct refcnt ref
in pfvar.h, initialised in pf_create_anchor(), wrapped by
pf_anchor_take()/pf_anchor_rele() in pf_ruleset.c).  struct pf_ruleset
is embedded in pf_anchor via anchor->ruleset, and pf_main_ruleset is
a macro for pf_main_anchor.ruleset, so ruleset lifetime tracks the
anchor for free.  struct pf_rule has no refcount.

Diff below adds one to pf_rule, matching the anchor pattern:
refcnt_init() at the two pool_get(&pf_rule_pl, ...) sites in
pfioctl(), refcnt_rele() gating the three pool_put(&pf_rule_pl, ...)
sites in pf_rule_free(), pf_rm_rule() and the DIOCCHANGERULE error
path.  No caller takes an extra reference yet; every refcnt_rele()
sees count 1, returns non-zero, and pool_put() runs exactly as
before.  The intent is to get the field and the release gate in
place so a later diff can refcnt_take() around rule selection in
pf_test() without having to touch the lifetime paths again.

sizeof(struct pf_rule) changes, which cascades through pfioc_rule to
DIOC{ADD,GET,CHANGE}RULE; pfctl has to be rebuilt in lockstep.  No
new include: pfvar.h already pulls in <sys/refcnt.h>.

Tested on amd64 GENERIC.MP against a recent snapshot:

  - builds clean (-Werror, clang)
  - regress/sys/net/pf_trans passes (run-regress-dev-limit with 2048
    procs and 1023 /dev/pf fds each, run-regress-iocmd-limit with
    DIOCGETRULES and DIOCXEND)
  - 500 iterations of `pfctl -f /etc/pf.conf` under concurrent TCP
    traffic: no errors, no warnings, pfrule pool stats balanced
    (3518 cumulative requests, 8 in-use at end matching the loaded
    ruleset)
  - no panics, no splassert, no witness complaints, no leaks

ok?


Index: sys/net/pfvar.h
===================================================================
--- sys/net/pfvar.h
+++ sys/net/pfvar.h
@@ -614,6 +614,7 @@
        }                       divert;

        time_t                   exptime;
+       struct refcnt            ref;           /* [a] rule lifetime */
 };

 /* rule flags */
Index: sys/net/pf_ioctl.c
===================================================================
--- sys/net/pf_ioctl.c
+++ sys/net/pf_ioctl.c
@@ -355,6 +355,8 @@
        pfi_kif_free(rule->nat.kif);
        pfi_kif_free(rule->route.kif);

+       if (refcnt_rele(&rule->ref) == 0)
+               return;
        pool_put(&pf_rule_pl, rule);
 }

@@ -408,6 +410,8 @@
        pfi_kif_unref(rule->nat.kif, PFI_KIF_REF_RULE);
        pfi_kif_unref(rule->route.kif, PFI_KIF_REF_RULE);
        pf_remove_anchor(rule);
+       if (refcnt_rele(&rule->ref) == 0)
+               return;
        pool_put(&pf_rule_pl, rule);
 }

@@ -2327,6 +2331,7 @@
                        error = ENOMEM;
                        goto fail;
                }
+               refcnt_init(&rule->ref);

                if ((error = pf_rule_copyin(&pr->rule, rule))) {
                        pf_rule_free(rule);
@@ -2578,10 +2583,12 @@
                                error = ENOMEM;
                                goto fail;
                        }
+                       refcnt_init(&newrule->ref);

                        if (pcr->rule.return_icmp >> 8 > ICMP_MAXTYPE) {
                                error = EINVAL;
-                               pool_put(&pf_rule_pl, newrule);
+                               if (refcnt_rele(&newrule->ref) != 0)
+                                       pool_put(&pf_rule_pl, newrule);
                                goto fail;
                        }
                        error = pf_rule_copyin(&pcr->rule, newrule);