Page MenuHomeFreeBSD

iflib: clean up correctly when device attach fails
Needs ReviewPublic

Authored by nick_spun.io on Tue, Jul 21, 3:03 PM.
Referenced Files
F164117890: D58376.diff
Tue, Jul 28, 9:46 PM
F164098783: D58376.diff
Tue, Jul 28, 5:07 PM
Unknown Object (File)
Tue, Jul 28, 10:13 AM
Unknown Object (File)
Tue, Jul 28, 6:21 AM
Unknown Object (File)
Tue, Jul 28, 12:50 AM
Unknown Object (File)
Mon, Jul 27, 9:47 PM
Unknown Object (File)
Mon, Jul 27, 8:25 PM
Unknown Object (File)
Mon, Jul 27, 9:07 AM

Details

Reviewers
shurd
gallatin
Group Reviewers
iflib
Summary

Two leaks on the iflib_device_register() failure paths.

The device sysctls are the first thing registered, so free them last:
move sysctl_ctx_free() to the fail_unlock path that every failure
reaches. On that path only the pre-attach sysctls exist and none
reference queue memory, so freeing them after the queue teardown is
safe.

Every failure after a successful IFDI_ATTACH_PRE() then unwinds through
fail_queues, which calls IFDI_DETACH() and IFDI_QUEUES_FREE() so the
driver can release whatever it set up in attach_pre. The one exception
is iflib_queues_alloc(): it jumped straight to fail_intr_free, so the
driver never learned that attach failed. Anything it allocated in
attach_pre was leaked -- for aq(4) that is the BAR0 resource, a 512-byte
VLAN bitmap, and an initialized mutex whose freed memory stays on the
witness lock list. The per-interface admin taskqueue, created earlier
in iflib_device_register(), was leaked on that path as well.

That path cannot simply join fail_queues: iflib_queues_alloc() already
frees the queue rings and clears ifc_txqs/ifc_rxqs on failure, so
iflib_tx_structures_free() would walk a NULL array rather than leak.
Add a label after the structure teardown instead, so the queue
allocation failure gets IFDI_DETACH(), IFDI_QUEUES_FREE() and
taskqueue_free() without repeating the frees iflib_queues_alloc() has
already done. IFDI_QUEUES_FREE() is needed because the driver's TX
queue allocation may have succeeded before the RX one failed.

fail_intr_free loses its last goto and becomes a plain fallthrough, so
drop the label.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 75116
Build 71999: arc lint + arc unit

Event Timeline

nick_spun.io retitled this revision from iflib: free the device sysctls on every attach-failure path to iflib: free the device pre-attach sysctls on every attach-failure path.Tue, Jul 21, 3:13 PM
nick_spun.io edited the summary of this revision. (Show Details)
nick_spun.io edited the summary of this revision. (Show Details)

Fold in the queue-allocation attach-failure leak; both fixes are on the same iflib_device_register() failure ladder.

nick_spun.io retitled this revision from iflib: free the device pre-attach sysctls on every attach-failure path to iflib: clean up correctly when device attach fails.Sat, Jul 25, 3:56 AM
nick_spun.io edited the summary of this revision. (Show Details)
adrian added a reviewer: gallatin.
adrian added a subscriber: gallatin.

@gallatin would you mind eyeballing this? ty!