On Tue, Jul 04, 2023 at 03:26:30PM +1000, David Gwynne wrote:
> tl;dr: this adds sec(4) p2p ip interfaces. Traffic in and out of these
> interfaces is protected by IPsec security associations (SAs), but
> there's no flows (security policy database (SPD) entries) associated
> with these SAs. The policy for using the sec(4) interfaces and their
> SAs is route-based instead.
>
Hi,
I don't like this sleep with netlock held. sec_send() handler has the
netlock provided sleep point, so concurrent sec_ioctl() could grab
netlock and call sec_down() while sec_send() awaiting netlock release.
So sec_down() will sleep with netlock held awaiting sec_send() to finish
and sec_send() will sleep awaiting netlock acquisition. Smells like
deadlock.
Could you move taskq_del_barrier() and refcnt_finalize() to
sec_clone_destroy() to prevent this?
> +static int
> +sec_down(struct sec_softc *sc)
> +{
> + struct ifnet *ifp = &sc->sc_if;
> + unsigned int idx = stoeplitz_h32(sc->sc_unit) % nitems(sec_map);
> +
> + NET_ASSERT_LOCKED();
> +
> + CLR(ifp->if_flags, IFF_RUNNING);
> +
> + SMR_SLIST_REMOVE_LOCKED(&sec_map[idx], sc, sec_softc, sc_entry);
> +
> + smr_barrier();
> + taskq_del_barrier(systq, &sc->sc_send);
> +
> + refcnt_finalize(&sc->sc_refs, "secdown");
> +
> + return (0);
> +}
> +
> +static void
> +sec_send(void *arg)
> +{
> + struct sec_softc *sc = arg;
> + struct ifnet *ifp = &sc->sc_if;
> + struct ifqueue *ifq = &ifp->if_snd;
> + struct tdb *tdb;
> + struct mbuf *m;
> + int error;
> +
> + if (!ISSET(ifp->if_flags, IFF_RUNNING))
> + return;
> +
> + tdb = sec_tdb_get(sc->sc_unit);
> + if (tdb == NULL)
> + goto purge;
> +
> + NET_LOCK();
> + while ((m = ifq_dequeue(ifq)) != NULL) {
> + CLR(m->m_flags, M_BCAST|M_MCAST);
> +
> +#if NPF > 0
> + pf_pkt_addr_changed(m);
> +#endif
> +
> + error = ipsp_process_packet(m, tdb,
> + m->m_pkthdr.ph_family, /* already tunnelled? */ 0);
> + if (error != 0)
> + counters_inc(ifp->if_counters, ifc_oerrors);
> + }
> + NET_UNLOCK();
> +
> + tdb_unref(tdb);
> + return;
> +
> +purge:
> + counters_add(ifp->if_counters, ifc_oerrors, ifq_purge(ifq));
> +}
> +
Nothing denies sec_start() be MP safe. No reason to wait or stop the
rest of kernel locked code here.
> +static void
> +sec_start(struct ifnet *ifp)
> +{
> + counters_add(ifp->if_counters, ifc_oerrors, ifq_purge(&ifp->if_snd));
> +}
