From: Alexander Bluhm Subject: Re: Don't release NET_LOCK in vio(4) ioctl To: Vitaliy Makkoveev Cc: Stefan Fritsch , tech@openbsd.org Date: Tue, 16 Apr 2024 22:58:24 +0200 On Fri, Apr 12, 2024 at 03:38:44PM +0300, Vitaliy Makkoveev wrote: > On Fri, Apr 12, 2024 at 02:05:05PM +0200, Stefan Fritsch wrote: > > 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? > > > > Sleep with netlock held stops packet processing and operations in inet > sockets. This is much worse when use another lock with netlock release. > pflowioctl() could be good example. Holding net lock for a long time is not good. But kernel crashes are worse. I prefer short stop of packet processing during ifconfig over a crash in netinet code. Here is the original bug report https://marc.info/?l=openbsd-bugs&m=166124228103568&w=2 I would try get this in and work on fine grained vio(4) locking afterwards. bluhm > > 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); > > } > > > >