Index | Thread | Search

From:
Alexandr Nedvedicky <sashan@fastmail.net>
Subject:
Re: Error when at startup more than 512 anchors are loaded from pf.conf
To:
Rafa?? Ramocki <rafal.ramocki@eo.pl>
Cc:
tech <tech@openbsd.org>
Date:
Mon, 12 May 2025 10:26:10 +0200

Download raw body.

Thread
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];