From c831112179614a167b829a5af95fdff7c1178a11 Mon Sep 17 00:00:00 2001 From: Peter Grehan Date: Sat, 12 Oct 2013 00:32:34 +0000 Subject: [PATCH] Fix a lock-order reversal in the net driver by dropping the lock and holding a reference prior to calling further into the hyperv stack. Added missing FreeBSD idents. Submitted by: Microsoft hyperv dev team Approved by: re@ (gjb) --- sys/dev/hyperv/netvsc/hv_net_vsc.h | 4 + sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c | 101 ++++++++++++++---- 2 files changed, 86 insertions(+), 19 deletions(-) diff --git a/sys/dev/hyperv/netvsc/hv_net_vsc.h b/sys/dev/hyperv/netvsc/hv_net_vsc.h index f7e7d00a903f..b9f3a1ee9cb5 100644 --- a/sys/dev/hyperv/netvsc/hv_net_vsc.h +++ b/sys/dev/hyperv/netvsc/hv_net_vsc.h @@ -24,6 +24,8 @@ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * $FreeBSD$ */ /* @@ -970,6 +972,8 @@ typedef struct hn_softc { int hn_if_flags; struct mtx hn_lock; int hn_initdone; + /* See hv_netvsc_drv_freebsd.c for rules on how to use */ + int temp_unusable; struct hv_device *hn_dev_obj; netvsc_dev *net_dev; } hn_softc_t; diff --git a/sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c b/sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c index 74700061e545..f6ace58d4ff8 100644 --- a/sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c +++ b/sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c @@ -52,6 +52,9 @@ * SUCH DAMAGE. */ +#include +__FBSDID("$FreeBSD$"); + #include #include #include @@ -701,6 +704,17 @@ netvsc_recv(struct hv_device *device_ctx, netvsc_packet *packet) return (0); } +/* + * Rules for using sc->temp_unusable: + * 1. sc->temp_unusable can only be read or written while holding NV_LOCK() + * 2. code reading sc->temp_unusable under NV_LOCK(), and finding + * sc->temp_unusable set, must release NV_LOCK() and exit + * 3. to retain exclusive control of the interface, + * sc->temp_unusable must be set by code before releasing NV_LOCK() + * 4. only code setting sc->temp_unusable can clear sc->temp_unusable + * 5. code setting sc->temp_unusable must eventually clear sc->temp_unusable + */ + /* * Standard ioctl entry point. Called when the user wants to configure * the interface. @@ -713,7 +727,8 @@ hn_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data) netvsc_device_info device_info; struct hv_device *hn_dev; int mask, error = 0; - + int retry_cnt = 500; + switch(cmd) { case SIOCSIFADDR: @@ -723,38 +738,80 @@ hn_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data) case SIOCSIFMTU: hn_dev = vmbus_get_devctx(sc->hn_dev); - NV_LOCK(sc); + /* Check MTU value change */ + if (ifp->if_mtu == ifr->ifr_mtu) + break; if (ifr->ifr_mtu > NETVSC_MAX_CONFIGURABLE_MTU) { error = EINVAL; - NV_UNLOCK(sc); break; } + /* Obtain and record requested MTU */ ifp->if_mtu = ifr->ifr_mtu; + + do { + NV_LOCK(sc); + if (!sc->temp_unusable) { + sc->temp_unusable = TRUE; + retry_cnt = -1; + } + NV_UNLOCK(sc); + if (retry_cnt > 0) { + retry_cnt--; + DELAY(5 * 1000); + } + } while (retry_cnt > 0); - /* - * We must remove and add back the device to cause the new + if (retry_cnt == 0) { + error = EINVAL; + break; + } + + /* We must remove and add back the device to cause the new * MTU to take effect. This includes tearing down, but not * deleting the channel, then bringing it back up. */ error = hv_rf_on_device_remove(hn_dev, HV_RF_NV_RETAIN_CHANNEL); if (error) { + NV_LOCK(sc); + sc->temp_unusable = FALSE; NV_UNLOCK(sc); break; } error = hv_rf_on_device_add(hn_dev, &device_info); if (error) { + NV_LOCK(sc); + sc->temp_unusable = FALSE; NV_UNLOCK(sc); break; } hn_ifinit_locked(sc); + NV_LOCK(sc); + sc->temp_unusable = FALSE; NV_UNLOCK(sc); break; case SIOCSIFFLAGS: - NV_LOCK(sc); + do { + NV_LOCK(sc); + if (!sc->temp_unusable) { + sc->temp_unusable = TRUE; + retry_cnt = -1; + } + NV_UNLOCK(sc); + if (retry_cnt > 0) { + retry_cnt--; + DELAY(5 * 1000); + } + } while (retry_cnt > 0); + + if (retry_cnt == 0) { + error = EINVAL; + break; + } + if (ifp->if_flags & IFF_UP) { /* * If only the state of the PROMISC flag changed, @@ -766,21 +823,14 @@ hn_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data) */ #ifdef notyet /* Fixme: Promiscuous mode? */ - /* No promiscuous mode with Xen */ if (ifp->if_drv_flags & IFF_DRV_RUNNING && ifp->if_flags & IFF_PROMISC && !(sc->hn_if_flags & IFF_PROMISC)) { /* do something here for Hyper-V */ - ; -/* XN_SETBIT(sc, XN_RX_MODE, */ -/* XN_RXMODE_RX_PROMISC); */ } else if (ifp->if_drv_flags & IFF_DRV_RUNNING && - !(ifp->if_flags & IFF_PROMISC) && - sc->hn_if_flags & IFF_PROMISC) { + !(ifp->if_flags & IFF_PROMISC) && + sc->hn_if_flags & IFF_PROMISC) { /* do something here for Hyper-V */ - ; -/* XN_CLRBIT(sc, XN_RX_MODE, */ -/* XN_RXMODE_RX_PROMISC); */ } else #endif hn_ifinit_locked(sc); @@ -789,8 +839,10 @@ hn_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data) hn_stop(sc); } } - sc->hn_if_flags = ifp->if_flags; + NV_LOCK(sc); + sc->temp_unusable = FALSE; NV_UNLOCK(sc); + sc->hn_if_flags = ifp->if_flags; error = 0; break; case SIOCSIFCAP: @@ -838,7 +890,6 @@ hn_stop(hn_softc_t *sc) int ret; struct hv_device *device_ctx = vmbus_get_devctx(sc->hn_dev); - NV_LOCK_ASSERT(sc); ifp = sc->hn_ifp; printf(" Closing Device ...\n"); @@ -859,6 +910,10 @@ hn_start(struct ifnet *ifp) sc = ifp->if_softc; NV_LOCK(sc); + if (sc->temp_unusable) { + NV_UNLOCK(sc); + return; + } hn_start_locked(ifp); NV_UNLOCK(sc); } @@ -873,8 +928,6 @@ hn_ifinit_locked(hn_softc_t *sc) struct hv_device *device_ctx = vmbus_get_devctx(sc->hn_dev); int ret; - NV_LOCK_ASSERT(sc); - ifp = sc->hn_ifp; if (ifp->if_drv_flags & IFF_DRV_RUNNING) { @@ -902,7 +955,17 @@ hn_ifinit(void *xsc) hn_softc_t *sc = xsc; NV_LOCK(sc); + if (sc->temp_unusable) { + NV_UNLOCK(sc); + return; + } + sc->temp_unusable = TRUE; + NV_UNLOCK(sc); + hn_ifinit_locked(sc); + + NV_LOCK(sc); + sc->temp_unusable = FALSE; NV_UNLOCK(sc); }