From f63e2133c9c8b80862d2fa31b1d55522f5ae30d3 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sat, 12 Jun 2021 16:47:58 +0000 Subject: [PATCH] usb(4): Tighten interface locking and pipe references. - Just use a reference count, not a list of pipes. - 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. --- sys/dev/usb/usb_subr.c | 148 +++++++++++++++++++++++++++++++++++------ sys/dev/usb/usbdi.c | 19 +++--- sys/dev/usb/usbdivar.h | 9 ++- 3 files changed, 141 insertions(+), 35 deletions(-) diff --git a/sys/dev/usb/usb_subr.c b/sys/dev/usb/usb_subr.c index c92c05163ac3..ff83764d77cf 100644 --- a/sys/dev/usb/usb_subr.c +++ b/sys/dev/usb/usb_subr.c @@ -415,8 +415,7 @@ usbd_iface_init(struct usbd_device *dev, int ifaceidx) ifc->ui_index = 0; ifc->ui_altindex = 0; ifc->ui_endpoints = NULL; - LIST_INIT(&ifc->ui_pipes); - mutex_init(&ifc->ui_pipelock, MUTEX_DEFAULT, IPL_NONE); + ifc->ui_busy = 0; } static void @@ -429,9 +428,100 @@ usbd_iface_fini(struct usbd_device *dev, int ifaceidx) KASSERT(ifc->ui_index == 0); KASSERT(ifc->ui_altindex == 0); KASSERT(ifc->ui_endpoints == NULL); - KASSERT(LIST_EMPTY(&ifc->ui_pipes)); + KASSERTMSG(ifc->ui_busy == 0, "%"PRId64, ifc->ui_busy); +} + +/* + * usbd_iface_lock/locked/unlock, usbd_iface_piperef/pipeunref + * + * We lock the interface while we are setting it, and we acquire a + * reference to the interface for each pipe opened on it. + * + * Setting the interface while pipes are open is forbidden, and + * opening pipes while the interface is being set is forbidden. + */ + +bool +usbd_iface_locked(struct usbd_interface *iface) +{ + bool locked; + + mutex_enter(iface->ui_dev->ud_bus->ub_lock); + locked = (iface->ui_busy == -1); + mutex_exit(iface->ui_dev->ud_bus->ub_lock); + + return locked; +} + +static void +usbd_iface_exlock(struct usbd_interface *iface) +{ + + mutex_enter(iface->ui_dev->ud_bus->ub_lock); + KASSERTMSG(iface->ui_busy == 0, "interface is not idle," + " busy=%"PRId64, iface->ui_busy); + iface->ui_busy = -1; + mutex_exit(iface->ui_dev->ud_bus->ub_lock); +} + +usbd_status +usbd_iface_lock(struct usbd_interface *iface) +{ + usbd_status err; + + mutex_enter(iface->ui_dev->ud_bus->ub_lock); + KASSERTMSG(iface->ui_busy != -1, "interface is locked"); + KASSERTMSG(iface->ui_busy >= 0, "%"PRId64, iface->ui_busy); + if (iface->ui_busy) { + err = USBD_IN_USE; + } else { + iface->ui_busy = -1; + err = 0; + } + mutex_exit(iface->ui_dev->ud_bus->ub_lock); + + return err; +} + +void +usbd_iface_unlock(struct usbd_interface *iface) +{ + + mutex_enter(iface->ui_dev->ud_bus->ub_lock); + KASSERTMSG(iface->ui_busy == -1, "interface is not locked," + " busy=%"PRId64, iface->ui_busy); + iface->ui_busy = 0; + mutex_exit(iface->ui_dev->ud_bus->ub_lock); +} + +usbd_status +usbd_iface_piperef(struct usbd_interface *iface) +{ + usbd_status err; + + mutex_enter(iface->ui_dev->ud_bus->ub_lock); + KASSERTMSG(iface->ui_busy >= -1, "%"PRId64, iface->ui_busy); + if (iface->ui_busy == -1) { + err = USBD_IN_USE; + } else { + iface->ui_busy++; + err = 0; + } + mutex_exit(iface->ui_dev->ud_bus->ub_lock); + + return err; +} + +void +usbd_iface_pipeunref(struct usbd_interface *iface) +{ - mutex_destroy(&ifc->ui_pipelock); + mutex_enter(iface->ui_dev->ud_bus->ub_lock); + KASSERTMSG(iface->ui_busy != -1, "interface is locked"); + KASSERTMSG(iface->ui_busy != 0, "interface not in use"); + KASSERTMSG(iface->ui_busy >= 1, "%"PRId64, iface->ui_busy); + iface->ui_busy--; + mutex_exit(iface->ui_dev->ud_bus->ub_lock); } usbd_status @@ -447,7 +537,7 @@ usbd_fill_iface_data(struct usbd_device *dev, int ifaceidx, int altidx) int endpt, nendpt; KASSERT(ifc->ui_dev == dev); - KASSERT(LIST_EMPTY(&ifc->ui_pipes)); + KASSERT(usbd_iface_locked(ifc)); idesc = usbd_find_idesc(dev->ud_cdesc, ifaceidx, altidx); if (idesc == NULL) @@ -547,7 +637,7 @@ usbd_free_iface_data(struct usbd_device *dev, int ifcno) KASSERT(ifc->ui_dev == dev); KASSERT(ifc->ui_idesc != NULL); - KASSERT(LIST_EMPTY(&ifc->ui_pipes)); + KASSERT(usbd_iface_locked(ifc)); if (ifc->ui_endpoints) { int nendpt = ifc->ui_idesc->bNumEndpoints; @@ -608,7 +698,9 @@ usbd_set_config_index(struct usbd_device *dev, int index, int msg) /* Free all configuration data structures. */ nifc = dev->ud_cdesc->bNumInterface; for (ifcidx = 0; ifcidx < nifc; ifcidx++) { + usbd_iface_exlock(&dev->ud_ifaces[ifcidx]); usbd_free_iface_data(dev, ifcidx); + usbd_iface_unlock(&dev->ud_ifaces[ifcidx]); usbd_iface_fini(dev, ifcidx); } kmem_free(dev->ud_ifaces, nifc * sizeof(struct usbd_interface)); @@ -783,10 +875,14 @@ usbd_set_config_index(struct usbd_device *dev, int index, int msg) dev->ud_config = cdp->bConfigurationValue; for (ifcidx = 0; ifcidx < nifc; ifcidx++) { usbd_iface_init(dev, ifcidx); + usbd_iface_exlock(&dev->ud_ifaces[ifcidx]); err = usbd_fill_iface_data(dev, ifcidx, 0); + usbd_iface_unlock(&dev->ud_ifaces[ifcidx]); if (err) { while (--ifcidx >= 0) { + usbd_iface_exlock(&dev->ud_ifaces[ifcidx]); usbd_free_iface_data(dev, ifcidx); + usbd_iface_unlock(&dev->ud_ifaces[ifcidx]); usbd_iface_fini(dev, ifcidx); } kmem_free(dev->ud_ifaces, @@ -827,12 +923,23 @@ usbd_setup_pipe_flags(struct usbd_device *dev, struct usbd_interface *iface, USBHIST_FUNC(); USBHIST_CALLARGS(usbdebug, "dev=%#jx addr=%jd iface=%#jx ep=%#jx", (uintptr_t)dev, dev->ud_addr, (uintptr_t)iface, (uintptr_t)ep); - struct usbd_pipe *p; + struct usbd_pipe *p = NULL; + bool piperef = false, ep_acquired = false; usbd_status err; + /* Block usbd_set_interface. */ + if (iface) { + err = usbd_iface_piperef(iface); + if (err) + goto fail; + piperef = true; + } + + /* Block exclusive use of the endpoint by later pipes. */ err = usbd_endpoint_acquire(dev, ep, flags & USBD_EXCLUSIVE_USE); if (err) - return err; + goto fail; + ep_acquired = true; p = kmem_alloc(dev->ud_bus->ub_pipesize, KM_SLEEP); DPRINTFN(1, "pipe=%#jx", (uintptr_t)p, 0, 0, 0); @@ -848,24 +955,11 @@ usbd_setup_pipe_flags(struct usbd_device *dev, struct usbd_interface *iface, p->up_flags = flags; SIMPLEQ_INIT(&p->up_queue); - if (iface) { - mutex_enter(&iface->ui_pipelock); - LIST_INSERT_HEAD(&iface->ui_pipes, p, up_next); - mutex_exit(&iface->ui_pipelock); - } - err = dev->ud_bus->ub_methods->ubm_open(p); if (err) { DPRINTF("endpoint=%#jx failed, error=%jd", (uintptr_t)ep->ue_edesc->bEndpointAddress, err, 0, 0); - if (iface) { - mutex_enter(&iface->ui_pipelock); - LIST_REMOVE(p, up_next); - mutex_exit(&iface->ui_pipelock); - } - kmem_free(p, dev->ud_bus->ub_pipesize); - usbd_endpoint_release(dev, ep); - return err; + goto fail; } KASSERT(p->up_methods->upm_start || p->up_serialise == false); @@ -875,6 +969,14 @@ usbd_setup_pipe_flags(struct usbd_device *dev, struct usbd_interface *iface, DPRINTFN(1, "pipe=%#jx", (uintptr_t)p, 0, 0, 0); *pipe = p; return USBD_NORMAL_COMPLETION; + +fail: if (p) + kmem_free(p, dev->ud_bus->ub_pipesize); + if (ep_acquired) + usbd_endpoint_release(dev, ep); + if (iface && piperef) + usbd_iface_pipeunref(iface); + return err; } usbd_status @@ -1712,7 +1814,9 @@ usb_free_device(struct usbd_device *dev) if (dev->ud_ifaces != NULL) { nifc = dev->ud_cdesc->bNumInterface; for (ifcidx = 0; ifcidx < nifc; ifcidx++) { + usbd_iface_exlock(&dev->ud_ifaces[ifcidx]); usbd_free_iface_data(dev, ifcidx); + usbd_iface_unlock(&dev->ud_ifaces[ifcidx]); usbd_iface_fini(dev, ifcidx); } kmem_free(dev->ud_ifaces, diff --git a/sys/dev/usb/usbdi.c b/sys/dev/usb/usbdi.c index ddb5e811a1d0..1ba4c2088611 100644 --- a/sys/dev/usb/usbdi.c +++ b/sys/dev/usb/usbdi.c @@ -316,12 +316,9 @@ usbd_close_pipe(struct usbd_pipe *pipe) usbd_destroy_xfer(pipe->up_intrxfer); usb_rem_task_wait(pipe->up_dev, &pipe->up_async_task, USB_TASKQ_DRIVER, NULL); - if (pipe->up_iface) { - mutex_enter(&pipe->up_iface->ui_pipelock); - LIST_REMOVE(pipe, up_next); - mutex_exit(&pipe->up_iface->ui_pipelock); - } usbd_endpoint_release(pipe->up_dev, pipe->up_endpoint); + if (pipe->up_iface) + usbd_iface_pipeunref(pipe->up_iface); kmem_free(pipe, pipe->up_dev->ud_bus->ub_pipesize); return USBD_NORMAL_COMPLETION; @@ -865,17 +862,17 @@ usbd_pipe2device_handle(struct usbd_pipe *pipe) usbd_status usbd_set_interface(struct usbd_interface *iface, int altidx) { + bool locked = false; usb_device_request_t req; usbd_status err; USBHIST_FUNC(); USBHIST_CALLARGS(usbdebug, "iface %#jx", (uintptr_t)iface, 0, 0, 0); - mutex_enter(&iface->ui_pipelock); - if (LIST_FIRST(&iface->ui_pipes) != NULL) { - err = USBD_IN_USE; + err = usbd_iface_lock(iface); + if (err) goto out; - } + locked = true; err = usbd_fill_iface_data(iface->ui_dev, iface->ui_index, altidx); if (err) @@ -888,7 +885,9 @@ usbd_set_interface(struct usbd_interface *iface, int altidx) USETW(req.wLength, 0); err = usbd_do_request(iface->ui_dev, &req, 0); -out: mutex_exit(&iface->ui_pipelock); +out: /* XXX back out iface data? */ + if (locked) + usbd_iface_unlock(iface); return err; } diff --git a/sys/dev/usb/usbdivar.h b/sys/dev/usb/usbdivar.h index c275f6a2327f..6f036860990a 100644 --- a/sys/dev/usb/usbdivar.h +++ b/sys/dev/usb/usbdivar.h @@ -230,8 +230,7 @@ struct usbd_interface { int ui_index; int ui_altindex; struct usbd_endpoint *ui_endpoints; - kmutex_t ui_pipelock; - LIST_HEAD(, usbd_pipe) ui_pipes; + int64_t ui_busy; /* #pipes, or -1 if setting */ }; struct usbd_pipe { @@ -243,7 +242,6 @@ struct usbd_pipe { bool up_serialise; SIMPLEQ_HEAD(, usbd_xfer) up_queue; - LIST_ENTRY(usbd_pipe) up_next; struct usb_task up_async_task; struct usbd_xfer *up_intrxfer; /* used for repeating requests */ @@ -347,6 +345,11 @@ usbd_status usbd_reattach_device(device_t, struct usbd_device *, int, const int *); void usbd_remove_device(struct usbd_device *, struct usbd_port *); +bool usbd_iface_locked(struct usbd_interface *); +usbd_status usbd_iface_lock(struct usbd_interface *); +void usbd_iface_unlock(struct usbd_interface *); +usbd_status usbd_iface_piperef(struct usbd_interface *); +void usbd_iface_pipeunref(struct usbd_interface *); usbd_status usbd_fill_iface_data(struct usbd_device *, int, int); void usb_free_device(struct usbd_device *);