On 10.5.2023. 0:24, Alexandr Nedvedicky wrote: > Hello, > > > On Tue, May 09, 2023 at 06:26:43PM +0000, mabi wrote: >> Hi, >> >> On a brand new OpenBSD 7.3 firewall (amd64) I get a kernel panic every few >> days and was wondering if this panic I get is related to this issue/bug? >> > > your panic got fixed by recent commit [1] > > Hrvoje was/is hitting very close to that KASSERT() (now removed) at line 2274. > in Hrvoje's case the TAILQ_REMOVE() macro complains we attempt to remove state > which is removed already: > > 2273 atomic_sub_long(&sc->sc_len, pfsync_qs[q].len); > 2274 TAILQ_REMOVE(&sc->sc_qs[q], st, sync_list); > 2275 if (TAILQ_EMPTY(&sc->sc_qs[q])) > 2276 atomic_sub_long(&sc->sc_len, sizeof (struct > pfsync_subheader)); > 2277 st->sync_state = PFSYNC_S_NONE; > 2278 mtx_leave(&sc->sc_st_mtx); > 2279 > 2280 pf_state_unref(st); > > the cause is very similar pfsync relies on volatile value in ->sync_state > member. > The ->sync_state member must be modified under protection of ->mtx. > > The issue has been pointed out by bluhm@ during m2k23 hackathon where I shared > my pfsync(4) headache with him. Diff below is my attempt to fix it. I had no > chance to test it. I'll appreciate If you will give it a try and let me know > how things look like. > > thanks and > regards > sashan > > [1] https://marc.info/?l=openbsd-cvs&m=168269695603160&w=2 > >
Hi, with this diff I can't trigger panic below r620-1# uvm_fault(0xffffffff82598710, 0x17, 0, 2) -> e kernel: page fault trap, code=2 Stopped at pfsync_q_del+0x8d: movq %rdx,0x8(%rax) TID PID UID PRFLAGS PFLAGS CPU COMMAND *254020 58090 0 0x14000 0x200 0K systq pfsync_q_del(fffffd8360adf6e0) at pfsync_q_del+0x8d pfsync_delete_state(fffffd8360adf6e0) at pfsync_delete_state+0x118 pf_remove_state(fffffd8360adf6e0) at pf_remove_state+0x156 pf_purge_expired_states(2e9c1) at pf_purge_expired_states+0x273 pf_purge(ffffffff8258caa0) at pf_purge+0x2c taskq_thread(ffffffff824a8150) at taskq_thread+0x100 end trace frame: 0x0, count: 9 https://www.openbsd.org/ddb.html describes the minimum info required in bug reports. Insufficient info makes it difficult to find and fix bugs. ddb{0}> I'm only being able to trigger it in lab and quite fast. Now, after few hours it's still stable. > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c > index 822b4211d0f..811d9d59666 100644 > --- a/sys/net/if_pfsync.c > +++ b/sys/net/if_pfsync.c > @@ -1362,14 +1362,17 @@ pfsync_grab_snapshot(struct pfsync_snapshot *sn, > struct pfsync_softc *sc) > > while ((st = TAILQ_FIRST(&sc->sc_qs[q])) != NULL) { > TAILQ_REMOVE(&sc->sc_qs[q], st, sync_list); > + mtx_enter(&st->mtx); > if (st->snapped == 0) { > TAILQ_INSERT_TAIL(&sn->sn_qs[q], st, sync_snap); > st->snapped = 1; > + mtx_leave(&st->mtx); > } else { > /* > * item is on snapshot list already, so we can > * skip it now. > */ > + mtx_leave(&st->mtx); > pf_state_unref(st); > } > } > @@ -1422,11 +1425,13 @@ pfsync_drop_snapshot(struct pfsync_snapshot *sn) > continue; > > while ((st = TAILQ_FIRST(&sn->sn_qs[q])) != NULL) { > + mtx_enter(&st->mtx); > KASSERT(st->sync_state == q); > KASSERT(st->snapped == 1); > TAILQ_REMOVE(&sn->sn_qs[q], st, sync_snap); > st->sync_state = PFSYNC_S_NONE; > st->snapped = 0; > + mtx_leave(&st->mtx); > pf_state_unref(st); > } > } > @@ -1665,6 +1670,7 @@ pfsync_sendout(void) > > count = 0; > while ((st = TAILQ_FIRST(&sn.sn_qs[q])) != NULL) { > + mtx_enter(&st->mtx); > TAILQ_REMOVE(&sn.sn_qs[q], st, sync_snap); > KASSERT(st->sync_state == q); > KASSERT(st->snapped == 1); > @@ -1672,6 +1678,7 @@ pfsync_sendout(void) > st->snapped = 0; > pfsync_qs[q].write(st, m->m_data + offset); > offset += pfsync_qs[q].len; > + mtx_leave(&st->mtx); > > pf_state_unref(st); > count++; > @@ -1725,8 +1732,6 @@ pfsync_insert_state(struct pf_state *st) > ISSET(st->state_flags, PFSTATE_NOSYNC)) > return; > > - KASSERT(st->sync_state == PFSYNC_S_NONE); > - > if (sc->sc_len == PFSYNC_MINPKT) > timeout_add_sec(&sc->sc_tmo, 1); > > @@ -2221,6 +2226,7 @@ pfsync_q_ins(struct pf_state *st, int q) > panic("pfsync pkt len is too low %zd", sc->sc_len); > do { > mtx_enter(&sc->sc_st_mtx); > + mtx_enter(&st->mtx); > > /* > * There are either two threads trying to update the > @@ -2228,6 +2234,7 @@ pfsync_q_ins(struct pf_state *st, int q) > * (is on snapshot queue). > */ > if (st->sync_state != PFSYNC_S_NONE) { > + mtx_leave(&st->mtx); > mtx_leave(&sc->sc_st_mtx); > break; > } > @@ -2240,6 +2247,7 @@ pfsync_q_ins(struct pf_state *st, int q) > sclen = atomic_add_long_nv(&sc->sc_len, nlen); > if (sclen > sc->sc_if.if_mtu) { > atomic_sub_long(&sc->sc_len, nlen); > + mtx_leave(&st->mtx); > mtx_leave(&sc->sc_st_mtx); > pfsync_sendout(); > continue; > @@ -2249,6 +2257,7 @@ pfsync_q_ins(struct pf_state *st, int q) > > TAILQ_INSERT_TAIL(&sc->sc_qs[q], st, sync_list); > st->sync_state = q; > + mtx_leave(&st->mtx); > mtx_leave(&sc->sc_st_mtx); > } while (0); > } > @@ -2260,6 +2269,7 @@ pfsync_q_del(struct pf_state *st) > int q; > > mtx_enter(&sc->sc_st_mtx); > + mtx_enter(&st->mtx); > q = st->sync_state; > /* > * re-check under mutex > @@ -2267,6 +2277,7 @@ pfsync_q_del(struct pf_state *st) > * too late, the state is being just processed/dispatched to peer. > */ > if ((q == PFSYNC_S_NONE) || (st->snapped)) { > + mtx_leave(&st->mtx); > mtx_leave(&sc->sc_st_mtx); > return; > } > @@ -2275,6 +2286,7 @@ pfsync_q_del(struct pf_state *st) > if (TAILQ_EMPTY(&sc->sc_qs[q])) > atomic_sub_long(&sc->sc_len, sizeof (struct pfsync_subheader)); > st->sync_state = PFSYNC_S_NONE; > + mtx_leave(&st->mtx); > mtx_leave(&sc->sc_st_mtx); > > pf_state_unref(st); >