From: Alexandr Nedvedicky Subject: Re: Error when at startup more than 512 anchors are loaded from pf.conf To: Rafa?? Ramocki Cc: tech Date: Mon, 12 May 2025 10:26:10 +0200 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];