git: fe00ca2f8871 - main - iflib: Complete registration failure cleanup
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Fri, 14 Aug 2026 21:51:59 UTC
The branch main has been updated by kbowling:
URL: https://cgit.FreeBSD.org/src/commit/?id=fe00ca2f88718c7751b8953cc8ee7fc0dad0bbe1
commit fe00ca2f88718c7751b8953cc8ee7fc0dad0bbe1
Author: Kevin Bowling <kbowling@FreeBSD.org>
AuthorDate: 2026-08-08 04:14:53 +0000
Commit: Kevin Bowling <kbowling@FreeBSD.org>
CommitDate: 2026-08-14 21:49:21 +0000
iflib: Complete registration failure cleanup
Pre-attach sysctls contain pointers into the iflib context. Any later
registration failure that frees the context must first remove that
sysctl tree.
Failures after a successful IFDI_ATTACH_PRE also did not consistently
call IFDI_DETACH or free the private taskqueue. In particular, routing
a taskqueue creation failure through the context cleanup could free the
driver softc while resources allocated by attach_pre remained live.
Track successful interrupt and queue setup and use one common unwind
path. Invoke IFDI_DETACH with IFNET_WLOCK dropped and release only
resources whose setup completed. Leave a failed IFDI_ATTACH_PRE to
unwind its own partial state, as required by the existing driver
contract.
A failed post-attach can follow driver registration of an SR-IOV
schema. Remove that registration before detaching the interface and
driver, matching normal deregistration, so a failed attach cannot leave
a stale /dev/iov node or make the next attach report EBUSY.
A successful attach_pre can now be followed by detach before driver
queue allocation. Make the remaining queue-backed interrupt cleanup
paths tolerate absent queue arrays.
Mark a failed registration as detaching before draining the entire
private taskqueue. Drivers can register configuration tasks there, and
taskqueue_drain_all() does not wait for work queued during its drain.
Make every current non-admin callback reject detaching contexts so late
work cannot touch driver state.
Drain tasks and call ether_ifdetach() with neither the ifnet nor context
lock held. A callback already running may need either lock, while
ether_ifdetach() acquires ifnet_detach_sx. Reacquire IFNET_WLOCK before
the context lock to preserve the established lock order.
The shared automatic core-offset allocator also lacked acquisition
state. Late registration failures leaked its reference, while normal
detach could decrement a reference belonging to another device when a
configured offset or allocation failure meant that this context never
acquired one. Record acquisition explicitly and release only references
held.
MFC after: 2 weeks
Reviewed by: gallatin
Sponsored by: BBOX.io
Differential Revision: https://reviews.freebsd.org/D58721
---
sys/dev/bnxt/bnxt_en/if_bnxt.c | 9 ++-
sys/dev/enetc/if_enetc.c | 10 ++-
sys/dev/igc/if_igc.c | 5 +-
sys/dev/ixgbe/if_sriov.c | 2 +
sys/dev/vmware/vmxnet3/if_vmx.c | 8 ++-
sys/net/iflib.c | 143 ++++++++++++++++++++++++++++++----------
sys/net/iflib.h | 1 +
7 files changed, 135 insertions(+), 43 deletions(-)
diff --git a/sys/dev/bnxt/bnxt_en/if_bnxt.c b/sys/dev/bnxt/bnxt_en/if_bnxt.c
index ec88b30edcde..432dceb6bfb2 100644
--- a/sys/dev/bnxt/bnxt_en/if_bnxt.c
+++ b/sys/dev/bnxt/bnxt_en/if_bnxt.c
@@ -5492,12 +5492,17 @@ bnxt_def_cp_task(void *context, int pending)
/* Handle completions on the default completion ring */
struct cmpl_base *cmpl;
- uint32_t cons = cpr->cons;
- bool v_bit = cpr->v_bit;
+ uint32_t cons;
+ bool v_bit;
bool last_v_bit;
uint32_t last_cons;
uint16_t type;
+ if (iflib_in_detach(ctx))
+ return;
+ cons = cpr->cons;
+ v_bit = cpr->v_bit;
+
for (;;) {
last_cons = cons;
last_v_bit = v_bit;
diff --git a/sys/dev/enetc/if_enetc.c b/sys/dev/enetc/if_enetc.c
index 53002f9d73ce..55fec66266fe 100644
--- a/sys/dev/enetc/if_enetc.c
+++ b/sys/dev/enetc/if_enetc.c
@@ -456,14 +456,18 @@ enetc_detach(if_ctx_t ctx)
sc = iflib_get_softc(ctx);
- for (i = 0; i < sc->rx_num_queues; i++)
- iflib_irq_free(ctx, &sc->rx_queues[i].irq);
+ if (sc->rx_queues != NULL) {
+ for (i = 0; i < sc->rx_num_queues; i++)
+ iflib_irq_free(ctx, &sc->rx_queues[i].irq);
+ }
bus_generic_detach(sc->dev);
- if (sc->regs != NULL)
+ if (sc->regs != NULL) {
error = bus_release_resource(sc->dev, SYS_RES_MEMORY,
rman_get_rid(sc->regs), sc->regs);
+ sc->regs = NULL;
+ }
if (sc->ctrl_queue.dma.idi_size != 0)
iflib_dma_free(&sc->ctrl_queue.dma);
diff --git a/sys/dev/igc/if_igc.c b/sys/dev/igc/if_igc.c
index 11fe32f62df3..7026a4e50bb9 100644
--- a/sys/dev/igc/if_igc.c
+++ b/sys/dev/igc/if_igc.c
@@ -1988,8 +1988,9 @@ igc_free_pci_resources(if_ctx_t ctx)
if (sc->intr_type == IFLIB_INTR_MSIX)
iflib_irq_free(ctx, &sc->irq);
- for (int i = 0; i < sc->rx_num_queues; i++, que++) {
- iflib_irq_free(ctx, &que->que_irq);
+ if (que != NULL) {
+ for (int i = 0; i < sc->rx_num_queues; i++, que++)
+ iflib_irq_free(ctx, &que->que_irq);
}
if (sc->memory != NULL) {
diff --git a/sys/dev/ixgbe/if_sriov.c b/sys/dev/ixgbe/if_sriov.c
index 9fc3437a2cf3..09954a519664 100644
--- a/sys/dev/ixgbe/if_sriov.c
+++ b/sys/dev/ixgbe/if_sriov.c
@@ -775,6 +775,8 @@ ixgbe_iov_recovery_task(void *context, int pending __unused)
int i, iov_pos, n, num_vfs = 0, recovery_vf;
ctx = context;
+ if (iflib_in_detach(ctx))
+ return;
sc = iflib_get_softc(ctx);
ctx_lock = iflib_ctx_lock_get(ctx);
diff --git a/sys/dev/vmware/vmxnet3/if_vmx.c b/sys/dev/vmware/vmxnet3/if_vmx.c
index c3706ed24ff5..744205ef0e7d 100644
--- a/sys/dev/vmware/vmxnet3/if_vmx.c
+++ b/sys/dev/vmware/vmxnet3/if_vmx.c
@@ -527,9 +527,11 @@ vmxnet3_free_irqs(struct vmxnet3_softc *sc)
scctx = sc->vmx_scctx;
- for (i = 0; i < scctx->isc_nrxqsets; i++) {
- rxq = &sc->vmx_rxq[i];
- iflib_irq_free(sc->vmx_ctx, &rxq->vxrxq_irq);
+ if (sc->vmx_rxq != NULL) {
+ for (i = 0; i < scctx->isc_nrxqsets; i++) {
+ rxq = &sc->vmx_rxq[i];
+ iflib_irq_free(sc->vmx_ctx, &rxq->vxrxq_irq);
+ }
}
iflib_irq_free(sc->vmx_ctx, &sc->vmx_event_intr_irq);
diff --git a/sys/net/iflib.c b/sys/net/iflib.c
index d2ac8d884a60..7b5de820fd71 100644
--- a/sys/net/iflib.c
+++ b/sys/net/iflib.c
@@ -205,6 +205,7 @@ struct iflib_ctx {
uint8_t ifc_sysctl_use_logical_cores;
uint16_t ifc_sysctl_extra_msix_vectors;
bool ifc_cpus_are_physical_cores;
+ bool ifc_core_offset_ref;
bool ifc_sysctl_simple_tx;
bool ifc_sysctl_tx_defer_mfree;
uint16_t ifc_sysctl_tx_reclaim_thresh;
@@ -4259,6 +4260,8 @@ _task_fn_iov(void *context, int pending)
{
if_ctx_t ctx = context;
+ if (iflib_in_detach(ctx))
+ return;
if (!(if_getdrvflags(ctx->ifc_ifp) & IFF_DRV_RUNNING) &&
!(ctx->ifc_sctx->isc_flags & IFLIB_ADMIN_ALWAYS_RUN))
return;
@@ -5071,6 +5074,7 @@ get_ctx_core_offset(if_ctx_t ctx)
unsigned int last_valid;
unsigned int i;
+ MPASS(!ctx->ifc_core_offset_ref);
first_valid = CPU_FFS(&ctx->ifc_cpus) - 1;
last_valid = CPU_FLS(&ctx->ifc_cpus) - 1;
@@ -5144,6 +5148,7 @@ get_ctx_core_offset(if_ctx_t ctx)
cores_consumed);
MPASS(op->refcount < UINT_MAX);
op->refcount++;
+ ctx->ifc_core_offset_ref = true;
break;
}
}
@@ -5160,6 +5165,7 @@ get_ctx_core_offset(if_ctx_t ctx)
op->refcount = 1;
CPU_COPY(&ctx->ifc_cpus, &op->set);
SLIST_INSERT_HEAD(&cpu_offsets, op, entries);
+ ctx->ifc_core_offset_ref = true;
}
}
mtx_unlock(&cpu_offset_mtx);
@@ -5172,6 +5178,9 @@ unref_ctx_core_offset(if_ctx_t ctx)
{
struct cpu_offset *op, *top;
+ if (!ctx->ifc_core_offset_ref)
+ return;
+
mtx_lock(&cpu_offset_mtx);
SLIST_FOREACH_SAFE(op, &cpu_offsets, entries, top) {
if (CPU_CMP(&ctx->ifc_cpus, &op->set) == 0) {
@@ -5181,10 +5190,12 @@ unref_ctx_core_offset(if_ctx_t ctx)
SLIST_REMOVE(&cpu_offsets, op, cpu_offset, entries);
free(op, M_IFLIB);
}
+ ctx->ifc_core_offset_ref = false;
break;
}
}
mtx_unlock(&cpu_offset_mtx);
+ MPASS(!ctx->ifc_core_offset_ref);
}
int
@@ -5195,10 +5206,17 @@ iflib_device_register(device_t dev, void *sc, if_shared_ctx_t sctx, if_ctx_t *ct
if_softc_ctx_t scctx;
kobjop_desc_t kobj_desc;
kobj_method_t *kobj_method;
+ bool attach_pre_succeeded, intr_allocated, queues_allocated;
int err, msix, rid;
+#ifdef PCI_IOV
+ int iov_error;
+#endif
int num_txd, num_rxd;
char namebuf[TASKQUEUE_NAMELEN];
+ attach_pre_succeeded = false;
+ intr_allocated = false;
+ queues_allocated = false;
ctx = malloc(sizeof(*ctx), M_IFLIB, M_WAITOK | M_ZERO);
if (sc == NULL) {
@@ -5229,8 +5247,9 @@ iflib_device_register(device_t dev, void *sc, if_shared_ctx_t sctx, if_ctx_t *ct
CTX_LOCK(ctx);
if ((err = IFDI_ATTACH_PRE(ctx)) != 0) {
device_printf(dev, "IFDI_ATTACH_PRE failed %d\n", err);
- goto fail_unlock;
+ goto fail_cleanup;
}
+ attach_pre_succeeded = true;
_iflib_pre_assert(scctx);
ctx->ifc_txrx = *scctx->isc_txrx;
@@ -5298,7 +5317,8 @@ iflib_device_register(device_t dev, void *sc, if_shared_ctx_t sctx, if_ctx_t *ct
taskqueue_thread_enqueue, &ctx->ifc_tq);
if (ctx->ifc_tq == NULL) {
device_printf(dev, "Unable to create admin taskqueue\n");
- return (ENOMEM);
+ err = ENOMEM;
+ goto fail_cleanup;
}
err = taskqueue_start_threads(&ctx->ifc_tq, 1, PI_NET, "%s", namebuf);
@@ -5307,7 +5327,8 @@ iflib_device_register(device_t dev, void *sc, if_shared_ctx_t sctx, if_ctx_t *ct
"Unable to start admin taskqueue threads error: %d\n",
err);
taskqueue_free(ctx->ifc_tq);
- return (err);
+ ctx->ifc_tq = NULL;
+ goto fail_cleanup;
}
TASK_INIT(&ctx->ifc_admin_task, 0, _task_fn_admin, ctx);
@@ -5341,14 +5362,16 @@ iflib_device_register(device_t dev, void *sc, if_shared_ctx_t sctx, if_ctx_t *ct
scctx->isc_intr = IFLIB_INTR_LEGACY;
msix = 0;
}
+ intr_allocated = true;
/* Get memory for the station queues */
if ((err = iflib_queues_alloc(ctx))) {
device_printf(dev, "Unable to allocate queue memory\n");
- goto fail_intr_free;
+ goto fail_cleanup;
}
+ queues_allocated = true;
if ((err = iflib_qset_structures_setup(ctx)))
- goto fail_queues;
+ goto fail_cleanup;
/*
* Now that we know how many queues there are, get the core offset.
@@ -5367,7 +5390,7 @@ iflib_device_register(device_t dev, void *sc, if_shared_ctx_t sctx, if_ctx_t *ct
device_printf(dev,
"MSI-X requires ifdi_rx_queue_intr_enable method");
err = EOPNOTSUPP;
- goto fail_queues;
+ goto fail_cleanup;
}
kobj_desc = &ifdi_tx_queue_intr_enable_desc;
kobj_method = kobj_lookup_method(((kobj_t)ctx)->ops->cls, NULL,
@@ -5376,7 +5399,7 @@ iflib_device_register(device_t dev, void *sc, if_shared_ctx_t sctx, if_ctx_t *ct
device_printf(dev,
"MSI-X requires ifdi_tx_queue_intr_enable method");
err = EOPNOTSUPP;
- goto fail_queues;
+ goto fail_cleanup;
}
/*
@@ -5388,7 +5411,7 @@ iflib_device_register(device_t dev, void *sc, if_shared_ctx_t sctx, if_ctx_t *ct
if (err != 0) {
device_printf(dev, "IFDI_MSIX_INTR_ASSIGN failed %d\n",
err);
- goto fail_queues;
+ goto fail_cleanup;
}
} else if (scctx->isc_intr != IFLIB_INTR_MSIX) {
rid = 0;
@@ -5398,13 +5421,13 @@ iflib_device_register(device_t dev, void *sc, if_shared_ctx_t sctx, if_ctx_t *ct
}
if ((err = iflib_legacy_setup(ctx, ctx->isc_legacy_intr, ctx->ifc_softc, &rid, "irq0")) != 0) {
device_printf(dev, "iflib_legacy_setup failed %d\n", err);
- goto fail_queues;
+ goto fail_cleanup;
}
} else {
device_printf(dev,
"Cannot use iflib with only 1 MSI-X interrupt!\n");
err = ENODEV;
- goto fail_queues;
+ goto fail_cleanup;
}
/*
@@ -5452,42 +5475,96 @@ iflib_device_register(device_t dev, void *sc, if_shared_ctx_t sctx, if_ctx_t *ct
return (0);
fail_detach:
+ STATE_LOCK(ctx);
+ ctx->ifc_flags |= IFC_IN_DETACH;
+ STATE_UNLOCK(ctx);
+ /* Tasks may need either lock; ether_ifdetach() takes ifnet_detach_sx. */
CTX_UNLOCK(ctx);
- taskqueue_drain(ctx->ifc_tq, &ctx->ifc_admin_task);
+ IFNET_WUNLOCK();
+ taskqueue_drain_all(ctx->ifc_tq);
+#ifdef PCI_IOV
+ /*
+ * IFDI_ATTACH_POST may have registered an SR-IOV schema. Match the
+ * normal deregistration order so a failed attach cannot leave a stale
+ * /dev/iov node behind. device_attach() holds Giant throughout this
+ * path, so an IOV configuration cannot race the detach.
+ */
+ if (!CTX_IS_VF(ctx)) {
+ iov_error = pci_iov_detach(dev);
+ if (iov_error != 0)
+ device_printf(dev, "Could not detach SR-IOV after "
+ "attach failure: %d\n", iov_error);
+ }
+#endif
ether_ifdetach(ctx->ifc_ifp);
+ IFNET_WLOCK();
CTX_LOCK(ctx);
-fail_queues:
- sysctl_ctx_free(&ctx->ifc_sysctl_ctx);
- ctx->ifc_sysctl_node = NULL;
+ goto fail_cleanup_detaching;
+
+fail_cleanup:
+ STATE_LOCK(ctx);
+ ctx->ifc_flags |= IFC_IN_DETACH;
+ STATE_UNLOCK(ctx);
+
+fail_cleanup_detaching:
/*
- * Drain without holding CTX_LOCK so _task_fn_admin can run to
- * completion if it needs the context lock. On fail_detach we already
- * drained above; a second drain is a no-op when the queue is empty.
+ * The pre-attach sysctls contain pointers into ctx. Remove them on
+ * every registration failure before iflib_deregister() frees ctx.
*/
- CTX_UNLOCK(ctx);
- taskqueue_drain(ctx->ifc_tq, &ctx->ifc_admin_task);
- CTX_LOCK(ctx);
- iflib_tqg_detach(ctx);
- iflib_tx_structures_free(ctx);
- iflib_rx_structures_free(ctx);
+ if (ctx->ifc_sysctl_node != NULL) {
+ sysctl_ctx_free(&ctx->ifc_sysctl_ctx);
+ ctx->ifc_sysctl_node = NULL;
+ }
+
+ if (ctx->ifc_tq != NULL) {
+ /*
+ * Drain without holding the ifnet or context locks so configuration
+ * tasks can run to completion. On fail_detach a second drain also
+ * catches tasks queued during the first drain.
+ */
+ CTX_UNLOCK(ctx);
+ IFNET_WUNLOCK();
+ taskqueue_drain_all(ctx->ifc_tq);
+ IFNET_WLOCK();
+ CTX_LOCK(ctx);
+ }
+
+ if (queues_allocated) {
+ iflib_tqg_detach(ctx);
+ iflib_tx_structures_free(ctx);
+ iflib_rx_structures_free(ctx);
+ }
+
/*
- * Match iflib_device_deregister: IFDI_DETACH before taskqueue_free.
- * Avoid IFNET_WLOCK across driver detach (LinuxKPI workqueue drain).
+ * A successful IFDI_ATTACH_PRE must be matched by IFDI_DETACH, even
+ * when registration fails before queue allocation. Match
+ * iflib_device_deregister by detaching before taskqueue_free, and avoid
+ * holding IFNET_WLOCK across driver detach (LinuxKPI workqueue drain).
*/
- IFNET_WUNLOCK();
- IFDI_DETACH(ctx);
- IFDI_QUEUES_FREE(ctx);
- IFNET_WLOCK();
- taskqueue_free(ctx->ifc_tq);
-fail_intr_free:
- iflib_free_intr_mem(ctx);
-fail_unlock:
+ if (attach_pre_succeeded) {
+ IFNET_WUNLOCK();
+ IFDI_DETACH(ctx);
+ if (queues_allocated)
+ IFDI_QUEUES_FREE(ctx);
+ /* Reacquire the global lock before the context lock. */
+ CTX_UNLOCK(ctx);
+ IFNET_WLOCK();
+ CTX_LOCK(ctx);
+ }
+ if (ctx->ifc_tq != NULL) {
+ taskqueue_free(ctx->ifc_tq);
+ ctx->ifc_tq = NULL;
+ }
+ if (intr_allocated)
+ iflib_free_intr_mem(ctx);
+
CTX_UNLOCK(ctx);
IFNET_WUNLOCK();
iflib_deregister(ctx);
device_set_softc(ctx->ifc_dev, NULL);
if (ctx->ifc_flags & IFC_SC_ALLOCATED)
free(ctx->ifc_softc, M_IFLIB);
+ unref_ctx_core_offset(ctx);
free(ctx, M_IFLIB);
return (err);
}
diff --git a/sys/net/iflib.h b/sys/net/iflib.h
index 40080b49b0ab..0f35f1080bad 100644
--- a/sys/net/iflib.h
+++ b/sys/net/iflib.h
@@ -481,6 +481,7 @@ void iflib_irq_free(if_ctx_t ctx, if_irq_t irq);
void iflib_io_tqg_attach(struct grouptask *gt, void *uniq, int cpu,
const char *name);
+/* Configuration task callbacks must return when iflib_in_detach() is true. */
void iflib_config_task_init(if_ctx_t ctx, struct task *config_task,
task_fn_t *fn);
void iflib_config_task_enqueue(if_ctx_t ctx, struct task *config_task);