Page MenuHomeFreeBSD

Fix handling of BUS_PROBE_NOWILDCARD in 'device_probe_child()'.
ClosedPublic

Authored by neel on Apr 14 2015, 7:14 PM.

Details

Summary

Device probe value of BUS_PROBE_NOWILDCARD should be treated specially only
if the device has a fixed devclass. Otherwise it should be interpreted just
as if the driver doesn't want to claim the device.

Prior to this change a device that was not claimed explicitly by its driver
would remain "attached" to the driver that returned BUS_PROBE_NOWILDCARD.
This would bump up the reference on 'driver->refs' and its 'dev->ops' would
point to the 'driver->ops'. When the driver is subsequently unloaded the
'dev->ops->cls' is left pointing to freed memory.

This fixes an easily reproducible #GP fault caused by loading and unloading
vmm.ko multiple times.

Test Plan

kldload and kldunload vmm.ko multiple times and verify that it does not
trigger a #GP fault.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

neel retitled this revision from to Fix handling of BUS_PROBE_NOWILDCARD in 'device_probe_child()'..Apr 14 2015, 7:14 PM
neel updated this object.
neel added reviewers: imp, jhb, rstone.
neel edited the test plan for this revision. (Show Details)
neel updated this revision to Diff 4829.
neel added a comment.Apr 14 2015, 7:16 PM

The test plan should also verify that VF passthrough still works after this change.

Unfortunately I don't have SR-IOV capable hardware so hoping that Ryan would be able to verify this without too much effort.

jhb added inline comments.Apr 14 2015, 7:38 PM
sys/kern/subr_bus.c
2156 ↗(On Diff #4829)

Arguably the issue here is related to this XXX (namely there should be an else clause for the best == NULL case to clean up). However, I think your way of fixing this is fine (and cleaner).

OTOH, to help fix the XXX above, it seems like the device_set_driver(child, NULL) at line 2120 should just be unconditional. In the rebidding case we probably need to put the driver back (I don't see us do that anywhere which is probably an old bug)

rstone edited edge metadata.Apr 14 2015, 8:14 PM

I think that it would be sufficient to test that this still allows ppt to attach to a device dynamically:

  1. devctl detach ixl4
  2. devctl set driver pci11:0:3 ppt

(replace ixl4/pci11:0:3 with some unused PCI device from your test machine)

devctl also uses the same BUS_PROBE_NOWILDCARD feature that SR-IOV uses.

neel added inline comments.Apr 14 2015, 9:45 PM
sys/kern/subr_bus.c
2156 ↗(On Diff #4829)

I don't think DF_REBID works at all. I can see at least the following issues:

  • DF_REBID is not actually set in 'dev->flags' anywhere.
  • the driver needs to be detached at the top of this function for rebiddable devices:
    • without this device_add_driver() always fails so other drivers are not considered for this device

I can restructure the code but I don't see a way to test the DF_REBID case.

neel added a comment.Apr 14 2015, 9:48 PM
In D2294#6, @rstone wrote:

I think that it would be sufficient to test that this still allows ppt to attach to a device dynamically:

  1. devctl detach ixl4
  2. devctl set driver pci11:0:3 ppt

(replace ixl4/pci11:0:3 with some unused PCI device from your test machine)
devctl also uses the same BUS_PROBE_NOWILDCARD feature that SR-IOV uses.

Ok, thanks. This works fine.

imp edited edge metadata.Apr 14 2015, 9:48 PM
imp accepted this revision.
imp added inline comments.
sys/kern/subr_bus.c
2156 ↗(On Diff #4829)

DF_REBID does (or did) work when it was set. The code to set it never quite made it into the tree, however. Just leave the comment for now, since I have some patches to clean this stuff up so we can cope with multi-pass and drivers that get one thing in an early pass, but would match a better thing in a later pass...

This revision is now accepted and ready to land.Apr 14 2015, 9:48 PM
jhb edited edge metadata.Apr 15 2015, 1:49 PM
jhb accepted this revision.
neel closed this revision.Apr 15 2015, 4:22 PM
neel updated this revision to Diff 4839.

Closed by commit rS281559 (authored by @neel).