Page MenuHomeFreeBSD

Ensure to free memory and pci resources in order
ClosedPublic

Authored by stallamr_netapp.com on Dec 16 2020, 5:15 PM.

Details

Summary

Memory and PCI resources are freed with no particular order. As a result, later code access pointers/resources that are already freed.
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.

Test Plan

Tested Intel 1G/10G/40G on NetApp platforms in error path of iflib_device_register() via failpoints.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

FYI, I recently committed rS368281 (also obtained from NetApp), which added a safeguard against NULL adapter queues in em_free_pci_resources(). Your patch seems to solve the problem more completely.

sys/net/iflib.c
5180

It looks like we're missing a call to IFDI_QUEUES_FREE here.

5239–5246

Should this move down to just before the iflib_tx_structures_free() call, for consistency with other teardown paths?

stallamr_netapp.com added inline comments.
sys/net/iflib.c
5180

Thanks for catching this.

Also we are missing IFDI_DETACH(ctx) too. Will add that too and refresh the review.

5239–5246

agree. Will make the change.

stallamr_netapp.com marked an inline comment as done.
  • Add IFDI_QUEUES_FREE() to iflib_pseudo_deregister()
markj added inline comments.
sys/dev/e1000/if_em.c
1094

Let's set adapter->mta = NULL here too for consistency.

sys/net/iflib.c
5254

Now I'm wondering why we need separate DETACH and QUEUES_FREE methods... that's outside the scope of this change though.

sys/net/iflib.c
5254

You are right. We can absorb IFDI_QUEUES_FREE() into IFDI_DETACH().

May be the real intention to keep separate is to handle iflib_queues_alloc() failure. But iflib_queues_alloc() failure trickle down and eventually becomes iflib_device_register() failure, where we do IFDI_DETACH() anyway.

sys/net/iflib.c
5249

Hmm, sorry for not noticing sooner, but there is an ordering issue here. IFDI_DETACH releases the irq resources for the driver, but iflib_free_intr_mem() is responsible for releasing MSI-X vectors back to the bus. This is happening in the wrong order now, so I believe the pci_release_msi() call will fail.

stallamr_netapp.com marked an inline comment as done.
  • Ensure to release MSIX after freeing IRQ resources
sys/net/iflib.c
5249

That is very true. Thanks for noticing this.

pci_release_msi() was returning 16 i.,e EBUSY prior to the fix.
Now I re-ordered to have iflib_free_intr_mem() get free after IFDI_DETACH(). Now, I see pci_release_mem() return 0.
Refreshing the review with the change.

This revision was not accepted when it landed; it landed in state Needs Review.Mon, Feb 1, 4:18 PM
This revision was automatically updated to reflect the committed changes.