Date: Sun, 14 Jun 2020 20:47:31 +0000 (UTC) From: Vincenzo Maffione <vmaffione@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r362183 - in head/sys/dev: netmap virtio/network Message-ID: <202006142047.05EKlV2u036760@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: vmaffione Date: Sun Jun 14 20:47:31 2020 New Revision: 362183 URL: https://svnweb.freebsd.org/changeset/base/362183 Log: 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 Modified: head/sys/dev/netmap/if_vtnet_netmap.h head/sys/dev/virtio/network/if_vtnet.c head/sys/dev/virtio/network/if_vtnetvar.h Modified: head/sys/dev/netmap/if_vtnet_netmap.h ============================================================================== --- head/sys/dev/netmap/if_vtnet_netmap.h Sun Jun 14 19:41:24 2020 (r362182) +++ head/sys/dev/netmap/if_vtnet_netmap.h Sun Jun 14 20:47:31 2020 (r362183) @@ -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 Modified: head/sys/dev/virtio/network/if_vtnet.c ============================================================================== --- head/sys/dev/virtio/network/if_vtnet.c Sun Jun 14 19:41:24 2020 (r362182) +++ head/sys/dev/virtio/network/if_vtnet.c Sun Jun 14 20:47:31 2020 (r362183) @@ -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 dat } } } 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 dat 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); } Modified: head/sys/dev/virtio/network/if_vtnetvar.h ============================================================================== --- head/sys/dev/virtio/network/if_vtnetvar.h Sun Jun 14 19:41:24 2020 (r362182) +++ head/sys/dev/virtio/network/if_vtnetvar.h Sun Jun 14 20:47:31 2020 (r362183) @@ -372,4 +372,10 @@ CTASSERT(((VTNET_MAX_TX_SEGS - 1) * MCLBYTES) >= VTNET "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 */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202006142047.05EKlV2u036760>