From: dlg <dlg@openbsd.org>
Date: Tue, 8 Aug 2023 10:14:29 +0000 (+0000)
Subject: try to avoid a deadlock between sec_down and sec_send.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=e2ace5e53c443884629c14b9c2f46f1cb5ff48aa;p=openbsd

try to avoid a deadlock between sec_down and sec_send.

sec_send runs in the systq (because it calls ipsec stuff which uses
crypto, which is not mpsafe) and takes the net lock (because ipsec
output calls ip_output and other stuff). sec_down is called with
NET_LOCK held and tries to run a barrier for send task. if the send
task is running but is waiting for the net lock while sec_down is
holding the net lock while waiting for the task to finish, we're
deadlocked.

this copies the sc_up thing from pfsync, which hopefuly avoids this.

pointed out by mvs@
---

diff --git a/sys/net/if_sec.c b/sys/net/if_sec.c
index d41f2c3d3f5..f78acec715d 100644
--- a/sys/net/if_sec.c
+++ b/sys/net/if_sec.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_sec.c,v 1.1 2023/08/07 01:57:33 dlg Exp $ */
+/*	$OpenBSD: if_sec.c,v 1.2 2023/08/08 10:14:29 dlg Exp $ */
 
 /*
  * Copyright (c) 2022 The University of Queensland
@@ -83,6 +83,7 @@
 
 struct sec_softc {
 	struct ifnet			sc_if;
+	unsigned int			sc_up;
 
 	struct task			sc_send;
 
@@ -237,10 +238,19 @@ sec_up(struct sec_softc *sc)
 	unsigned int idx = stoeplitz_h32(sc->sc_unit) % nitems(sec_map);
 
 	NET_ASSERT_LOCKED();
+	KASSERT(!ISSET(ifp->if_flags, IFF_RUNNING));
 
-	SET(ifp->if_flags, IFF_RUNNING);
-	refcnt_init(&sc->sc_refs);
+	/*
+	 * coordinate with sec_down(). if sc_up is still up and
+	 * we're here then something else is running sec_down.
+	 */
+	if (sc->sc_up)
+		return (EBUSY);
+
+	sc->sc_up = 1;
 
+	refcnt_init(&sc->sc_refs);
+	SET(ifp->if_flags, IFF_RUNNING);
 	SMR_SLIST_INSERT_HEAD_LOCKED(&sec_map[idx], sc, sc_entry);
 
 	return (0);
@@ -253,16 +263,28 @@ sec_down(struct sec_softc *sc)
 	unsigned int idx = stoeplitz_h32(sc->sc_unit) % nitems(sec_map);
 
 	NET_ASSERT_LOCKED();
+	KASSERT(ISSET(ifp->if_flags, IFF_RUNNING));
 
-	CLR(ifp->if_flags, IFF_RUNNING);
+	/*
+	 * taking sec down involves waiting for it to stop running
+	 * in various contexts. this thread cannot hold netlock
+	 * while waiting for a barrier for a task that could be trying
+	 * to take netlock itself. so give up netlock, but don't clear
+	 * sc_up to prevent sec_up from running.
+	 */
 
-	SMR_SLIST_REMOVE_LOCKED(&sec_map[idx], sc, sec_softc, sc_entry);
+	CLR(ifp->if_flags, IFF_RUNNING);
+	NET_UNLOCK();
 
 	smr_barrier();
 	taskq_del_barrier(systq, &sc->sc_send);
 
 	refcnt_finalize(&sc->sc_refs, "secdown");
 
+	NET_LOCK();
+	SMR_SLIST_REMOVE_LOCKED(&sec_map[idx], sc, sec_softc, sc_entry);
+	sc->sc_up = 0;
+
 	return (0);
 }