From 16f224b5f827af1dd162f0c1d8d2f6a8150b6ce4 Mon Sep 17 00:00:00 2001 From: Vincenzo Maffione Date: Sun, 14 Jun 2020 20:47:31 +0000 Subject: [PATCH] netmap: vtnet: fix races in vtnet_netmap_reg() The nm_register callback needs to call nm_set_native_flags() or nm_clear_native_flags() once the device has been stopped. However, in the current implementation this is not true, as the device is stopped by vtnet_init_locked(). This causes race conditions where the driver crashes as soon as it dequeues netmap buffers assuming they are mbufs (or the other way around). To fix the issue, we extend vtnet_init_locked() with a second argument that, if not zero, will set/clear the netmap flags. This results in a huge simplification of the nm_register callback itself. Also, use netmap_reset() to check if a ring is going to be re-initialized in netmap mode. MFC after: 1 week --- sys/dev/netmap/if_vtnet_netmap.h | 58 ++++++---------------------- sys/dev/virtio/network/if_vtnet.c | 31 +++++++++++---- sys/dev/virtio/network/if_vtnetvar.h | 6 +++ 3 files changed, 40 insertions(+), 55 deletions(-) diff --git a/sys/dev/netmap/if_vtnet_netmap.h b/sys/dev/netmap/if_vtnet_netmap.h index fc667738a1f5..0f686ed60788 100644 --- a/sys/dev/netmap/if_vtnet_netmap.h +++ b/sys/dev/netmap/if_vtnet_netmap.h @@ -39,52 +39,18 @@ vtnet_netmap_reg(struct netmap_adapter *na, int state) { struct ifnet *ifp = na->ifp; struct vtnet_softc *sc = ifp->if_softc; - int success; - int i; - - /* Drain the taskqueues to make sure that there are no worker threads - * accessing the virtqueues. */ - vtnet_drain_taskqueues(sc); + /* + * Trigger a device reinit, asking vtnet_init_locked() to + * also enter or exit netmap mode. + */ VTNET_CORE_LOCK(sc); - - /* We need nm_netmap_on() to return true when called by - * vtnet_init_locked() below. */ - if (state) - nm_set_native_flags(na); - - /* We need to trigger a device reset in order to unexpose guest buffers - * published to the host. */ - ifp->if_drv_flags &= ~(IFF_DRV_RUNNING | IFF_DRV_OACTIVE); - /* Get pending used buffers. The way they are freed depends on whether - * they are netmap buffer or they are mbufs. We can tell apart the two - * cases by looking at kring->nr_mode, before this is possibly updated - * in the loop below. */ - for (i = 0; i < sc->vtnet_act_vq_pairs; i++) { - struct vtnet_txq *txq = &sc->vtnet_txqs[i]; - struct vtnet_rxq *rxq = &sc->vtnet_rxqs[i]; - - VTNET_TXQ_LOCK(txq); - vtnet_txq_free_mbufs(txq); - VTNET_TXQ_UNLOCK(txq); - - VTNET_RXQ_LOCK(rxq); - vtnet_rxq_free_mbufs(rxq); - VTNET_RXQ_UNLOCK(rxq); - } - vtnet_init_locked(sc); - success = (ifp->if_drv_flags & IFF_DRV_RUNNING) ? 0 : ENXIO; - - if (state) { - netmap_krings_mode_commit(na, state); - } else { - nm_clear_native_flags(na); - netmap_krings_mode_commit(na, state); - } - + ifp->if_drv_flags &= ~IFF_DRV_RUNNING; + vtnet_init_locked(sc, state ? VTNET_INIT_NETMAP_ENTER + : VTNET_INIT_NETMAP_EXIT); VTNET_CORE_UNLOCK(sc); - return success; + return 0; } @@ -245,15 +211,13 @@ vtnet_netmap_rxq_populate(struct vtnet_rxq *rxq) { struct netmap_adapter *na = NA(rxq->vtnrx_sc->vtnet_ifp); struct netmap_kring *kring; + struct netmap_slot *slot; int error; - if (!nm_native_on(na) || rxq->vtnrx_id >= na->num_rx_rings) + slot = netmap_reset(na, NR_RX, rxq->vtnrx_id, 0); + if (slot == NULL) return -1; - kring = na->rx_rings[rxq->vtnrx_id]; - if (!(nm_kring_pending_on(kring) || - kring->nr_pending_mode == NKR_NETMAP_ON)) - return -1; /* Expose all the RX netmap buffers we can. In case of no indirect * buffers, the number of netmap slots in the RX ring matches the diff --git a/sys/dev/virtio/network/if_vtnet.c b/sys/dev/virtio/network/if_vtnet.c index 68fcfb4cb9db..3ef8d90d5fa9 100644 --- a/sys/dev/virtio/network/if_vtnet.c +++ b/sys/dev/virtio/network/if_vtnet.c @@ -181,7 +181,7 @@ static int vtnet_init_tx_queues(struct vtnet_softc *); static int vtnet_init_rxtx_queues(struct vtnet_softc *); static void vtnet_set_active_vq_pairs(struct vtnet_softc *); static int vtnet_reinit(struct vtnet_softc *); -static void vtnet_init_locked(struct vtnet_softc *); +static void vtnet_init_locked(struct vtnet_softc *, int); static void vtnet_init(void *); static void vtnet_free_ctrl_vq(struct vtnet_softc *); @@ -523,7 +523,7 @@ vtnet_resume(device_t dev) VTNET_CORE_LOCK(sc); if (ifp->if_flags & IFF_UP) - vtnet_init_locked(sc); + vtnet_init_locked(sc, 0); sc->vtnet_flags &= ~VTNET_FLAG_SUSPENDED; VTNET_CORE_UNLOCK(sc); @@ -1087,7 +1087,7 @@ vtnet_change_mtu(struct vtnet_softc *sc, int new_mtu) if (ifp->if_drv_flags & IFF_DRV_RUNNING) { ifp->if_drv_flags &= ~IFF_DRV_RUNNING; - vtnet_init_locked(sc); + vtnet_init_locked(sc, 0); } return (0); @@ -1131,7 +1131,7 @@ vtnet_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data) } } } else - vtnet_init_locked(sc); + vtnet_init_locked(sc, 0); if (error == 0) sc->vtnet_if_flags = ifp->if_flags; @@ -1189,7 +1189,7 @@ vtnet_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data) if (reinit && (ifp->if_drv_flags & IFF_DRV_RUNNING)) { ifp->if_drv_flags &= ~IFF_DRV_RUNNING; - vtnet_init_locked(sc); + vtnet_init_locked(sc, 0); } VTNET_CORE_UNLOCK(sc); @@ -2783,7 +2783,7 @@ vtnet_tick(void *xsc) if (timedout != 0) { ifp->if_drv_flags &= ~IFF_DRV_RUNNING; - vtnet_init_locked(sc); + vtnet_init_locked(sc, 0); } else callout_schedule(&sc->vtnet_tick_ch, hz); } @@ -3068,6 +3068,9 @@ vtnet_init_tx_queues(struct vtnet_softc *sc) for (i = 0; i < sc->vtnet_act_vq_pairs; i++) { txq = &sc->vtnet_txqs[i]; txq->vtntx_watchdog = 0; +#ifdef DEV_NETMAP + netmap_reset(NA(sc->vtnet_ifp), NR_TX, i, 0); +#endif /* DEV_NETMAP */ } return (0); @@ -3151,7 +3154,7 @@ vtnet_reinit(struct vtnet_softc *sc) } static void -vtnet_init_locked(struct vtnet_softc *sc) +vtnet_init_locked(struct vtnet_softc *sc, int init_mode) { device_t dev; struct ifnet *ifp; @@ -3166,6 +3169,18 @@ vtnet_init_locked(struct vtnet_softc *sc) vtnet_stop(sc); +#ifdef DEV_NETMAP + /* Once stopped we can update the netmap flags, if necessary. */ + switch (init_mode) { + case VTNET_INIT_NETMAP_ENTER: + nm_set_native_flags(NA(ifp)); + break; + case VTNET_INIT_NETMAP_EXIT: + nm_clear_native_flags(NA(ifp)); + break; + } +#endif /* DEV_NETMAP */ + /* Reinitialize with the host. */ if (vtnet_virtio_reinit(sc) != 0) goto fail; @@ -3192,7 +3207,7 @@ vtnet_init(void *xsc) sc = xsc; VTNET_CORE_LOCK(sc); - vtnet_init_locked(sc); + vtnet_init_locked(sc, 0); VTNET_CORE_UNLOCK(sc); } diff --git a/sys/dev/virtio/network/if_vtnetvar.h b/sys/dev/virtio/network/if_vtnetvar.h index dae955b1228c..01775cc186f2 100644 --- a/sys/dev/virtio/network/if_vtnetvar.h +++ b/sys/dev/virtio/network/if_vtnetvar.h @@ -372,4 +372,10 @@ CTASSERT(((VTNET_MAX_TX_SEGS - 1) * MCLBYTES) >= VTNET_MAX_MTU); "VTNET Core Lock", MTX_DEF); \ } while (0) +/* + * Values for the init_mode argument of vtnet_init_locked(). + */ +#define VTNET_INIT_NETMAP_ENTER 1 +#define VTNET_INIT_NETMAP_EXIT 2 + #endif /* _IF_VTNETVAR_H */