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, 8 May 2025 19:45:01 +0200 On Sat, May 03, 2025 at 10:17:34PM +0200, Alexandr Nedvedicky wrote: > Hello, > > below is a diff I'd like to commit to address this issue. > currently we update the limit when committing transaction > which does not work when we need to increase the limit > in order to load the rules. > > I'd like to set the limit immediately as soon as particular > 'set limit ...' statement gets accepted by parser. > > The proposed change also does best effort to reset the limits > back to their original values when the configuration file > can not be processed successfully. The idea here is to read > current limits from pf(4) driver and arm atexit(3) handler which > eventually reverts the changes if anything goes wrong. On the > other hand if configuration gets processed successfully the main() > functions neuters the atexit(3) handler by closing /dev/pf to > keep any limit changes in. > > OK? passes regress OK bluhm@ one remark: At the end of main we prefer return(exit_val) over exit(exit_val). Then stack protector runs again and has a chance to find bugs. Just seen in the context, not part of your diff. > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c > index 825bd4cf04c..0fdb5269fd1 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) > @@ -1685,8 +1720,6 @@ pfctl_rules(int dev, char *filename, int opts, int optimize, > pfctl_clear_queues(&rootqs); > > if ((opts & PF_OPT_NOACTION) == 0) { > - if (!anchorname[0] && pfctl_load_options(&pf)) > - goto _error; > if (pfctl_trans(dev, t, DIOCXCOMMIT, osize)) > ERR("DIOCXCOMMIT"); > } > @@ -1878,6 +1911,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,6 +2766,8 @@ 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) > @@ -2951,6 +2989,15 @@ main(int argc, char *argv[]) > if (lfile != NULL) > pfctl_state_load(dev, lfile); > > + /* > + * prevent pfctl_restore_limits() exit handler from restoring > + * pf(4) options settings on successful exit. > + */ > + if (exit_val == 0) { > + close(dev); > + dev = -1; > + } > + > exit(exit_val); > } > #endif /* REGRESS_NOMAIN */ > diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c > index e7591ab4d40..16f8719386b 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];