config_detach is used in too many places to audit for now -- so
although I'm quite sure it is racy (e.g., with cloning devices and
drvctl: drvctl -d a newly opened fss0 before sc_state has
transitioned from FSS_IDLE), this will mitigate the immediate fallout
until we can properly fix autoconf's notions of device pointers.
- Just use a reference count, not a list of pipes.
- Take the reference in usbd_open_pipe*, before we even look up the
endpoint by address; the endpoint is not stable until we hold the
interface and prevent usbd_set_interface.
- Make opening pipes just fail if usbd_set_interface is in progress.
=> No need to block -- might block for a while, and this is
essentially a driver error rather than a legitimate reason to
block.
=> This should maybe be a kassert, but it's not clear that ugen(4)
doesn't have a user-triggerable path to that kassert, so let's
keep it as a graceful failure for now until someone can audit
ugen(4) and make an informed decision.
- No need for a separate interface pipe lock; just use the bus lock.
This is a little bit longer than before, but makes the bracketed
nature of the references a little clearer and introduces more
kasserts to detect mistakes with internal API usage.
Otherwise, if uhub4 is attached at uhub1, then when we do
# drvctl -d uhub4
# drvctl -r -a usbdevif uhub1
the rescan never discovers devices attached recursively at uhub4, and
uhub4 leaks a config_pending_incr count so it can't be detached.
This would have made uhub's config_pending_incr leak more obvious by
crashing in KASSERT(dev->dv_pending == 0) early on, rather than
crashing in a tailq panic later on when the config_pending list gets
corrupted with use-after-free because nothing took the device off
dv_pending_list when attached.
(This is slightly academic now because config_detach blocks until
dev->dv_pending == 0, but it doesn't hurt and makes the intent
clearer.)
The arguments to config_attach_pseudo, config_init/fini_component,
and config_cfdata_attach/detach are generally statically allocated
objects in a module or the main kernel and as such are stable, and
there are no data structure invariants they assume the kernel lock
will covers from call to call. So there should be no need for the
caller to hold the kernel lock.
Should fix panic on modload of, e.g., nvmm.
Now either it replaces and frees the old endpoints array, or it
leaves everything in place; it never leaves a partial update nor
requires the caller to free the old array.
If there is an interface:
- Always put the pipe on the list in usbd_setup_pipe (if successful).
- Always have the pipe on the list from _before_ upm_open.
- Always keep the pipe on the list to _after_ upm_close, and after
the async task has completed.
This brings the logic in usbd_close_pipe and usbd_kill_pipe closer.
Rules:
1. After usbd_setup_pipe*, must usbd_kill_pipe.
2. After usbd_open_pipe*, must usbd_close_pipe.
Still haven't merged the logic in usbd_kill_pipe and usbd_close_pipe,
but getting closer.
Otherwise another concurrent detach -- e.g., from concurrent drvctl
or from USB port disconnection -- can pull the device_t out from
under us.
XXX Need to do this for _every_ operation config_* operation on a
device_t; device_find_by_xname is just fundamentally broken because a
concurrent detach can pull the device_t rug out from under you.
We really need another mechanism for holding a weak reference to a
device, and temporarily getting a strong reference to it; abusing
deviter is a bit of a kludge, and doesn't work very well because it
doesn't properly trigger garbage collection of devices detached while
other concurrent deviters are pending.
- Use config_pending_incr/decr around attach. No need for separate
flag indicating device is still attaching. (dv_flags locking is also
incoherent.)
- Any config_pending now blocks config_detach.
- New dv_detaching exclusive lock for config_detach.
mlelstv