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 d41f2c3..f78acec 100644 (file)
--- 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);
 }