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@

1 files 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