Hello,

I've tested this patch. It looks that rules are loading correctly and without
warning or errors. When you think this patch may be merged with code base? Is
there a chance that this fix will be released as patch to the current version? (7.7)


----- Original Message -----
From: "Alexandr Nedvedicky" <sashan@fastmail.net>
To: "Rafał Ramocki" <rafal.ramocki@eo.pl>
Cc: "tech" <tech@openbsd.org>
Sent: Monday, May 12, 2025 10:26:10 AM
Subject: Re: Error when at startup more than 512 anchors are loaded from pf.conf

Hello,

On Wed, May 07, 2025 at 10:34:49AM +0200, Rafa?? Ramocki wrote:
> Hello,
> 
> I've tested a bit more. I have the problem but only if i load configuration second time.
> 
> (16:31:08):~
> # pfctl -f /etc/pf.conf                                                                                                                                                                                                                                                                 

    I think I've figured out what's going on there. Currently the pfctl(8)
    always initializes defaults by calling pfctl_init_options(). This is where
    we set anchor limit to 512 although the limit used by kernel might be different.
    I've just updated the diff to let  pfctl_init_options() use values we found
    when calling pfctl_read_limits() when we open /dev/pf. The pfctl_init_options()
    is being called from pfctl_rules() which is entered well after /dev/pf gets
    opened.

    the other tweak is to replace exit() at the end of main() by return (suggested
    by bluhm@)

    and there is one more tweak to what's been OKed by bluhm@: in earlier diff I've
    removed call to pfctl_load_options() in pfctl_rules(). Now I think this must stay,
    because we must make sure we call pfctl_load_options() at least one time when
    loading rules even if the pf.conf itself has no 'set limit ...'

    The diff below should be good enough for now. OK?

thanks and
regards
sashan

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 825bd4cf04c..04f2af43770 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -93,6 +93,8 @@ int         pfctl_show_states(int, const char *, int, long);
 int         pfctl_show_status(int, int);
 int         pfctl_show_timeouts(int, int);
 int         pfctl_show_limits(int, int);
+void         pfctl_read_limits(int);
+void         pfctl_restore_limits(void);
 void         pfctl_debug(int, u_int32_t, int);
 int         pfctl_show_anchors(int, int, char *);
 int         pfctl_ruleset_trans(struct pfctl *, char *, struct pf_anchor *);
@@ -163,6 +165,8 @@ static const struct {
         { NULL,                        0 }
 };
 
+static unsigned int        limit_curr[PF_LIMIT_MAX];
+
 struct pf_hint {
         const char        *name;
         int                timeout;
@@ -1162,6 +1166,37 @@ pfctl_show_limits(int dev, int opts)
         return (0);
 }
 
+void
+pfctl_read_limits(int dev)
+{
+        struct pfioc_limit pl;
+        int i;
+
+        for (i = 0; pf_limits[i].name; i++) {
+                pl.index = pf_limits[i].index;
+                if (ioctl(dev, DIOCGETLIMIT, &pl) == -1)
+                        err(1, "DIOCGETLIMIT");
+                limit_curr[i] = pl.limit;
+        }
+}
+
+void
+pfctl_restore_limits(void)
+{
+        struct pfioc_limit pl;
+        int i;
+
+        if (dev == -1)
+                return;
+
+        for (i = 0; pf_limits[i].name; i++) {
+                pl.index = pf_limits[i].index;
+                pl.limit = limit_curr[i];
+                if (ioctl(dev, DIOCSETLIMIT, &pl) == -1)
+                        warn("DIOCSETLIMIT (%s)", pf_limits[i].name);
+        }
+}
+
 /* callbacks for rule/nat/rdr/addr */
 void
 pfctl_add_rule(struct pfctl *pf, struct pf_rule *r)
@@ -1762,18 +1797,30 @@ pfctl_init_options(struct pfctl *pf)
         pf->syncookieswat[0] = PF_SYNCOOKIES_LOWATPCT;
         pf->syncookieswat[1] = PF_SYNCOOKIES_HIWATPCT;
 
+        /*
+         * limit_curr is populated by pfctl_read_limits() after main() opens
+         * /dev/pf.
+         */
         mib[0] = CTL_KERN;
         mib[1] = KERN_MAXCLUSTERS;
         size = sizeof(mcl);
         if (sysctl(mib, 2, &mcl, &size, NULL, 0) == -1)
                 err(1, "sysctl");
