From d40c63659aefc32359b6396f35c7bc9a9ae8f265 Mon Sep 17 00:00:00 2001 From: sashan Date: Thu, 10 Aug 2023 16:44:04 +0000 Subject: [PATCH] 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 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@ --- regress/sys/net/pf_table/Makefile | 91 +++++++++++++++++++++++++++++-- sys/net/pf_table.c | 8 ++- 2 files changed, 92 insertions(+), 7 deletions(-) diff --git a/regress/sys/net/pf_table/Makefile b/regress/sys/net/pf_table/Makefile index a71f0190c73..7b4de28624d 100644 --- 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 and are both referenced by rule only +# +pf-instab.conf: + @echo 'table { 192.168.1.0/24 }' > $@ + @echo 'pass in from to ' >> $@ + +# +# table is active and referred by rule, table +# is referenced only. +pf-reftab.conf: + @echo 'pass in from to ' > $@ + +# +# check persistent flag (p) is gone from table after +# we load pf-instab.conf. Deals with case when persistent table +# exists before pf-instab.conf gets loaded. +# +table-pgone.out: + @echo '--a-r-- instance regress/ttest' > $@ + @echo '----r-- reference regress/ttest' >> $@ + +# +# verify table 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 and 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 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 and should both have ----r--' + ${SUDO} pfctl -a regress/ttest -sT -vg | diff table-ref.out - + @echo 'creating 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 should be gone, table 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 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 diff --git a/sys/net/pf_table.c b/sys/net/pf_table.c index 6f23a6f795d..5e8fff8c544 100644 --- 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); } } -- 2.20.1