From owner-svn-src-all@freebsd.org Sun Jun 14 20:47:32 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 2B9783444FE; Sun, 14 Jun 2020 20:47:32 +0000 (UTC) (envelope-from vmaffione@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 49lRNh0MlLz4rb1; Sun, 14 Jun 2020 20:47:32 +0000 (UTC) (envelope-from vmaffione@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 038A912663; Sun, 14 Jun 2020 20:47:32 +0000 (UTC) (envelope-from vmaffione@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 05EKlVGN036763; Sun, 14 Jun 2020 20:47:31 GMT (envelope-from vmaffione@FreeBSD.org) Received: (from vmaffione@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 05EKlV2u036760; Sun, 14 Jun 2020 20:47:31 GMT (envelope-from vmaffione@FreeBSD.org) Message-Id: <202006142047.05EKlV2u036760@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: vmaffione set sender to vmaffione@FreeBSD.org using -f From: Vincenzo Maffione Date: Sun, 14 Jun 2020 20:47:31 +0000 (UTC) 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 X-SVN-Group: head X-SVN-Commit-Author: vmaffione X-SVN-Commit-Paths: in head/sys/dev: netmap virtio/network X-SVN-Commit-Revision: 362183 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 14 Jun 2020 20:47:32 -0000 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 */