-        pf->limit[PF_LIMIT_FRAGS] = mcl / 4;
-
-        pf->limit[PF_LIMIT_SRC_NODES] = PFSNODE_HIWAT;
-        pf->limit[PF_LIMIT_TABLES] = PFR_KTABLE_HIWAT;
-        pf->limit[PF_LIMIT_TABLE_ENTRIES] = PFR_KENTRY_HIWAT;
-        pf->limit[PF_LIMIT_PKTDELAY_PKTS] = PF_PKTDELAY_MAXPKTS;
-        pf->limit[PF_LIMIT_ANCHORS] = PF_ANCHOR_HIWAT;
+        pf->limit[PF_LIMIT_FRAGS] = (limit_curr[PF_LIMIT_FRAGS] == 0) ?
+            mcl / 4 : limit_curr[PF_LIMIT_FRAGS];
+
+        pf->limit[PF_LIMIT_SRC_NODES] = (limit_curr[PF_LIMIT_SRC_NODES] == 0) ?
+            PFSNODE_HIWAT : limit_curr[PF_LIMIT_SRC_NODES];
+        pf->limit[PF_LIMIT_TABLES] = (limit_curr[PF_LIMIT_TABLES] == 0) ?
+            PFR_KTABLE_HIWAT : limit_curr[PF_LIMIT_TABLES];
+        pf->limit[PF_LIMIT_TABLE_ENTRIES] =
+            (limit_curr[PF_LIMIT_TABLE_ENTRIES] == 0) ?
+                PFR_KENTRY_HIWAT : limit_curr[PF_LIMIT_TABLE_ENTRIES];
+        pf->limit[PF_LIMIT_PKTDELAY_PKTS] =
+            (limit_curr[PF_LIMIT_PKTDELAY_PKTS] == 0) ?
+                PF_PKTDELAY_MAXPKTS : limit_curr[PF_LIMIT_PKTDELAY_PKTS];
+        pf->limit[PF_LIMIT_ANCHORS] = (limit_curr[PF_LIMIT_ANCHORS] == 0) ?
+            PF_ANCHOR_HIWAT : limit_curr[PF_LIMIT_ANCHORS];
 
         mib[0] = CTL_HW;
         mib[1] = HW_PHYSMEM64;
@@ -1878,6 +1925,9 @@ pfctl_set_limit(struct pfctl *pf, const char *opt, unsigned int limit)
         if (pf->opts & PF_OPT_VERBOSE)
                 printf("set limit %s %d\n", opt, limit);
 
+        if ((pf->opts & PF_OPT_NOACTION) == 0)
+                pfctl_load_options(pf);
+
         return (0);
 }
 
@@ -2730,10 +2780,13 @@ main(int argc, char *argv[])
                 dev = open(pf_device, mode);
                 if (dev == -1)
                         err(1, "%s", pf_device);
+                pfctl_read_limits(dev);
+                atexit(pfctl_restore_limits);
         } else {
                 dev = open(pf_device, O_RDONLY);
                 if (dev >= 0)
                         opts |= PF_OPT_DUMMYACTION;
+                pfctl_read_limits(dev);
                 /* turn off options */
                 opts &= ~ (PF_OPT_DISABLE | PF_OPT_ENABLE);
                 clearopt = showopt = debugopt = NULL;
@@ -2951,7 +3004,16 @@ main(int argc, char *argv[])
         if (lfile != NULL)
                 pfctl_state_load(dev, lfile);
 
-        exit(exit_val);
+        /*
+         * prevent pfctl_restore_limits() exit handler from restoring
+         * pf(4) options settings on successful exit.
+         */
+        if (exit_val == 0) {
+                close(dev);
+                dev = -1;
+        }
+
+        return exit_val;
 }
 #endif        /* REGRESS_NOMAIN */
 
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index 1e9a51e357b..6478aedc7b2 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -2140,8 +2140,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
                         goto fail;
                 }
 
-                pf_pool_limits[pl->index].limit_new = pl->limit;
-                pl->limit = pf_pool_limits[pl->index].limit;
+                error = pool_sethardlimit(pf_pool_limits[pl->index].pp,
+                    pl->limit, NULL, 0);
+                if (error == 0) {
+                        pf_pool_limits[pl->index].limit_new = pl->limit;
+                        pf_pool_limits[pl->index].limit = pl->limit;
+                }
                 PF_UNLOCK();
                 break;
         }
@@ -2702,21 +2706,6 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
                 NET_LOCK();
                 PF_LOCK();
 
-                /*
-                 * Checked already in DIOCSETLIMIT, but check again as the
-                 * situation might have changed.
-                 */
-                for (i = 0; i < PF_LIMIT_MAX; i++) {
-                        if (((struct pool *)pf_pool_limits[i].pp)->pr_nout >
-                            pf_pool_limits[i].limit_new) {
-                                PF_UNLOCK();
-                                NET_UNLOCK();
-                                free(table, M_PF, sizeof(*table));
-                                free(ioe, M_PF, sizeof(*ioe));
-                                error = EBUSY;
-                                goto fail;
-                        }
-                }
                 /* now do the commit - no errors should happen here */
                 for (i = 0; i < io->size; i++) {
                         PF_UNLOCK();
@@ -2769,20 +2758,6 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
                                 goto fail; /* really bad */
                         }
                 }
-                for (i = 0; i < PF_LIMIT_MAX; i++) {
-                        if (pf_pool_limits[i].limit_new !=
-                            pf_pool_limits[i].limit &&
-                            pool_sethardlimit(pf_pool_limits[i].pp,
-                            pf_pool_limits[i].limit_new, NULL, 0) != 0) {
-                                PF_UNLOCK();
-                                NET_UNLOCK();
-                                free(table, M_PF, sizeof(*table));
-                                free(ioe, M_PF, sizeof(*ioe));
-                                error = EBUSY;
-                                goto fail; /* really bad */
-                        }
-                        pf_pool_limits[i].limit = pf_pool_limits[i].limit_new;
-                }
                 for (i = 0; i < PFTM_MAX; i++) {
                         int old = pf_default_rule.timeout[i];