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);
> > >  }
> > >
> > >
>