Just for the record, I'm running that pf_table patch for almost a month now without any negative impact on my load balancers.
pfsync/carp/relayd It also solved my problem with relayd. However I believe some care should also be taken on relayd part - do not check statistics on disabled redirects - make redirect respect disabled table I did posted some patches on tech@, don't know if they are ok but I do also run them on my load balancers. https://marc.info/?l=openbsd-tech&m=168859090917010&w=2 https://marc.info/?l=openbsd-tech&m=168899743827537&w=2 G On 01/08/2023 02:50, Alexandr Nedvedicky wrote: > Hello, > > the issue has been reported by Gianni Kapetanakis month ago [1]. It took > several emails to figure out relayd(8) exists after hosts got disabled > by 'relayctl host dis ...' > > The thing is that relayd(8) relies on pf(4) to create persistent > tables (PFR_TFLAG_PERSIST) as relayd requests that: > > 47 void > 48 init_tables(struct relayd *env) > 49 { > ... > 62 TAILQ_FOREACH(rdr, env->sc_rdrs, entry) { > 63 if (strlcpy(tables[i].pfrt_anchor, RELAYD_ANCHOR "/", > 64 sizeof(tables[i].pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE) > 65 goto toolong; > 66 if (strlcat(tables[i].pfrt_anchor, rdr->conf.name, > 67 sizeof(tables[i].pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE) > 68 goto toolong; > 69 if (strlcpy(tables[i].pfrt_name, rdr->conf.name, > 70 sizeof(tables[i].pfrt_name)) >= > 71 sizeof(tables[i].pfrt_name)) > 72 goto toolong; > 73 tables[i].pfrt_flags |= PFR_TFLAG_PERSIST; > 74 i++; > 75 } > > unfortunately it's not the case as further investigation revealed [2]. > > the issue can be easily reproduced by pfctl(8) which also creates > persistent tables on behalf of command line: > > pfctl -t foo -T add ... > > command above always asks pf(4) to create persistent table, however > pf(4) does not honor persistent flag when <foo> table exists already. > One can verify that using commands as follows: > > ## create 'referenced' table only (table exists but has no active flag) > # echo 'pass from in <foo> to any' |pfctl -f - > # pfctl -sT -vg > ----r-- foo > # create instance of table <foo> using command line: > # pfctl -t foo -T add 192.168.1.0/24 > 1/1 addresses added. > # pfctl -sT -vg > --a-r-- foo > ## create instance of table <bar>, note the table will get 'p' flag > # pfctl -t bar -T add 192.168.10.0/24 > 1 table created. > 1/1 addresses added. > # pfctl -sT -vg > -pa---- bar > --a-r-- foo > > one-liner change to sys/net/pf_table.c fixes that it also works for Gianni > Kapetanakis. I'm also adding tests to regress/sys/net/pf_table/Makefile > to cover it. > > On system which runs current the test fails with error as follows: > > pfctl -a regress/ttest -t instance -T add 192.168.1.0/24 > 1/1 addresses added. > pfctl -a regress/ttest -sT -vg | diff table-persist.out - > 1c1 > < -pa-r-- instance regress/ttest > --- > > --a-r-- instance regress/ttest > *** Error 1 in . (Makefile:96 'flags') > FAILED > > the failure is expected on system without patch. On system with > patch applied all tests do pass. > > OK to commit? > > thanks and > regards > sashan > > > [1] https://marc.info/?t=168811270400005&r=1&w=2 > > [2] https://marc.info/?l=openbsd-bugs&m=168868165801905&w=2 > > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sys/net/pf_table.c b/sys/net/pf_table.c > index 6f23a6f795d..c862c804f84 100644 > --- a/sys/net/pf_table.c > +++ b/sys/net/pf_table.c > @@ -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); > } > } > diff --git a/regress/sys/net/pf_table/Makefile > b/regress/sys/net/pf_table/Makefile > index a71f0190c73..8911e8a1d35 100644 > --- a/regress/sys/net/pf_table/Makefile > +++ b/regress/sys/net/pf_table/Makefile > @@ -1,15 +1,26 @@ > # $OpenBSD: Makefile,v 1.3 2017/07/07 23:15:27 bluhm Exp $ > > -REGRESS_TARGETS= hit miss cleanup > -CLEANFILES= stamp-* > +REGRESS_TARGETS= hit miss cleanup flags > +CLEANFILES= stamp-* \ > + pf-reftab.conf \ > + pf-instance.conf \ > + table-ref.conf \ > + table-pgone.out \ > + table-persist.out \ > + table-ref.out \ > + table-refgone.out > + > > 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 +38,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> > > >
