From 1c3ff09ad37ab9532af634dd4fbc4845d7a4edf4 Mon Sep 17 00:00:00 2001 From: dlg Date: Tue, 15 Jun 2021 05:06:24 +0000 Subject: [PATCH] rework pfsync deferal timeout handling. instead of having a timeout per deferred packet structure, use a single timeout in pfsync that pulls items off the list of deferred packets. this avoids confusion about whether a timeout is handling the defer or another context owns it. this way round, the context that removes a defer from the list owns it and is responsible for completing it. this should fix a panic we hit on the firewalls at work. there's still another one that needs a fix, but sashan@ has been looking at it. this might make it simpler to deal with though. ok sashan@ jmatthew@ --- sys/net/if_pfsync.c | 83 ++++++++++++++++++++++++++++++++------------- 1 file changed, 60 insertions(+), 23 deletions(-) diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c index d82f31a8878..4f062f1c02e 100644 --- a/sys/net/if_pfsync.c +++ b/sys/net/if_pfsync.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_pfsync.c,v 1.289 2021/06/02 21:49:31 sashan Exp $ */ +/* $OpenBSD: if_pfsync.c,v 1.290 2021/06/15 05:06:24 dlg Exp $ */ /* * Copyright (c) 2002 Michael Shalayeff @@ -187,7 +187,7 @@ struct pfsync_deferral { TAILQ_ENTRY(pfsync_deferral) pd_entry; struct pf_state *pd_st; struct mbuf *pd_m; - struct timeout pd_tmo; + struct timeval pd_deadline; }; TAILQ_HEAD(pfsync_deferrals, pfsync_deferral); @@ -223,6 +223,7 @@ struct pfsync_softc { struct pfsync_deferrals sc_deferrals; u_int sc_deferred; struct mutex sc_deferrals_mtx; + struct timeout sc_deferrals_tmo; void *sc_plus; size_t sc_pluslen; @@ -273,7 +274,7 @@ void pfsync_ifdetach(void *); void pfsync_deferred(struct pf_state *, int); void pfsync_undefer(struct pfsync_deferral *, int); -void pfsync_defer_tmo(void *); +void pfsync_deferrals_tmo(void *); void pfsync_cancel_full_update(struct pfsync_softc *); void pfsync_request_full_update(struct pfsync_softc *); @@ -346,6 +347,7 @@ pfsync_clone_create(struct if_clone *ifc, int unit) mtx_init(&sc->sc_upd_req_mtx, IPL_SOFTNET); TAILQ_INIT(&sc->sc_deferrals); mtx_init(&sc->sc_deferrals_mtx, IPL_SOFTNET); + timeout_set_proc(&sc->sc_deferrals_tmo, pfsync_deferrals_tmo, sc); task_set(&sc->sc_ltask, pfsync_syncdev_state, sc); task_set(&sc->sc_dtask, pfsync_ifdetach, sc); sc->sc_deferred = 0; @@ -1931,6 +1933,9 @@ pfsync_defer(struct pf_state *st, struct mbuf *m) { struct pfsync_softc *sc = pfsyncif; struct pfsync_deferral *pd; + struct timeval now; + unsigned int sched; + static const struct timeval defer = { 0, 20000 }; NET_ASSERT_LOCKED(); @@ -1942,10 +1947,12 @@ pfsync_defer(struct pf_state *st, struct mbuf *m) if (sc->sc_deferred >= 128) { mtx_enter(&sc->sc_deferrals_mtx); pd = TAILQ_FIRST(&sc->sc_deferrals); - TAILQ_REMOVE(&sc->sc_deferrals, pd, pd_entry); - sc->sc_deferred--; + if (pd != NULL) { + TAILQ_REMOVE(&sc->sc_deferrals, pd, pd_entry); + sc->sc_deferred--; + } mtx_leave(&sc->sc_deferrals_mtx); - if (timeout_del(&pd->pd_tmo)) + if (pd != NULL) pfsync_undefer(pd, 0); } @@ -1959,13 +1966,18 @@ pfsync_defer(struct pf_state *st, struct mbuf *m) pd->pd_st = pf_state_ref(st); pd->pd_m = m; + getmicrouptime(&now); + timeradd(&now, &defer, &pd->pd_deadline); + mtx_enter(&sc->sc_deferrals_mtx); - sc->sc_deferred++; + sched = TAILQ_EMPTY(&sc->sc_deferrals); + TAILQ_INSERT_TAIL(&sc->sc_deferrals, pd, pd_entry); + sc->sc_deferred++; mtx_leave(&sc->sc_deferrals_mtx); - timeout_set_proc(&pd->pd_tmo, pfsync_defer_tmo, pd); - timeout_add_msec(&pd->pd_tmo, 20); + if (sched) + timeout_add_tv(&sc->sc_deferrals_tmo, &defer); schednetisr(NETISR_PFSYNC); @@ -2047,17 +2059,44 @@ pfsync_undefer(struct pfsync_deferral *pd, int drop) } void -pfsync_defer_tmo(void *arg) +pfsync_deferrals_tmo(void *arg) { - struct pfsync_softc *sc = pfsyncif; - struct pfsync_deferral *pd = arg; + struct pfsync_softc *sc = arg; + struct pfsync_deferral *pd; + struct timeval now, tv; + struct pfsync_deferrals pds = TAILQ_HEAD_INITIALIZER(pds); mtx_enter(&sc->sc_deferrals_mtx); - TAILQ_REMOVE(&sc->sc_deferrals, pd, pd_entry); - sc->sc_deferred--; + for (;;) { + pd = TAILQ_FIRST(&sc->sc_deferrals); + if (pd == NULL) + break; + + if (timercmp(&now, &pd->pd_deadline, <)) { + timersub(&now, &pd->pd_deadline, &tv); + break; + } + + TAILQ_REMOVE(&sc->sc_deferrals, pd, pd_entry); + sc->sc_deferred--; + TAILQ_INSERT_TAIL(&pds, pd, pd_entry); + } mtx_leave(&sc->sc_deferrals_mtx); + + if (pd != NULL) { + /* we were looking at a pd, but it wasn't old enough */ + timeout_add_tv(&sc->sc_deferrals_tmo, &tv); + } + + if (TAILQ_EMPTY(&pds)) + return; + NET_LOCK(); - pfsync_undefer(pd, 0); + while ((pd = TAILQ_FIRST(&pds)) != NULL) { + TAILQ_REMOVE(&pds, pd, pd_entry); + + pfsync_undefer(pd, 0); + } NET_UNLOCK(); } @@ -2072,17 +2111,15 @@ pfsync_deferred(struct pf_state *st, int drop) mtx_enter(&sc->sc_deferrals_mtx); TAILQ_FOREACH(pd, &sc->sc_deferrals, pd_entry) { if (pd->pd_st == st) { - if (timeout_del(&pd->pd_tmo)) { - TAILQ_REMOVE(&sc->sc_deferrals, pd, pd_entry); - sc->sc_deferred--; - mtx_leave(&sc->sc_deferrals_mtx); - pfsync_undefer(pd, drop); - } else - mtx_leave(&sc->sc_deferrals_mtx); - return; + TAILQ_REMOVE(&sc->sc_deferrals, pd, pd_entry); + sc->sc_deferred--; + break; } } mtx_leave(&sc->sc_deferrals_mtx); + + if (pd != NULL) + pfsync_undefer(pd, drop); } void -- 2.20.1