Ensure to free memory and pci resources in order

[stallamr_netapp.com]

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.

[mhorne]

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]

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]

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]

• Add IFDI_QUEUES_FREE() to iflib_pseudo_deregister()

[markj]

sys/dev/e1000/if_em.c

1094

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

sys/net/iflib.c

5255

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]

sys/net/iflib.c

5255

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]

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]

• 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.