Ensure to free memory and pci resources in order
ClosedPublic
Actions

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

Details

Reviewers

markj
gallatin
shurd
erj
olivier
krzysztof.galazka_intel.com

Group Reviewers

iflib
NetApp

Commits

rG5b4cfd3f1315: iflib: Free resources in a consistent order during detach
rG859d72fa6538: iflib: Free resources in a consistent order during detach
rG38bfc6dee33b: iflib: Free resources in a consistent order during detach

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

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 35439
Build 32350: arc lint + arc unit

Event Timeline

stallamr_netapp.com created this revision.Dec 16 2020, 5:15 PM

Herald added a reviewer: iflib. · View Herald TranscriptDec 16 2020, 5:15 PM

Herald added subscribers: melifaro, ae, imp. · View Herald Transcript

stallamr_netapp.com requested review of this revision.Dec 16 2020, 5:15 PM

Harbormaster completed remote builds in B35439: Diff 80771.Dec 16 2020, 5:15 PM

stallamr_netapp.com added reviewers: NetApp, krzysztof.galazka_intel.com.Dec 16 2020, 5:17 PM

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.

markj added inline comments.Dec 21 2020, 7:22 PM

sys/net/iflib.c
5082	It looks like we're missing a call to IFDI_QUEUES_FREE here.
5141	Should this move down to just before the iflib_tx_structures_free() call, for consistency with other teardown paths?

stallamr_netapp.com marked an inline comment as done.Dec 21 2020, 7:56 PM

stallamr_netapp.com added inline comments.

sys/net/iflib.c
5082	Thanks for catching this. Also we are missing IFDI_DETACH(ctx) too. Will add that too and refresh the review.
5141	agree. Will make the change.

Add IFDI_QUEUES_FREE() to iflib_pseudo_deregister()

Harbormaster completed remote builds in B35581: Diff 81012.Dec 21 2020, 9:08 PM

markj accepted this revision.Dec 22 2020, 6:24 PM

markj added inline comments.

sys/dev/e1000/if_em.c
1096	Let's set `adapter->mta = NULL` here too for consistency.
sys/net/iflib.c
5153	Now I'm wondering why we need separate DETACH and QUEUES_FREE methods... that's outside the scope of this change though.

stallamr_netapp.com added inline comments.Dec 22 2020, 6:36 PM

sys/net/iflib.c
5153	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.

markj added inline comments.Dec 22 2020, 6:58 PM

sys/net/iflib.c
5144	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.

Ensure to release MSIX after freeing IRQ resources

sys/net/iflib.c
5144	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.