Hi Sasha,
Thanks for looking into the report,
On Fri, Sep 12, 2025 at 09:12:11PM +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> > * Is the issue reproducible on OpenBSD?
>
> yes, OpenBSD behavior is same
>
> > * Is there any reason why pfctl calls err() function
> > after ioctl(DIOCADDRULE) failure ?
> >
>
> the small change you are proposing
> > [pfctl.c]
> > - if (ioctl(pf->dev, DIOCADDRULE, &pr) == -1)
> > - err(1, "DIOCADDRULE");
> > + if (ioctl(pf->dev, DIOCADDRULE, &pr)) {
> > + warnx("DIOCADDRULE");
> > + return (1);
> > + }
>
> does not help much. the outcome is the memory held by
> transaction in kernel is released earlier.
Yes, it doesn't save much, but let me add few more details...
> in current OpenBSD the memory is released with next call DIOXBEGIN on
> existing ruleset.
True. Until the next DIOXBEGIN, the memory pool is exhausted and any
allocation attempt fails. If PF wants to allocate a new table for some
reason, the allocation request fails.
> I think the ROLLBACK of transaction is pointless here.
> hover you tired to load the second ruleset directly?
It's pointless for the testcase and its step 3. But in general I think it's
good to let the process exit gracefully and let it to either commit or
rollback the transaction it created.
The difference can be seen on Solaris, I wonder if the same difference can
be seen on OpenBSD. Please try to change the test case, add step 4:
# pfctl -t T1 -T add 1.2.3.4
Step 4 is executed after DIOCADDRULE failure and the difference
that I see is:
* with the patch:
# pfctl -t T1 -T add 1.2.3.4
1/1 addresses added.
* without the patch:
# pfctl -t T1 -T add 1.2.3.4
pfctl: Cannot allocate memory.
> this is what happens on my OpenBSD test system.
> I'm using pfctl with your suggested change:
>
> puffy# cat pf-2.conf
> set limit tables 3
> anchor A1 in inet all {
> pass in quick from <T1>
> }
> anchor A2 in inet all {
> pass in quick from <T1>
> }
> table <T1> persist { 1.2.3.4/24 }
>
>
> puffy# ./pfctl -f pf-2.conf
> pfctl: DIOCADDRULE: Invalid argument
> pfctl: Unable to load rules into kernel
Same here, the pfctl prints it's unable to load rules.
> and this is what happens with pfctl I have installed on my system
>
> puffy# pfctl -f pf-2.conf
> pfctl: DIOCADDRULE: Invalid argument
Yes, pfctl terminates abruptly.
> what is worth to note is the pfctl needs to create 4 tables
> when loads pf-2.conf ruleset. The tables are resolved with
> DIOCXCOMMIT operation when the tables which are not needed
> are released.
Right, I instrumented PF to debug the table creation and destroy, etc.
added SDT probes and I also monitor memory pools using the kernel debugger
on Solaris with the d-cmd:
# mdb -k <<< "pf_nin_table::print rbh_root | ::print
pfnin_pool_u.u_pool_s.s_pfr_ktable_pl[0].pmp_kmem_cache | ::walk kmem | ::print
-ta pfr_ktable ! egrep name\|anchor\|flags\|shadow\|root"
ffffa100aad445e8 char [1024] pfrt_anchor = [ "A1" ]
ffffa100aad449e8 char [32] pfrt_name = [ "T1" ]
ffffa100aad44a08 uint32_t pfrt_flags = 0x10
ffffa100aad44af0 struct pfr_ktable *pfrkt_shadow = 0
ffffa100aad44af8 struct pfr_ktable *pfrkt_root = 0xffffa1000c208b10
ffffa100aad44b10 int pfrkt_nflags = 0
ffffa1000c208b10 char [1024] pfrt_anchor = [ "" ]
ffffa1000c208f10 char [32] pfrt_name = [ "T1" ]
ffffa1000c208f30 uint32_t pfrt_flags = 0x25
ffffa1000c209018 struct pfr_ktable *pfrkt_shadow = 0
ffffa1000c209020 struct pfr_ktable *pfrkt_root = 0
ffffa1000c209038 int pfrkt_nflags = 0t37 (0x25)
Plus I can monitor the table activity via DTrace and SDT probes.
> the icotl loads two rulesets. each rule refers to table T1.
> however until commit happens the pf does not know both
> rules refer to the same table hence it keeps two table
> instances. One would think two tables should fit under
> the limit (which is set to 3). However there is a one more
> detail: each table found in anchor allocates ->prfkt_root table.
> so this is why pf-2.conf ruleset is doomed.
Agreed, there's no way to get pf-2.conf working with current
implementation of tables.
> and also the change
> you propose is futile.
The patch isn't going to fix pf-2.conf, but it can help to keep the memory pool
in "clean" state, so that the pool can serve allocation requests even
after pfctl failure.
But I don't push the patch!.... I agree it doesn't help much.
> I'm afraid implementation of more sensible behavior requires
> a lot more work. What I'd like eventually do is to
> make table a member of ruleset. pf in kernel keeps rulesets
> and tables in their respective look up tables (rb-trees),
> while pf.conf syntax (and pfctl option -a) kind of suggests
> the table is part of anchor.
Yes, that's for sure - I think there is definitely some room for an
improvement, e.g.:
PF documents the table limit that user can set on tables, but it doesn't
document tables that it creates automatically, shadow tables, root tables,
etc. So the limit defined by "set limit tables" and number of tables the
user is allowed to define in pf.conf may not match.
Another interesting observation on Solaris is that:
Adding tables via ioctl occurs immediately as the file is being parsed.
The user may want to raise the table limit if too many tables are defined
in pf.conf. However, raising the limit via "set limit tables" is effective
after the transaction is committed. Tables are added via ioctl before the
transaction is committed and if the original limit was too small, then
addition fails.... That means a single pf.conf file cannot define many
tables and raise the limit altogether. But I'm unsure if this is specific
to Solaris or not... I don't know how to get around this, the only way I
to define many tables for me is to have two config files, raise the limit
with first config file, then define the tables in another pf.conf.
> hope my explanation is reasonably clear.
Yes and thanks for quick response!
Vita
> good luck
>
> thanks and
> regards
> sashan
