From: Stefan Fritsch Subject: Re: Don't release NET_LOCK in vio(4) ioctl To: Alexander Bluhm Cc: Vitaliy Makkoveev , tech@openbsd.org Date: Tue, 16 Apr 2024 23:49:05 +0200 On Tue, 16 Apr 2024, Alexander Bluhm wrote: > 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. I agree. I have tried with a separate lock that serializes the whole vio_ioctl function but I still get crashes with that: db_command_loop() at db_command_loop+0xea db_trap(4,0) at db_trap+0x129 db_ktrap(4,0,ffff8000239d0f60) at db_ktrap+0x111 kerntrap(ffff8000239d0f60) at kerntrap+0xa7 alltraps_kern_meltdown() at alltraps_kern_meltdown+0x7b rt_ifa_addlocal(ffff8000008d7b00) at rt_ifa_addlocal+0x2a in6_update_ifa(ffff8000000af2a8,ffff8000239d1230,0) at in6_update_ifa+0x581 in6_ifattach_linklocal(ffff8000000af2a8,0) at in6_ifattach_linklocal+0x204 in6_ifattach(ffff8000000af2a8) at in6_ifattach+0x103 ifioctl(fffffd8051ac7dd8,801169ab,ffff8000239d13f0,ffff8000239834a8) at ifioctl+0xc99 sys_ioctl(ffff8000239834a8,ffff8000239d15a0,ffff8000239d1510) at sys_ioctl+0x2af syscall(ffff8000239d15a0) at syscall+0x588 Xsyscall() at Xsyscall+0x128 end of kernel end trace frame: 0x773f17f14c30, count: -13 I suspect that the global ifioctl() function needs some changes in order to allow NIC drivers' ioctl functions to drop the NET_LOCK. Cheers, Stefan > > 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); > > > } > > > > > > >