Hello,
this is yet another fall out from my experiments. It has turned out my initial
change introduced some time ago [1] omits pfsyncintr() and pfsync_sendout().
Both functions call pfsync_sendout(), which currently requires protection
of PF_LOCK(). Without change below the test box was dying on ASSERT()
in pf_state_uref() at line 7529.
7520 pf_state_unref(struct pf_state *s)
7521 {
7522 if ((s != NULL) && PF_REF_RELE(s->refcnt)) {
7523 /* never inserted or removed */
7524 #if NPFSYNC > 0
7525 KASSERT((TAILQ_NEXT(s, sync_list) == NULL) ||
7526 ((TAILQ_NEXT(s, sync_list) == _Q_INVALID) &&
7527 (s->sync_state == PFSYNC_S_NONE)));
7528 #endif /* NPFSYNC */
7529 KASSERT((TAILQ_NEXT(s, entry_list) == NULL) ||
7530 (TAILQ_NEXT(s, entry_list) == _Q_INVALID));
7531 KASSERT((s->key[PF_SK_WIRE] == NULL) &&
7532 (s->key[PF_SK_STACK] == NULL));
7533
7534 pool_put(&pf_state_pl, s);
7535 }
7536 }
The assert was being triggered from various code paths. Most frequently
from pfsync_update_state(). Occurrence of pf_state_unref() at pf_test()
or timer thread were very rare.
OK?
thanks and
regards
sashan
[1]
https://github.com/openbsd/src/commit/ddb22b9530d7ad1b4698803d7b6dcb21c96a3f7d#diff-9517dfce4e8db974781a4536fd38cfc1
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c
index 20a0136e838..e891832b3ab 100644
--- a/sys/net/if_pfsync.c
+++ b/sys/net/if_pfsync.c
@@ -1581,6 +1581,8 @@ pfsync_sendout(void)
int offset;
int q, count = 0;
+ PF_ASSERT_LOCKED();
+
if (sc == NULL || sc->sc_len == PFSYNC_MINPKT)
return;
@@ -2472,7 +2474,9 @@ void
pfsync_timeout(void *arg)
{
NET_LOCK();
+ PF_LOCK();
pfsync_sendout();
+ PF_UNLOCK();
NET_UNLOCK();
}
@@ -2480,7 +2484,9 @@ pfsync_timeout(void *arg)
void
pfsyncintr(void)
{
+ PF_LOCK();
pfsync_sendout();
+ PF_UNLOCK();
}
int
