From: Rafał Ramocki Subject: Re: Error when at startup more than 512 anchors are loaded from pf.conf To: Alexandr Nedvedicky Cc: tech Date: Mon, 5 May 2025 15:28:39 +0200 Hello, In this order, with your diff it will get loaded. But if you will have: ----8<-------8<-------8<-------8<-----------8<---- set limit states 700000 set limit src-nodes 500000 set limit tables 2000 set limit anchors 2000 anchor test_0 anchor test_1 anchor test_2 ----8<-------8<-------8<-------8<-----------8<---- you will have three errors for three limits set before anchors. Regarding strange state when rule load is interrupted... I've do some tests to narrow this issue down. And I do not confirm your hypotesis. Error is triggered in "PF_TRANS_RULESET" branch in second "for" loop where "no errors should happen". I've figured out that error is returing function pf_commit_rules(). Then I've narrowed down it a bit further and found out that the error is proxied from function pf_commit_queues() that is called at the verry end of pf_commit_rules(). Then I've found out that error is actualy triggered from pf_create_queues(). And it looks like EIVAL comes from error = qif->pfqops->pfq_addqueue(qif->disc, q); It looks like it is verry low level error. I also found out that this strange state is triggered when rules are loaded but there were no commit triggered. I the configuration I have verry few queues. Only three actualy. queue dl_XXXXX_limit on $if_XXXXX_ipsec bandwidth "120Mb" queue XXXXX_limit parent dl_XXXXX_limit bandwidth "50Mb" queue dl_XXXXXX_limit_default parent dl_XXXXX_limit bandwidth "40Mb" default pass in quick on $if_XXXXX_ipsec proto tcp from { X.X.X.X Y.Y.Y.Y } to Z.Z.Z.Z port { AAAA } rdr-to X.X.X.X port AAAA queue XXXXX_limit ----- Original Message ----- From: "Alexandr Nedvedicky" To: "Rafał Ramocki" Cc: "tech" Sent: Tuesday, April 29, 2025 4:01:16 PM Subject: Re: Error when at startup more than 512 anchors are loaded from pf.conf Hello, I did have some more time to re-test the diff. this is my testing rule-set I'm using: ----8<-------8<-------8<-------8<-----------8<---- set limit anchors 2048 set limit states 60000 anchor test_0 anchor test_1 anchor test_2 .... anchor test_1023 ----8<-------8<-------8<-------8<-----------8<---- I did verify it gets loaded. src# ./pfctl -sr |wc -l 1024 Also using pfctl -sm confirms the limit for anchors got changed: src# ./pfctl -sm states hard limit 60000 src-nodes hard limit 10000 frags hard limit 65536 tables hard limit 1000 table-entries hard limit 200000 pktdelay-pkts hard limit 10000 anchors hard limit 2048 So the fix seems to work when running without '-n'. I agree diff does not work when '-n' is used, more work is needed. I forgot to test my change with -n option. I will try to get to it over the weekend. > I think I've also hit another error. After execution this way: > "pfctl -vv -f /etc/pf.conf | less" and quitting before end of the > output I've got DIOCXCOMMIT error for any further loads: > > # pfctl -f /etc/pf.conf > pfctl: Current pool size exceeds requested anchors limit 512 > pfctl: Current pool size exceeds requested anchors limit 512 > pfctl: Current pool size exceeds requested anchors limit 512 > pfctl: DIOCXCOMMIT: Invalid argument I could not reproduce it yet. If I understand code right, then this error must coming from default branch where in switch: switch (ioe->type) { case PF_TRANS_TABLE: memset(table, 0, sizeof(*table)); strlcpy(table->pfrt_anchor, ioe->anchor, sizeof(table->pfrt_anchor)); if ((error = pfr_ina_commit(table, ioe->ticket, NULL, NULL, 0))) { PF_UNLOCK(); NET_UNLOCK(); free(table, M_PF, sizeof(*table)); free(ioe, M_PF, sizeof(*ioe)); goto fail; /* really bad */ } break; case PF_TRANS_RULESET: if ((error = pf_commit_rules(ioe->ticket, ioe->anchor))) { PF_UNLOCK(); NET_UNLOCK(); free(table, M_PF, sizeof(*table)); free(ioe, M_PF, sizeof(*ioe)); goto fail; /* really bad */ } break; default: PF_UNLOCK(); NET_UNLOCK(); free(table, M_PF, sizeof(*table)); free(ioe, M_PF, sizeof(*ioe)); error = EINVAL; goto fail; /* really bad */ The EINVAL here indicates there is some binary incompatibility between pfctl and pf. And you should be hitting it always regardless hitting CTRL+C when loading the rules or not. I would expect to see EBUSY error which is eventually returned before we arrive to the switch statement shown above. The EBUSY indicates there is mismatch between transaction id. This may happen when two processes attempt to change the same ruleset and it should be recoverable (temporal). thanks and regards sashan On Mon, Apr 28, 2025 at 07:13:59PM +0200, Rafa?? Ramocki wrote: > Hello, > > I've rebuilded kernel and pfctl with that change have proposed. > It when rules were loaded it popped up three warnings as bellow: > > # pfctl -f /etc/pf.conf > pfctl: Current pool size exceeds requested anchors limit 512 > pfctl: Current pool size exceeds requested anchors limit 512 > pfctl: Current pool size exceeds requested anchors limit 512 > > I've gathered some more information and it looks like pfctl_set_limit > is actually caled four times in my case. One time for every > "set limit" in pfctl.conf. And since set limit anchors was the forth limit > only forth execution passed without this error. When I rearanged > order of limits placing limit of anchors before all other limits loading > rules was possible without this message of exceeded anchors. > > I think I've hit another problem with that change too. > Adding "-n" to pfctl gives error messages too: > > # pfctl -n -f /etc/pf.conf > pfctl: Cannot set states limit to 700000 > pfctl: Cannot set src-nodes limit to 10000 > pfctl: Cannot set frags limit to 65536 > pfctl: Cannot set tables limit to 1000 > pfctl: Cannot set table-entries limit to 200000 > pfctl: Cannot set pktdelay-pkts limit to 10000 > pfctl: Cannot set anchors limit to 512 > pfctl: DIOCSETTIMEOUT > (...cut...) > pfctl: DIOCSETTIMEOUT > pfctl: DIOCSETDEBUG > pfctl: DIOCSETSTATUSIF > > (and this is was displayed three times). Following version is preventing > this: > > > diff -u -p -r1.390 pfctl.c > --- pfctl.c 6 Jan 2023 17:44:33 -0000 1.390 > +++ pfctl.c 28 Apr 2025 17:11:57 -0000 > @@ -1843,6 +1843,9 @@ pfctl_set_limit(struct pfctl *pf, const > 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); > } > > > I think I've also hit another error. After execution this way: > "pfctl -vv -f /etc/pf.conf | less" and quitting before end of the > output I've got DIOCXCOMMIT error for any further loads: > > # pfctl -f /etc/pf.conf > pfctl: Current pool size exceeds requested anchors limit 512 > pfctl: Current pool size exceeds requested anchors limit 512 > pfctl: Current pool size exceeds requested anchors limit 512 > pfctl: DIOCXCOMMIT: Invalid argument > > After no changes in pf.conf and reboot there is no DIOCXCOMMIT error > any more. It looks like interruption of transaction prevents for any > future transactions. > > > > ----- Original Message ----- > From: "Alexandr Nedvedicky" > To: "Rafa?? Ramocki" > Cc: "tech" > Sent: Saturday, April 26, 2025 8:28:10 PM > Subject: Re: Error when at startup more than 512 anchors are loaded from pf.conf > > Hello, > > I'm sorry I've missed your first email on Apr 17th. > > Below is a diff which should solve the issue for you. It also > illustrates the problem which you already understand very well. > > Indeed the new limit value is set after all rules are loaded > to transaction and pf(4) is doing a commit. So it is futile > to attempt increase the limit pf.conf(5) which attempts to > load the rules/anchors where achnor limit is being exceeded. > > to workaround it you need to do it in two steps now: > use file (pf-limits.conf) and load it first (pfctl -f...) > and then load the file rules (pf-rules.conf) > > diff below changes the DIOCSETLIMIT ioctl to set the limit > immediately when ioctl is being handled. We still need to > enable pfctl(8) parser to issue the ioctl as soon as rule > is accepted. The current one-liner tweak is a small-hack > to verify the idea. > > The not so intended effect of this change is that if > pfctl(8) parser fails to load/accept all rules then > the limits changed by command are left behind. > > The change which will also revert the limit settings > if something will go wrong with transaction will require > some more thought. > > I think it's worth to consider the proposed change > which leaves changed limits behind if error happens. > I still need to make pfctl side more tidy if there will > be a general agreement on this. > > To be honest I can not think of any scenario where changing > the limits like in presented diff can be problematic. > > thanks and > regards > sashan > > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c > index 825bd4cf04c..816d7644b97 100644 > --- a/sbin/pfctl/pfctl.c > +++ b/sbin/pfctl/pfctl.c > @@ -1878,6 +1878,8 @@ 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); > > + pfctl_load_options(pf); > + > return (0); > } > > 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];