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);
>
