Table persistent flag (PFR_TFLAG_PERSIST) won't get set

by ioctl(2) operation if table exists already. The issue
has been noticed by Giannis Kapetanakis (_at_) edu.physics.uoc.gr.
Giannis noticed relayd(8) aborts unexpectedly when 'relay host'
gets disabled by 'relayctl host dis ...' command. To understand
what's going on we must look at the way how relayd(8) manages
its firewall configuration. If relay-host 'example' is enabled relayd(8)
inserts rule to anchor relayd/example. The rule looks somewhat
like this:
     pass in from ... to ... rdr-to <example>
When the rule gets inserted to pf(4) the table 'example' is
created with flags:
    lumpy# pfctl -a relayd/example -vg -sT
    ----r-- example relayd/example
r-flag indicates table is referred by rule only. In the next
step relayd(8) creates and populates table example. It asks
pf(4) to add a persistent flag (PFR_TFLAG_PERSIST), so table
survives flush operation of relayd/example ruleset on behalf
of 'relayctl host dis ...' command. relayd(8) exits via abort()
when table is gone with disable operation.

Giannis was patient enough so we could debug and fix issue.
The committed change has been tested by Giannis too.

OK kn@, bluhm@

diff --git a/regress/sys/net/pf_table/Makefile b/regress/sys/net/pf_table/Makefile
index a71f019..7b4de28 100644 (file)
--- a/regress/sys/net/pf_table/Makefile
+++ b/regress/sys/net/pf_table/Makefile
@@ -1,15 +1,27 @@
-#      $OpenBSD: Makefile,v 1.3 2017/07/07 23:15:27 bluhm Exp $
+#      $OpenBSD: Makefile,v 1.4 2023/08/10 16:44:04 sashan Exp $
+
+REGRESS_TARGETS=       hit miss cleanup flags
+CLEANFILES=            stamp-* \
+                       pf-instab.conf          \
+                       pf-instance.conf        \
+                       pf-reftab.conf          \
+                       table-ref.conf          \
+                       table-pgone.out         \
+                       table-persist.out       \
+                       table-ref.out           \
+                       table-refgone.out
 
-REGRESS_TARGETS=       hit miss cleanup
-CLEANFILES=            stamp-*
 
 stamp-setup:
+       ${SUDO} pfctl -a regress/ttest -Fa
        ${SUDO} pfctl -qt __regress_tbl -T add -f ${.CURDIR}/table.in
        date >$@
 
 cleanup:
        rm -f stamp-setup
        ${SUDO} pfctl -qt __regress_tbl -T kill
+       ${SUDO} pfctl -q -a regress/ttest -Fr
+       ${SUDO} pfctl -q -a regress/ttest -qt instance -T kill
 
 hit: stamp-setup
        for i in `cat ${.CURDIR}/table.hit`; do \
@@ -27,6 +39,77 @@ miss: stamp-setup
        done; \
        exit 0
 
-.PHONY: hit miss
+#
+# tables <instance> and <reference> are both referenced by rule only
+#
+pf-instab.conf:
+       @echo 'table <instance> { 192.168.1.0/24 }' > $@
+       @echo 'pass in from <instance> to <reference>' >> $@
+
+#
+# table <instance> is active and referred by rule, table <reference>
+# is referenced only.
+pf-reftab.conf:
+       @echo 'pass in from <instance> to <reference>' > $@
+
+#
+# check persistent flag (p) is gone from table <instance> after
+# we load pf-instab.conf. Deals with case when persistent table <instance>
+# exists before pf-instab.conf gets loaded.
+#
+table-pgone.out:
+       @echo '--a-r--  instance        regress/ttest' > $@
+       @echo '----r--  reference       regress/ttest' >> $@
+
+#
+# verify table <instance> got persistent flag after we
+# run 'pfctl -t instance -T add ...'
+#
+table-persist.out:
+       @echo '-pa-r--  instance        regress/ttest' > $@
+       @echo '----r--  reference       regress/ttest' >> $@
+
+#
+# verify tables <instance> and <reference> are created on behalf of
+# reference by rule after pf-reftab.conf got loaded.
+#
+table-ref.out:
+       @echo '----r--  instance        regress/ttest' > $@
+       @echo '----r--  reference       regress/ttest' >> $@
+
+#
+# verify reference to <instance> table (persistent) is gone
+# after rules got flushed
+#
+table-refgone.out:
+       @echo '-pa----  instance        regress/ttest' > $@
+
+flags: pf-instab.conf pf-reftab.conf table-pgone.out table-persist.out \
+    table-ref.out table-refgone.out
+       @echo 'loading pf-reftab,conf (tables referenced by rules only)'
+       @cat pf-reftab.conf
+       ${SUDO} pfctl -a regress/ttest -f pf-reftab.conf
+       @echo 'tables <reference> and <instance> should both have ----r--'
+       ${SUDO} pfctl -a regress/ttest -sT -vg | diff table-ref.out -
+       @echo 'creating <instance> table on command line, flags should be:'
+       @cat table-persist.out
+       ${SUDO} pfctl -a regress/ttest -t instance -T add 192.168.1.0/24
+       ${SUDO} pfctl -a regress/ttest -sT -vg | diff table-persist.out -
+       @echo 'flushing rules'
+       ${SUDO} pfctl -a regress/ttest -Fr
+       @echo 'table <reference> should be gone, table <instance> should stay'
+       ${SUDO} pfctl -a regress/ttest -sT -vg | diff table-refgone.out -
+       @echo 'loading pf-instab.conf'
+       @cat pf-instab.conf
+       ${SUDO} pfctl -a regress/ttest -f pf-instab.conf
+       @echo 'table <instance> loses -p- flag:'
+       @cat table-pgone.out
+       ${SUDO} pfctl -a regress/ttest -sT -vg | diff table-pgone.out -
+       @echo 'flusing rules, both tables should be gone'
+       ${SUDO} pfctl -a regress/ttest -Fr
+       @echo 'anchor regress/ttest must be gone'
+       ${SUDO} pfctl -a regress/ttest -sr 2>&1 | grep 'pfctl: Anchor does not exist'
+
+.PHONY: hit miss flags
 
 .include <bsd.regress.mk>

diff --git a/sys/net/pf_table.c b/sys/net/pf_table.c
index 6f23a6f..5e8fff8 100644 (file)
--- a/sys/net/pf_table.c
+++ b/sys/net/pf_table.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: pf_table.c,v 1.144 2023/01/05 10:06:58 sashan Exp $   */
+/*     $OpenBSD: pf_table.c,v 1.145 2023/08/10 16:44:04 sashan Exp $   */
 
 /*
  * Copyright (c) 2002 Cedric Berger
@@ -1565,8 +1565,10 @@ pfr_add_tables(struct pfr_table *tbl, int size, int *nadd, int flags)
                        xadd++;
                } else if (!(flags & PFR_FLAG_DUMMY) &&
                    !(p->pfrkt_flags & PFR_TFLAG_ACTIVE)) {
-                       p->pfrkt_nflags = (p->pfrkt_flags &
-                           ~PFR_TFLAG_USRMASK) | PFR_TFLAG_ACTIVE;
+                       p->pfrkt_nflags =
+                           (p->pfrkt_flags & ~PFR_TFLAG_USRMASK) |
+                           (n->pfrkt_flags & PFR_TFLAG_USRMASK) |
+                           PFR_TFLAG_ACTIVE;
                        SLIST_INSERT_HEAD(&changeq, p, pfrkt_workq);
                }
        }