The branch stable/15 has been updated by kbowling: URL: https://cgit.FreeBSD.org/src/commit/?id=4a9daa081a6dbb2d27edf8fd0d34862f813979d0
commit 4a9daa081a6dbb2d27edf8fd0d34862f813979d0 Author: Kevin Bowling <[email protected]> AuthorDate: 2026-08-08 04:14:53 +0000 Commit: Kevin Bowling <[email protected]> CommitDate: 2026-08-28 02:11:02 +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. Reviewed by: gallatin Sponsored by: BBOX.io Differential Revision: https://reviews.freebsd.org/D58721 (cherry picked from commit fe00ca2f88718c7751b8953cc8ee7fc0dad0bbe1) --- 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 556d63e4015f..30df72ce6a0f 100644 --- a/sys/dev/bnxt/bnxt_en/if_bnxt.c +++ b/sys/dev/bnxt/bnxt_en/if_bnxt.c @@ -5214,12 +5214,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 9a854aec2bff..dcc6d4585991 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 75fbe5a67a2d..eae83a673786 100644 --- a/sys/dev/igc/if_igc.c +++ b/sys/dev/igc/if_igc.c @@ -2003,8 +2003,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 1a314ca6660e..c5e21b164b4b 100644 --- a/sys/dev/vmware/vmxnet3/if_vmx.c +++ b/sys/dev/vmware/vmxnet3/if_vmx.c @@ -529,9 +529,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 4726ca5bfaa9..febfa418fb46 100644 --- a/sys/net/iflib.c +++ b/sys/net/iflib.c @@ -203,6 +203,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; qidx_t ifc_sysctl_ntxds[8]; @@ -4124,6 +4125,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; @@ -4918,6 +4921,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; @@ -4991,6 +4995,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; } } @@ -5007,6 +5012,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); @@ -5019,6 +5025,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) { @@ -5028,10 +5037,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 @@ -5042,10 +5053,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) { @@ -5076,8 +5094,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; @@ -5145,7 +5164,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); @@ -5154,7 +5174,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); @@ -5188,14 +5209,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. @@ -5214,7 +5237,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, @@ -5223,7 +5246,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; } /* @@ -5235,7 +5258,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; @@ -5245,13 +5268,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; } /* @@ -5299,42 +5322,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 a8ab179aea21..a2bc9c1f8835 100644 --- a/sys/net/iflib.h +++ b/sys/net/iflib.h @@ -478,6 +478,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);
