i upgraded one of the work firewalls to -current and added the diff below in, and got what looks like a different panic:
ddb{6}> tr db_enter() at db_enter+0x5 panic(ffffffff81e2cc31) at panic+0xbf __assert(ffffffff81eae23b,ffffffff81ee4549,797,ffffffff81e7fd91) at __assert+0x25 pfsync_insert_state(fffffd816375b720) at pfsync_insert_state+0xec pf_state_insert(ffff8000001f0000,ffff800024cd9cc0,ffff800024cd9cb8,fffffd816375b720) at pf_state_insert+0x2df pf_test_rule(ffff800024cd9e50,ffff800024cd9e38,ffff800024cd9e30,ffff800024cd9e40,ffff800024cd9e28,ffff800024cd9e4e,1) at pf_test_rule+0xec4 pf_test(2,3,ffff8000016ab800,ffff800024cd9fe8) at pf_test+0x1126 ip_output(fffffd8052dae400,0,ffff800024cda0b0,1,0,0,ffff800001640801) at ip_out put+0x72a ip_forward(fffffd8052dae400,ffff800001640800,fffffd81840e2ad0,0) at ip_forward+0x286 ip_input_if(ffff800024cda2e0,ffff800024cda2dc,4,0,ffff800001640800) at ip_input_if+0x347 ipv4_input(ffff800001640800,fffffd8052dae400) at ipv4_input+0x37 ether_input(ffff800001640800,fffffd8052dae400) at ether_input+0x394 carp_input(ffff8000016a2000,fffffd8052dae400,5e000156) at carp_input+0x186 ether_input(ffff8000016a2000,fffffd8052dae400) at ether_input+0x1c5 vlan_input(ffff80000161a000,fffffd8052dae400,ffff800024cda4fc) at vlan_input+0x22d ether_input(ffff80000161a000,fffffd8052dae400) at ether_input+0x83 if_input_process(ffff80000019a048,ffff800024cda588) at if_input_process+0x4a ifiq_process(ffff8000001f0800) at ifiq_process+0x8e taskq_thread(ffff80000002c080) at taskq_thread+0xfa end trace frame: 0x0, count: -19 ddb{6}> sh panic *cpu6: kernel diagnostic assertion "st->sync_state == PFSYNC_S_NONE" failed: file "/usr/src/sys/net/if_pfsync.c", line 1943 i'll try and have a look at it. i am probably most responsible for the code :( On Wed, Jun 08, 2022 at 12:42:33AM +0200, Alexandr Nedvedicky wrote: > Hello Hrvoje, > > </snip> > > > > Hi, > > > > while booting with this diff I've got this log: > > > > starting early daemons: syslogd pflogd ntpdwitness: lock_object > > uninitialized: 0xfffffd8785c81a > > 90 > > Starting stack trace... > > witness_checkorder(fffffd8785c81a90,9,0) at witness_checkorder+0xad > > mtx_enter(fffffd8785c81a80) at mtx_enter+0x34 > > pf_remove_state(fffffd8785c81988) at pf_remove_state+0x1da > > pfsync_in_del_c(fffffd80028977b0,c,2,2) at pfsync_in_del_c+0x9f > > pfsync_input(ffff800020b056e8,ffff800020b056f4,f0,2) at pfsync_input+0x33c > > ip_deliver(ffff800020b056e8,ffff800020b056f4,f0,2) at ip_deliver+0x103 > > ip_local(ffff800020b056e8,ffff800020b056f4,fe0000007fff0220,0) at > > ip_local+0x1b7 > > ipintr() at ipintr+0x5f > > if_netisr(0) at if_netisr+0xca > > taskq_thread(ffff800000036000) at taskq_thread+0x11a > > thanks for quick test with pfsync. it has turned out I've forgot to > initialize > a pf_state::mtx in pfsync_state_import() function. > > below is updated diff, which should fix a stack trace reported by witness. > > thanks and > regards > sashan > > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c > index c78ca62766e..b039a50a7bb 100644 > --- a/sys/net/if_pfsync.c > +++ b/sys/net/if_pfsync.c > @@ -157,16 +157,16 @@ struct { > }; > > struct pfsync_q { > - void (*write)(struct pf_state *, void *); > + int (*write)(struct pf_state *, void *); > size_t len; > u_int8_t action; > }; > > /* we have one of these for every PFSYNC_S_ */ > -void pfsync_out_state(struct pf_state *, void *); > -void pfsync_out_iack(struct pf_state *, void *); > -void pfsync_out_upd_c(struct pf_state *, void *); > -void pfsync_out_del(struct pf_state *, void *); > +int pfsync_out_state(struct pf_state *, void *); > +int pfsync_out_iack(struct pf_state *, void *); > +int pfsync_out_upd_c(struct pf_state *, void *); > +int pfsync_out_del(struct pf_state *, void *); > > struct pfsync_q pfsync_qs[] = { > { pfsync_out_iack, sizeof(struct pfsync_ins_ack), PFSYNC_ACT_INS_ACK }, > @@ -516,10 +516,10 @@ pfsync_alloc_scrub_memory(struct pfsync_state_peer *s, > return (0); > } > > -void > +int > pfsync_state_export(struct pfsync_state *sp, struct pf_state *st) > { > - pf_state_export(sp, st); > + return (pf_state_export(sp, st)); > } > > int > @@ -680,6 +680,7 @@ pfsync_state_import(struct pfsync_state *sp, int flags) > st->sync_state = PFSYNC_S_NONE; > > refcnt_init(&st->refcnt); > + mtx_init(&st->mtx, IPL_NET); > > /* XXX when we have anchors, use STATE_INC_COUNTERS */ > r->states_cur++; > @@ -1529,24 +1530,25 @@ pfsyncioctl(struct ifnet *ifp, u_long cmd, caddr_t > data) > return (0); > } > > -void > +int > pfsync_out_state(struct pf_state *st, void *buf) > { > struct pfsync_state *sp = buf; > > - pfsync_state_export(sp, st); > + return (pfsync_state_export(sp, st)); > } > > -void > +int > pfsync_out_iack(struct pf_state *st, void *buf) > { > struct pfsync_ins_ack *iack = buf; > > iack->id = st->id; > iack->creatorid = st->creatorid; > + return (0); > } > > -void > +int > pfsync_out_upd_c(struct pf_state *st, void *buf) > { > struct pfsync_upd_c *up = buf; > @@ -1557,9 +1559,10 @@ pfsync_out_upd_c(struct pf_state *st, void *buf) > pf_state_peer_hton(&st->dst, &up->dst); > up->creatorid = st->creatorid; > up->timeout = st->timeout; > + return (0); > } > > -void > +int > pfsync_out_del(struct pf_state *st, void *buf) > { > struct pfsync_del_c *dp = buf; > @@ -1568,6 +1571,7 @@ pfsync_out_del(struct pf_state *st, void *buf) > dp->creatorid = st->creatorid; > > SET(st->state_flags, PFSTATE_NOSYNC); > + return (0); > } > > void > @@ -1881,8 +1885,8 @@ pfsync_sendout(void) > KASSERT(st->snapped == 1); > st->sync_state = PFSYNC_S_NONE; > st->snapped = 0; > - pfsync_qs[q].write(st, m->m_data + offset); > - offset += pfsync_qs[q].len; > + if (pfsync_qs[q].write(st, m->m_data + offset) == 0) > + offset += pfsync_qs[q].len; > > pf_state_unref(st); > count++; > diff --git a/sys/net/if_pfsync.h b/sys/net/if_pfsync.h > index bee6c77f228..4440a561854 100644 > --- a/sys/net/if_pfsync.h > +++ b/sys/net/if_pfsync.h > @@ -326,7 +326,7 @@ int pfsync_sysctl(int *, u_int, > void *, size_t *, > #define PFSYNC_SI_CKSUM 0x02 > #define PFSYNC_SI_ACK 0x04 > int pfsync_state_import(struct pfsync_state *, int); > -void pfsync_state_export(struct pfsync_state *, > +int pfsync_state_export(struct pfsync_state *, > struct pf_state *); > > void pfsync_insert_state(struct pf_state *); > diff --git a/sys/net/pf.c b/sys/net/pf.c > index 93fe5702625..c76f4c71263 100644 > --- a/sys/net/pf.c > +++ b/sys/net/pf.c > @@ -185,7 +185,8 @@ int pf_translate_icmp_af(struct > pf_pdesc*, int, void *); > void pf_send_icmp(struct mbuf *, u_int8_t, u_int8_t, int, > sa_family_t, struct pf_rule *, u_int); > void pf_detach_state(struct pf_state *); > -void pf_state_key_detach(struct pf_state *, int); > +void pf_state_key_detach(struct pf_state *, > + struct pf_state_key *); > u_int32_t pf_tcp_iss(struct pf_pdesc *); > void pf_rule_to_actions(struct pf_rule *, > struct pf_rule_actions *); > @@ -260,6 +261,9 @@ void > pf_state_key_unlink_inpcb(struct pf_state_key *); > void pf_inpcb_unlink_state_key(struct inpcb *); > void pf_pktenqueue_delayed(void *); > int32_t pf_state_expires(const struct pf_state *, > uint8_t); > +void pf_state_keys_take(struct pf_state *, > + struct pf_state_key **); > +void pf_state_keys_rele(struct pf_state_key **); > > #if NPFLOG > 0 > void pf_log_matches(struct pf_pdesc *, struct pf_rule *, > @@ -778,7 +782,8 @@ pf_state_key_attach(struct pf_state_key *sk, struct > pf_state *s, int idx) > s->key[idx] = sk; > > if ((si = pool_get(&pf_state_item_pl, PR_NOWAIT)) == NULL) { > - pf_state_key_detach(s, idx); > + pf_state_key_detach(s, s->key[idx]); > + s->key[idx] = NULL; > return (-1); > } > si->s = s; > @@ -798,42 +803,50 @@ pf_state_key_attach(struct pf_state_key *sk, struct > pf_state *s, int idx) > void > pf_detach_state(struct pf_state *s) > { > - if (s->key[PF_SK_WIRE] == s->key[PF_SK_STACK]) > - s->key[PF_SK_WIRE] = NULL; > + struct pf_state_key *key[2]; > + > + mtx_enter(&s->mtx); > + key[PF_SK_WIRE] = s->key[PF_SK_WIRE]; > + key[PF_SK_STACK] = s->key[PF_SK_STACK]; > + s->key[PF_SK_WIRE] = NULL; > + s->key[PF_SK_STACK] = NULL; > + mtx_leave(&s->mtx); > + > + if (key[PF_SK_WIRE] == key[PF_SK_STACK]) > + key[PF_SK_WIRE] = NULL; > > - if (s->key[PF_SK_STACK] != NULL) > - pf_state_key_detach(s, PF_SK_STACK); > + if (key[PF_SK_STACK] != NULL) > + pf_state_key_detach(s, key[PF_SK_STACK]); > > - if (s->key[PF_SK_WIRE] != NULL) > - pf_state_key_detach(s, PF_SK_WIRE); > + if (key[PF_SK_WIRE] != NULL) > + pf_state_key_detach(s, key[PF_SK_WIRE]); > } > > void > -pf_state_key_detach(struct pf_state *s, int idx) > +pf_state_key_detach(struct pf_state *s, struct pf_state_key *key) > { > struct pf_state_item *si; > - struct pf_state_key *sk; > > - if (s->key[idx] == NULL) > + PF_STATE_ASSERT_LOCKED(); > + > + if (key == NULL) > return; > > - si = TAILQ_FIRST(&s->key[idx]->states); > + si = TAILQ_FIRST(&key->states); > while (si && si->s != s) > si = TAILQ_NEXT(si, entry); > > if (si) { > - TAILQ_REMOVE(&s->key[idx]->states, si, entry); > + TAILQ_REMOVE(&key->states, si, entry); > pool_put(&pf_state_item_pl, si); > } > > - sk = s->key[idx]; > - s->key[idx] = NULL; > - if (TAILQ_EMPTY(&sk->states)) { > - RB_REMOVE(pf_state_tree, &pf_statetbl, sk); > - sk->removed = 1; > - pf_state_key_unlink_reverse(sk); > - pf_state_key_unlink_inpcb(sk); > - pf_state_key_unref(sk); > + if (TAILQ_EMPTY(&key->states)) { > + RB_REMOVE(pf_state_tree, &pf_statetbl, key); > + key->removed = 1; > + pf_state_key_unlink_reverse(key); > + pf_state_key_unlink_inpcb(key); > + pf_state_key_unref(key); > } > } > > @@ -996,7 +1009,9 @@ pf_state_insert(struct pfi_kif *kif, struct pf_state_key > **skw, > } > *skw = s->key[PF_SK_WIRE]; > if (pf_state_key_attach(*sks, s, PF_SK_STACK)) { > - pf_state_key_detach(s, PF_SK_WIRE); > + pf_state_key_detach(s, s->key[PF_SK_WIRE]); > + s->key[PF_SK_WIRE] = NULL; > + *skw = NULL; > PF_STATE_EXIT_WRITE(); > return (-1); > } > @@ -1185,30 +1200,35 @@ pf_find_state_all(struct pf_state_key_cmp *key, u_int > dir, int *more) > return (ret ? ret->s : NULL); > } > > -void > +int > pf_state_export(struct pfsync_state *sp, struct pf_state *st) > { > int32_t expire; > + struct pf_state_key *key[2] = { NULL, NULL }; > > memset(sp, 0, sizeof(struct pfsync_state)); > > /* copy from state key */ > - sp->key[PF_SK_WIRE].addr[0] = st->key[PF_SK_WIRE]->addr[0]; > - sp->key[PF_SK_WIRE].addr[1] = st->key[PF_SK_WIRE]->addr[1]; > - sp->key[PF_SK_WIRE].port[0] = st->key[PF_SK_WIRE]->port[0]; > - sp->key[PF_SK_WIRE].port[1] = st->key[PF_SK_WIRE]->port[1]; > - sp->key[PF_SK_WIRE].rdomain = htons(st->key[PF_SK_WIRE]->rdomain); > - sp->key[PF_SK_WIRE].af = st->key[PF_SK_WIRE]->af; > - sp->key[PF_SK_STACK].addr[0] = st->key[PF_SK_STACK]->addr[0]; > - sp->key[PF_SK_STACK].addr[1] = st->key[PF_SK_STACK]->addr[1]; > - sp->key[PF_SK_STACK].port[0] = st->key[PF_SK_STACK]->port[0]; > - sp->key[PF_SK_STACK].port[1] = st->key[PF_SK_STACK]->port[1]; > - sp->key[PF_SK_STACK].rdomain = htons(st->key[PF_SK_STACK]->rdomain); > - sp->key[PF_SK_STACK].af = st->key[PF_SK_STACK]->af; > + pf_state_keys_take(st, key); > + if ((key[PF_SK_WIRE] == NULL) || (key[PF_SK_STACK] == NULL)) > + return (-1); > + > + sp->key[PF_SK_WIRE].addr[0] = key[PF_SK_WIRE]->addr[0]; > + sp->key[PF_SK_WIRE].addr[1] = key[PF_SK_WIRE]->addr[1]; > + sp->key[PF_SK_WIRE].port[0] = key[PF_SK_WIRE]->port[0]; > + sp->key[PF_SK_WIRE].port[1] = key[PF_SK_WIRE]->port[1]; > + sp->key[PF_SK_WIRE].rdomain = htons(key[PF_SK_WIRE]->rdomain); > + sp->key[PF_SK_WIRE].af = key[PF_SK_WIRE]->af; > + sp->key[PF_SK_STACK].addr[0] = key[PF_SK_STACK]->addr[0]; > + sp->key[PF_SK_STACK].addr[1] = key[PF_SK_STACK]->addr[1]; > + sp->key[PF_SK_STACK].port[0] = key[PF_SK_STACK]->port[0]; > + sp->key[PF_SK_STACK].port[1] = key[PF_SK_STACK]->port[1]; > + sp->key[PF_SK_STACK].rdomain = htons(key[PF_SK_STACK]->rdomain); > + sp->key[PF_SK_STACK].af = key[PF_SK_STACK]->af; > sp->rtableid[PF_SK_WIRE] = htonl(st->rtableid[PF_SK_WIRE]); > sp->rtableid[PF_SK_STACK] = htonl(st->rtableid[PF_SK_STACK]); > - sp->proto = st->key[PF_SK_WIRE]->proto; > - sp->af = st->key[PF_SK_WIRE]->af; > + sp->proto = key[PF_SK_WIRE]->proto; > + sp->af = key[PF_SK_WIRE]->af; > > /* copy from state */ > strlcpy(sp->ifname, st->kif->pfik_name, sizeof(sp->ifname)); > @@ -1255,6 +1275,10 @@ pf_state_export(struct pfsync_state *sp, struct > pf_state *st) > sp->set_tos = st->set_tos; > sp->set_prio[0] = st->set_prio[0]; > sp->set_prio[1] = st->set_prio[1]; > + > + pf_state_keys_rele(key); > + > + return (0); > } > > /* END state table stuff */ > @@ -4108,6 +4132,7 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r, > struct pf_rule *a, > * pf_state_inserts() grabs reference for pfsync! > */ > refcnt_init(&s->refcnt); > + mtx_init(&s->mtx, IPL_NET); > > switch (pd->proto) { > case IPPROTO_TCP: > @@ -7789,3 +7814,19 @@ pf_pktenqueue_delayed(void *arg) > > pool_put(&pf_pktdelay_pl, pdy); > } > + > +void > +pf_state_keys_take(struct pf_state *st, struct pf_state_key *keys[]) > +{ > + mtx_enter(&st->mtx); > + keys[PF_SK_WIRE] = pf_state_key_ref(st->key[PF_SK_WIRE]); > + keys[PF_SK_STACK] = pf_state_key_ref(st->key[PF_SK_STACK]); > + mtx_leave(&st->mtx); > +} > + > +void > +pf_state_keys_rele(struct pf_state_key *keys[]) > +{ > + pf_state_key_unref(keys[PF_SK_WIRE]); > + pf_state_key_unref(keys[PF_SK_STACK]); > +} > diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h > index 93394e6da5c..4604869ede3 100644 > --- a/sys/net/pfvar.h > +++ b/sys/net/pfvar.h > @@ -763,6 +763,7 @@ struct pf_state { > struct pf_sn_head src_nodes; > struct pf_state_key *key[2]; /* addresses stack and wire */ > struct pfi_kif *kif; > + struct mutex mtx; > u_int64_t packets[2]; > u_int64_t bytes[2]; > int32_t creation; > @@ -1737,7 +1738,7 @@ void > pf_state_rm_src_node(struct pf_state *, > extern struct pf_state *pf_find_state_byid(struct pf_state_cmp > *); > extern struct pf_state *pf_find_state_all(struct > pf_state_key_cmp *, > u_int, int *); > -extern void pf_state_export(struct pfsync_state *, > +extern int pf_state_export(struct pfsync_state *, > struct pf_state *); > extern void pf_print_state(struct pf_state *); > extern void pf_print_flags(u_int8_t); >