git: 5b4cfd3f1315 - stable/12 - iflib: Free resources in a consistent order during detach

[Kevin Bowling]

The branch stable/12 has been updated by kbowling (ports committer):

URL: https://cgit.FreeBSD.org/src/commit/?id=5b4cfd3f1315856029256a9847d78d3b1b717fb0

commit 5b4cfd3f1315856029256a9847d78d3b1b717fb0
Author:     Sai Rajesh Tallamraju <stallamr at netapp.com>
AuthorDate: 2021-02-01 16:13:00 +0000
Commit:     Kevin Bowling <kbowling at FreeBSD.org>
CommitDate: 2021-09-28 17:21:52 +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
    
    (cherry picked from commit 38bfc6dee33bedb290e1ea2540f97a86fe3caee0)
---
 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 a571450a463e..7babe51c8c02 100644
--- a/sys/dev/e1000/if_em.c
+++ b/sys/dev/e1000/if_em.c
@@ -1127,10 +1127,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;
 	}
 
@@ -1148,14 +1149,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);
 }
 
@@ -1180,6 +1177,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);
 }
@@ -3012,12 +3011,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 8741eb06ac18..45ac55ec2da8 100644
--- a/sys/dev/ixl/if_ixl.c
+++ b/sys/dev/ixl/if_ixl.c
@@ -1275,7 +1275,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 f8d4c120b3ec..2d53b1ef8229 100644
--- a/sys/net/iflib.c
+++ b/sys/net/iflib.c
@@ -5157,7 +5157,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);
@@ -5192,13 +5192,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);
@@ -5397,11 +5398,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);
@@ -5431,6 +5433,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);
 
@@ -5490,8 +5494,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*/
@@ -5499,9 +5507,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);
@@ -6084,7 +6089,6 @@ iflib_tx_structures_free(if_ctx_t ctx)
 	}
 	free(ctx->ifc_txqs, M_IFLIB);
 	ctx->ifc_txqs = NULL;
-	IFDI_QUEUES_FREE(ctx);
 }
 
 /*********************************************************************