To fix crashes seen by Hrvoje with pfsync, IPsec and parallel

forwarding, protect tdb flags and lists in pfsync with a mutex.
help and OK sashan@

1 files changed, 30 insertions, 8 deletions

diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c
index 4c538ca7280..edeb2209161 100644
--- a/sys/net/if_pfsync.c
+++ b/sys/net/if_pfsync.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_pfsync.c,v 1.299 2021/11/25 13:46:02 bluhm Exp $	*/
+/*	$OpenBSD: if_pfsync.c,v 1.300 2022/02/25 22:18:44 bluhm Exp $	*/
 
 /*
  * Copyright (c) 2002 Michael Shalayeff
@@ -295,7 +295,7 @@ void	pfsync_bulk_update(void *);
 void	pfsync_bulk_fail(void *);
 
 void	pfsync_grab_snapshot(struct pfsync_snapshot *, struct pfsync_softc *);
-void	pfsync_drop_snapshot(struct pfsync_snapshot *);
+void	pfsync_drop_snapshot(struct pfsync_snapshot *, struct pfsync_softc *);
 
 void	pfsync_send_dispatch(void *);
 void	pfsync_send_pkt(struct mbuf *);
@@ -1618,7 +1618,7 @@ pfsync_grab_snapshot(struct pfsync_snapshot *sn, struct pfsync_softc *sc)
 }
 
 void
-pfsync_drop_snapshot(struct pfsync_snapshot *sn)
+pfsync_drop_snapshot(struct pfsync_snapshot *sn, struct pfsync_softc * sc)
 {
 	struct pf_state *st;
 	struct pfsync_upd_req_item *ur;
@@ -1645,10 +1645,14 @@ pfsync_drop_snapshot(struct pfsync_snapshot *sn)
 		pool_put(&sn->sn_sc->sc_pool, ur);
 	}
 
+	mtx_enter(&sc->sc_tdb_mtx);
 	while ((t = TAILQ_FIRST(&sn->sn_tdb_q)) != NULL) {
 		TAILQ_REMOVE(&sn->sn_tdb_q, t, tdb_sync_entry);
+		mtx_enter(&t->tdb_mtx);
 		CLR(t->tdb_flags, TDBF_PFSYNC);
+		mtx_leave(&t->tdb_mtx);
 	}
+	mtx_leave(&sc->sc_tdb_mtx);
 }
 
 int
@@ -1675,7 +1679,7 @@ pfsync_drop(struct pfsync_softc *sc)
 	struct pfsync_snapshot	sn;
 
 	pfsync_grab_snapshot(&sn, sc);
-	pfsync_drop_snapshot(&sn);
+	pfsync_drop_snapshot(&sn, sc);
 }
 
 void
@@ -1767,7 +1771,7 @@ pfsync_sendout(void)
 	if (m == NULL) {
 		sc->sc_if.if_oerrors++;
 		pfsyncstat_inc(pfsyncs_onomem);
-		pfsync_drop_snapshot(&sn);
+		pfsync_drop_snapshot(&sn, sc);
 		return;
 	}
 
@@ -1777,7 +1781,7 @@ pfsync_sendout(void)
 			m_free(m);
 			sc->sc_if.if_oerrors++;
 			pfsyncstat_inc(pfsyncs_onomem);
-			pfsync_drop_snapshot(&sn);
+			pfsync_drop_snapshot(&sn, sc);
 			return;
 		}
 	}
@@ -1835,14 +1839,18 @@ pfsync_sendout(void)
 		subh = (struct pfsync_subheader *)(m->m_data + offset);
 		offset += sizeof(*subh);
 
+		mtx_enter(&sc->sc_tdb_mtx);
 		count = 0;
 		while ((t = TAILQ_FIRST(&sn.sn_tdb_q)) != NULL) {
 			TAILQ_REMOVE(&sn.sn_tdb_q, t, tdb_sync_entry);
 			pfsync_out_tdb(t, m->m_data + offset);
 			offset += sizeof(struct pfsync_tdb);
+			mtx_enter(&t->tdb_mtx);
 			CLR(t->tdb_flags, TDBF_PFSYNC);
+			mtx_leave(&t->tdb_mtx);
 			count++;
 		}
+		mtx_leave(&sc->sc_tdb_mtx);
 
 		bzero(subh, sizeof(*subh));
 		subh->action = PFSYNC_ACT_TDB;
@@ -2477,21 +2485,31 @@ pfsync_update_tdb(struct tdb *t, int output)
 			mtx_enter(&sc->sc_tdb_mtx);
 			nlen = sizeof(struct pfsync_tdb);
 
+			mtx_enter(&t->tdb_mtx);
+			if (ISSET(t->tdb_flags, TDBF_PFSYNC)) {
+				/* we've lost race, no action for us then */
+				mtx_leave(&t->tdb_mtx);
+				mtx_leave(&sc->sc_tdb_mtx);
+				break;
+			}
+
 			if (TAILQ_EMPTY(&sc->sc_tdb_q))
 				nlen += sizeof(struct pfsync_subheader);
 
 			sc_len = atomic_add_long_nv(&sc->sc_len, nlen);
 			if (sc_len > sc->sc_if.if_mtu) {
 				atomic_sub_long(&sc->sc_len, nlen);
+				mtx_leave(&t->tdb_mtx);
 				mtx_leave(&sc->sc_tdb_mtx);
 				pfsync_sendout();
 				continue;
 			}
 
 			TAILQ_INSERT_TAIL(&sc->sc_tdb_q, t, tdb_sync_entry);
-			mtx_leave(&sc->sc_tdb_mtx);
-
 			SET(t->tdb_flags, TDBF_PFSYNC);
+			mtx_leave(&t->tdb_mtx);
+
+			mtx_leave(&sc->sc_tdb_mtx);
 			t->tdb_updates = 0;
 		} while (0);
 	} else {
@@ -2499,10 +2517,12 @@ pfsync_update_tdb(struct tdb *t, int output)
 			schednetisr(NETISR_PFSYNC);
 	}
 
+	mtx_enter(&t->tdb_mtx);
 	if (output)
 		SET(t->tdb_flags, TDBF_PFSYNC_RPL);
 	else
 		CLR(t->tdb_flags, TDBF_PFSYNC_RPL);
+	mtx_leave(&t->tdb_mtx);
 }
 
 void
@@ -2517,7 +2537,9 @@ pfsync_delete_tdb(struct tdb *t)
 	mtx_enter(&sc->sc_tdb_mtx);
 
 	TAILQ_REMOVE(&sc->sc_tdb_q, t, tdb_sync_entry);
+	mtx_enter(&t->tdb_mtx);
 	CLR(t->tdb_flags, TDBF_PFSYNC);
+	mtx_leave(&t->tdb_mtx);
 
 	nlen = sizeof(struct pfsync_tdb);
 	if (TAILQ_EMPTY(&sc->sc_tdb_q))