Date: Mon, 1 Feb 2021 16:18:00 GMT From: Mark Johnston <markj@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 38bfc6dee33b - main - iflib: Free resources in a consistent order during detach Message-ID: <202102011618.111GI0bo074673@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=38bfc6dee33bedb290e1ea2540f97a86fe3caee0 commit 38bfc6dee33bedb290e1ea2540f97a86fe3caee0 Author: Sai Rajesh Tallamraju <stallamr@netapp.com> AuthorDate: 2021-02-01 16:13:00 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2021-02-01 16:15:54 +0000 iflib: Free resources in a consistent order during detach Memory and PCI resources are freed with no particular order. This could cause use-after-frees when detaching following a failed attach. For instance, iflib_tx_structures_free() frees ctx->ifc_txqs[] but iflib_tqg_detach() attempts to access this array. Similarly, adapter queues gets freed by IFDI_QUEUES_FREE() but IFDI_DETACH() attempts to access adapter queues to free PCI resources. MFC after: 2 weeks Sponsored by: NetApp, Inc. Differential Revision: https://reviews.freebsd.org/D27634 --- sys/dev/e1000/if_em.c | 19 ++++++------------- sys/dev/ixl/if_ixl.c | 2 +- sys/net/iflib.c | 22 +++++++++++++--------- 3 files changed, 20 insertions(+), 23 deletions(-) diff --git a/sys/dev/e1000/if_em.c b/sys/dev/e1000/if_em.c index fb15a3e1f610..b24280dae412 100644 --- a/sys/dev/e1000/if_em.c +++ b/sys/dev/e1000/if_em.c @@ -1102,10 +1102,11 @@ em_if_attach_post(if_ctx_t ctx) struct adapter *adapter = iflib_get_softc(ctx); struct e1000_hw *hw = &adapter->hw; int error = 0; - + /* Setup OS specific network interface */ error = em_setup_interface(ctx); if (error != 0) { + device_printf(adapter->dev, "Interface setup failed: %d\n", error); goto err_late; } @@ -1123,14 +1124,10 @@ em_if_attach_post(if_ctx_t ctx) INIT_DEBUGOUT("em_if_attach_post: end"); - return (error); + return (0); err_late: - em_release_hw_control(adapter); - em_free_pci_resources(ctx); - em_if_queues_free(ctx); - free(adapter->mta, M_DEVBUF); - + /* upon attach_post() error, iflib calls _if_detach() to free resources. */ return (error); } @@ -1155,6 +1152,8 @@ em_if_detach(if_ctx_t ctx) em_release_manageability(adapter); em_release_hw_control(adapter); em_free_pci_resources(ctx); + free(adapter->mta, M_DEVBUF); + adapter->mta = NULL; return (0); } @@ -2981,12 +2980,6 @@ em_if_queues_free(if_ctx_t ctx) free(adapter->rx_queues, M_DEVBUF); adapter->rx_queues = NULL; } - - em_release_hw_control(adapter); - - if (adapter->mta != NULL) { - free(adapter->mta, M_DEVBUF); - } } /********************************************************************* diff --git a/sys/dev/ixl/if_ixl.c b/sys/dev/ixl/if_ixl.c index 50eb448a1154..097d4b480891 100644 --- a/sys/dev/ixl/if_ixl.c +++ b/sys/dev/ixl/if_ixl.c @@ -1253,7 +1253,7 @@ ixl_if_queues_free(if_ctx_t ctx) struct ixl_pf *pf = iflib_get_softc(ctx); struct ixl_vsi *vsi = &pf->vsi; - if (!vsi->enable_head_writeback) { + if (vsi->tx_queues != NULL && !vsi->enable_head_writeback) { struct ixl_tx_queue *que; int i = 0; diff --git a/sys/net/iflib.c b/sys/net/iflib.c index cfc6972bf987..cce56d0e7335 100644 --- a/sys/net/iflib.c +++ b/sys/net/iflib.c @@ -4900,7 +4900,7 @@ iflib_device_register(device_t dev, void *sc, if_shared_ctx_t sctx, if_ctx_t *ct device_printf(dev, "Cannot use iflib with only 1 MSI-X interrupt!\n"); err = ENODEV; - goto fail_intr_free; + goto fail_queues; } ether_ifattach(ctx->ifc_ifp, ctx->ifc_mac.octet); @@ -4936,13 +4936,14 @@ iflib_device_register(device_t dev, void *sc, if_shared_ctx_t sctx, if_ctx_t *ct fail_detach: ether_ifdetach(ctx->ifc_ifp); -fail_intr_free: - iflib_free_intr_mem(ctx); fail_queues: + iflib_tqg_detach(ctx); iflib_tx_structures_free(ctx); iflib_rx_structures_free(ctx); - iflib_tqg_detach(ctx); IFDI_DETACH(ctx); + IFDI_QUEUES_FREE(ctx); +fail_intr_free: + iflib_free_intr_mem(ctx); fail_unlock: CTX_UNLOCK(ctx); iflib_deregister(ctx); @@ -5139,11 +5140,12 @@ iflib_pseudo_register(device_t dev, if_shared_ctx_t sctx, if_ctx_t *ctxp, fail_detach: ether_ifdetach(ctx->ifc_ifp); fail_queues: + iflib_tqg_detach(ctx); iflib_tx_structures_free(ctx); iflib_rx_structures_free(ctx); - iflib_tqg_detach(ctx); fail_iflib_detach: IFDI_DETACH(ctx); + IFDI_QUEUES_FREE(ctx); fail_unlock: CTX_UNLOCK(ctx); iflib_deregister(ctx); @@ -5173,6 +5175,8 @@ iflib_pseudo_deregister(if_ctx_t ctx) iflib_tqg_detach(ctx); iflib_tx_structures_free(ctx); iflib_rx_structures_free(ctx); + IFDI_DETACH(ctx); + IFDI_QUEUES_FREE(ctx); iflib_deregister(ctx); @@ -5233,8 +5237,12 @@ iflib_device_deregister(if_ctx_t ctx) led_destroy(ctx->ifc_led_dev); iflib_tqg_detach(ctx); + iflib_tx_structures_free(ctx); + iflib_rx_structures_free(ctx); + CTX_LOCK(ctx); IFDI_DETACH(ctx); + IFDI_QUEUES_FREE(ctx); CTX_UNLOCK(ctx); /* ether_ifdetach calls if_qflush - lock must be destroy afterwards*/ @@ -5242,9 +5250,6 @@ iflib_device_deregister(if_ctx_t ctx) bus_generic_detach(dev); - iflib_tx_structures_free(ctx); - iflib_rx_structures_free(ctx); - iflib_deregister(ctx); device_set_softc(ctx->ifc_dev, NULL); @@ -5828,7 +5833,6 @@ iflib_tx_structures_free(if_ctx_t ctx) } free(ctx->ifc_txqs, M_IFLIB); ctx->ifc_txqs = NULL; - IFDI_QUEUES_FREE(ctx); } /*********************************************************************
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202102011618.111GI0bo074673>