Differential D20747

iflib: Prevent kernel panic caused by loading driver with a specific interrupt configuration
ClosedPublic
Actions

Authored by erj on Jun 24 2019, 8:17 PM.

Details

Reviewers

shurd
marius

Group Reviewers

iflib

Commits

rS350509: iflib: Prevent kernel panic caused by loading driver with a specific interrupt…

Summary

If a device has only 1 MSI-X interrupt available and does not support either MSI or legacy
interrupts, iflib_device_register() will fail, leak memory and MSI resources, and the driver
will not load. Worse, if another iflib-using driver tries to unload afterwards, a kernel panic will occur
because the previous failed iflib driver loead did not properly call "taskqgroup_detach()" during it's
cleanup.

This patch is band-aid for this situation -- don't try allocating MSI or legacy interrupts if
a single MSI-X interrupt was allocated, but fail to load instead. As well, during the cleanup,
properly call taskqgroup_detach() on the admin task to prevent panics when other iflib
drivers unload.

This whole interrupt allocation process probably needs re-doing to properly support a single
MSI-X interrupt as well as supporting devices that only support MSI-X.

Signed-off-by: Eric Joyner <erj@freebsd.org>

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25013
Build 23728: arc lint + arc unit

Event Timeline

erj created this revision.Jun 24 2019, 8:17 PM

Herald added a reviewer: shurd. · View Herald TranscriptJun 24 2019, 8:17 PM

Herald added a reviewer: iflib. · View Herald Transcript

Herald added a subscriber: ae. · View Herald Transcript

Harbormaster completed remote builds in B25013: Diff 58964.Jun 24 2019, 8:17 PM

Apart from the style problem, this change looks good to me.
Have you looked at how hard it would be to actually support configurations
with just 1 MSI-X, though? I think to remember the assumption that a single
interrupt means what iflib(4) calls "legacy", i. e. INTx or MSI, in 1 or 2 places
and generally iflib(4) would need to be made less smart and not so aggressive
in setting up interrupts, but nothing which seemed that much work.

sys/net/iflib.c
4747	Please wrap the line after "dev," so the code stays within 80 columns.

This revision now requires changes to proceed.Jun 24 2019, 8:42 PM

Wrap line to keep it under 80 cols

Harbormaster completed remote builds in B25018: Diff 58976.Jun 24 2019, 11:26 PM

erj marked an inline comment as done.Jun 24 2019, 11:27 PM

Looks good to me now, thanks.
For committing, please extend the commit message to additionally state
that the iflib(4) interrupt allocation needs re-doing not only to also support
single-message MSI-X and MSI-X-only devices, but for none-PCI devices
and multiple interrupt vectors that are not MSI-X as well, though.

This revision is now accepted and ready to land.Jun 26 2019, 12:57 PM

Closed by commit rS350509: iflib: Prevent kernel panic caused by loading driver with a specific interrupt… (authored by erj). · Explain WhyAug 1 2019, 5:37 PM

This revision was automatically updated to reflect the committed changes.

erj added a commit: rS350509: iflib: Prevent kernel panic caused by loading driver with a specific interrupt….

Herald added a subscriber: imp. · View Herald TranscriptAug 1 2019, 5:37 PM