From: Alexander Bluhm Subject: Re: Error when at startup more than 512 anchors are loaded from pf.conf To: Alexandr Nedvedicky Cc: Rafa?? Ramocki , tech Date: Thu, 22 May 2025 12:56:04 +0900 On Mon, May 12, 2025 at 10:26:10AM +0200, Alexandr Nedvedicky wrote: > 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? Note that pool_sethardlimit() has lost two of its parameters recently. You have to merge your diff. OK bluhm@ > --------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]; >