From: Stefan Fritsch <sf@sfritsch.de>
Subject: Don't release NET_LOCK in vio(4) ioctl
To: tech@openbsd.org
Date: Fri, 12 Apr 2024 14:05:05 +0200

Hi,

commits f0b002d01d5 "Release the netlock when sleeping for control 
messages in in vioioctl()" and 126b881f71 "Insert a workaround for per-ifp 
ioctl being called w/o NET_LOCK()." in vio(4) fixed a deadlock but may 
cause a crash with a protection fault trap at rt_ifa_del if addresses are 
added/removed concurrently.
    
Reproducible by running

while true; do ifconfig vio0 -inet; done &
while true; do ifconfig vio0 10.1.6.11/24; done &
while true; do ifconfig vio0 down; done &
while true; do ifconfig vio0 up; done &
while true; do ifconfig vio0 -inet6 ; done &
while true; do ifconfig vio0 inet6 2001:a61:3446:ddff:94be:52d1:d57c:1/64 ; done &

in parallel.

The diff below reverts the two above commits and changes the vio_ctrl_* 
functions to prevent any concurrent control queue requests while the 
device is being reset. I have seen no deadlocks or crashes with that 
patch.

Comments? OKs?

The whole locking in vio(4) needs some overhaul, but I would postpone that 
until vio(4) is unlocked and extended to multi-queue. Is there a good 
example of how to be able to sleep in the ioctl path in some other network 
driver? cad(4) uses a rwlock and seems to jump through quite a few hoops 
to get it right. Is there a better example?

Cheers,
Stefan

diff --git a/sys/dev/pv/if_vio.c b/sys/dev/pv/if_vio.c
index 9ad8e36e346..633c00f96b4 100644
--- a/sys/dev/pv/if_vio.c
+++ b/sys/dev/pv/if_vio.c
@@ -702,8 +702,10 @@ vio_stop(struct ifnet *ifp, int disable)
 	/* only way to stop I/O and DMA is resetting... */
 	virtio_reset(vsc);
 	vio_rxeof(sc);
-	if (vsc->sc_nvqs >= 3)
+	if (vsc->sc_nvqs >= 3) {
+		sc->sc_ctrl_inuse = RESET;
 		vio_ctrleof(&sc->sc_vq[VQCTL]);
+	}
 	vio_tx_drain(sc);
 	if (disable)
 		vio_rx_drain(sc);
@@ -715,8 +717,8 @@ vio_stop(struct ifnet *ifp, int disable)
 		virtio_start_vq_intr(vsc, &sc->sc_vq[VQCTL]);
 	virtio_reinit_end(vsc);
 	if (vsc->sc_nvqs >= 3) {
-		if (sc->sc_ctrl_inuse != FREE)
-			sc->sc_ctrl_inuse = RESET;
+		KASSERT(sc->sc_ctrl_inuse == RESET);
+		sc->sc_ctrl_inuse = FREE;
 		wakeup(&sc->sc_ctrl_inuse);
 	}
 }
@@ -1232,13 +1234,17 @@ vio_txeof(struct virtqueue *vq)
 
 	while (virtio_dequeue(vsc, vq, &slot, &len) == 0) {
 		struct virtio_net_hdr *hdr = &sc->sc_tx_hdrs[slot];
+		m = sc->sc_tx_mbufs[slot];
+		if (m == NULL) {
+			// interface is down by now
+			continue;
+		}
 		r++;
 		VIO_DMAMEM_SYNC(vsc, sc, hdr, sc->sc_hdr_size,
 		    BUS_DMASYNC_POSTWRITE);
 		bus_dmamap_sync(vsc->sc_dmat, sc->sc_tx_dmamaps[slot], 0,
 		    sc->sc_tx_dmamaps[slot]->dm_mapsize,
 		    BUS_DMASYNC_POSTWRITE);
-		m = sc->sc_tx_mbufs[slot];
 		bus_dmamap_unload(vsc->sc_dmat, sc->sc_tx_dmamaps[slot]);
 		sc->sc_tx_mbufs[slot] = NULL;
 		virtio_dequeue_commit(vq, slot);
@@ -1363,30 +1369,13 @@ out:
 	return r;
 }
 
-/*
- * XXXSMP As long as some per-ifp ioctl(2)s are executed with the
- * NET_LOCK() deadlocks are possible.  So release it here.
- */
-static inline int
-vio_sleep(struct vio_softc *sc, const char *wmesg)
-{
-	int status = rw_status(&netlock);
-
-	if (status != RW_WRITE && status != RW_READ)
-		return tsleep_nsec(&sc->sc_ctrl_inuse, PRIBIO|PCATCH, wmesg,
-		    INFSLP);
-
-	return rwsleep_nsec(&sc->sc_ctrl_inuse, &netlock, PRIBIO|PCATCH, wmesg,
-	    INFSLP);
-}
-
 int
 vio_wait_ctrl(struct vio_softc *sc)
 {
 	int r = 0;
 
 	while (sc->sc_ctrl_inuse != FREE) {
-		r = vio_sleep(sc, "viowait");
+		r = tsleep_nsec(&sc->sc_ctrl_inuse, PRIBIO|PCATCH, "viowait", INFSLP);
 		if (r == EINTR)
 			return r;
 	}
@@ -1400,12 +1389,12 @@ vio_wait_ctrl_done(struct vio_softc *sc)
 {
 	int r = 0;
 
-	while (sc->sc_ctrl_inuse != DONE && sc->sc_ctrl_inuse != RESET) {
+	while (sc->sc_ctrl_inuse != DONE) {
 		if (sc->sc_ctrl_inuse == RESET) {
-			r = 1;
+			r = ENXIO;
 			break;
 		}
-		r = vio_sleep(sc, "viodone");
+		r = tsleep_nsec(&sc->sc_ctrl_inuse, PRIBIO|PCATCH, "viodone", INFSLP);
 		if (r == EINTR)
 			break;
 	}
@@ -1415,7 +1404,8 @@ vio_wait_ctrl_done(struct vio_softc *sc)
 void
 vio_ctrl_wakeup(struct vio_softc *sc, enum vio_ctrl_state new)
 {
-	sc->sc_ctrl_inuse = new;
+	if (sc->sc_ctrl_inuse != RESET)
+		sc->sc_ctrl_inuse = new;
 	wakeup(&sc->sc_ctrl_inuse);
 }