Hello,
On Tue, Aug 09, 2022 at 06:47:54PM +0200, Hrvoje Popovski wrote:
> Hi all,
>
> I'm running
> OpenBSD 7.2-beta (GENERIC.MP) #651: Tue Jul 26 23:11:26 MDT 2022
> dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
>
> on production firewall and for few weeks it was stable. Firewall panic
> today and I will sysupgrade it, but maybe this panic message is
> interesting so I'm sending it here.
>
>
> bcbnfw1# uvm_fault(0xffffffff823a1a20, 0x0, 0, 1) -> e
> kernel: page fault trap, code=0
> Stopped at pf_state_export+0x38: movq 0(%rax),%rcx
> TID PID UID PRFLAGS PFLAGS CPU COMMAND
> 309438 83954 0 0x14000 0x200 1 softnet
> 486408 53515 0 0x14000 0x200 3 softnet
> * 80122 54608 0 0x14000 0x200 2 softnet
> pf_state_export(fffffd806152f9dc,fffffd8664eb12b0) at pf_state_export+0x38
> pfsync_sendout() at pfsync_sendout+0x5e4
> pfsync_update_state(fffffd8728968d40) at pfsync_update_state+0x15b
> pf_test(2,1,ffff800000bbb000,ffff800020c336d8) at pf_test+0x117a
> ip_input_if(ffff800020c336d8,ffff800020c336e4,4,0,ffff800000bbb000) at
> ip_input_if+0xcd
> ipv4_input(ffff800000bbb000,fffffd80661d5300) at ipv4_input+0x39
> ether_input(ffff800000bbb000,fffffd80661d5300) at ether_input+0x3b1
> carp_input(ffff800000bd2000,fffffd80661d5300,5e000101) at carp_input+0x196
> ether_input(ffff800000bd2000,fffffd80661d5300) at ether_input+0x1d9
> vlan_input(ffff800000b9d000,fffffd80661d5300,ffff800020c3390c) at
> vlan_input+0x23d
> ether_input(ffff800000b9d000,fffffd80661d5300) at ether_input+0x85
> if_input_process(ffff80000048b048,ffff800020c339a8) at if_input_process+0x6f
> ifiq_process(ffff80000048ea00) at ifiq_process+0x69
> taskq_thread(ffff800000035080) at taskq_thread+0x100
> end trace frame: 0x0, count: 1
> https://urldefense.com/v3/__https://www.openbsd.org/ddb.html__;!!ACWV5N9M2RV99hQ!IOiwmHap_PoZFHkbnG9qtmjA5iScoEpS7aLV1rsiWFh15vslrJlIXq44u3rPHpnKd7BcyB9J7lkrPKVEADwkHi4$
> describes the minimum info required in
> bug reports. Insufficient info makes it difficult to find and fix bugs.
>
>
> ddb{2}> show reg
> rdi 0xfffffd806152fae4
> rsi 0
> rbp 0xffff800020c33340
> rbx 0x19c
> rdx 0x4
> rcx 0
> rax 0
> r8 0x104
> r9 0x7d788a8c5153bdc
> r10 0x92a5ce4f38be8823
> r11 0xfffffd806152f9dc
> r12 0xfffffd8664eb12b0
> r13 0
> r14 0xfffffd806152f9dc
> r15 0xfffffd8664eb12b0
> rip 0xffffffff81387678 pf_state_export+0x38
> cs 0x8
> rflags 0x10246 __ALIGN_SIZE+0xf246
> rsp 0xffff800020c33300
> ss 0x10
> pf_state_export+0x38: movq 0(%rax),%rcx
>
this is a NULL pointer dereference panic. I think we've seen it few months
back. patch below was applied to one of your test machines if I remember
correct. can you give it a try again to see if it will help?
the change adds a mutex to pf_state structure to protect references
to keys attached to state.
we also have to take into account a fact that pf_state_export() may be
presented with state which keys got detached. Hence we have to
skip such state when doing export. Therefore pf_state_export()
indicates a failure to hint caller whether data were written (success)
and we should move to next free slot in output buffer. Or nothing
got written (failure) and current slot in output buffer is still free.
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 7183db91254..6e41ef49176 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 *,
@@ -779,7 +783,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;
@@ -799,42 +804,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);
}
}
@@ -997,7 +1010,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);
}
@@ -1187,30 +1202,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));
@@ -1257,6 +1277,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 */
@@ -4110,6 +4134,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:
@@ -7780,3 +7805,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 6f88ff687ed..986fa329b16 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -764,6 +764,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;
@@ -1738,7 +1739,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);
