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