Module Name: src
Committed By: riastradh
Date: Sun Jun 13 00:13:24 UTC 2021
Modified Files:
src/sys/dev/usb: usb_subr.c usbdi.c usbdivar.h
Log Message:
usb(4): Tighten interface locking and pipe references.
- 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.
To generate a diff of this commit:
cvs rdiff -u -r1.260 -r1.261 src/sys/dev/usb/usb_subr.c
cvs rdiff -u -r1.215 -r1.216 src/sys/dev/usb/usbdi.c
cvs rdiff -u -r1.127 -r1.128 src/sys/dev/usb/usbdivar.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/dev/usb/usb_subr.c
diff -u src/sys/dev/usb/usb_subr.c:1.260 src/sys/dev/usb/usb_subr.c:1.261
--- src/sys/dev/usb/usb_subr.c:1.260 Sat Jun 12 15:49:45 2021
+++ src/sys/dev/usb/usb_subr.c Sun Jun 13 00:13:24 2021
@@ -1,4 +1,4 @@
-/* $NetBSD: usb_subr.c,v 1.260 2021/06/12 15:49:45 riastradh Exp $ */
+/* $NetBSD: usb_subr.c,v 1.261 2021/06/13 00:13:24 riastradh Exp $ */
/* $FreeBSD: src/sys/dev/usb/usb_subr.c,v 1.18 1999/11/17 22:33:47 n_hibma Exp $ */
/*
@@ -32,7 +32,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.260 2021/06/12 15:49:45 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.261 2021/06/13 00:13:24 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_compat_netbsd.h"
@@ -415,8 +415,7 @@ usbd_iface_init(struct usbd_device *dev,
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,
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;
+}
- mutex_destroy(&ifc->ui_pipelock);
+void
+usbd_iface_pipeunref(struct usbd_interface *iface)
+{
+
+ 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
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
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
/* 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->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,15 @@ usbd_setup_pipe_flags(struct usbd_device
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 ep_acquired = false;
usbd_status err;
+ /* Block exclusive use of the endpoint by later pipes. */
err = usbd_endpoint_acquire(dev, ep, flags & USBD_EXCLUSIVE_USE);
if (err)
- return err;
+ goto out;
+ 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 +947,11 @@ usbd_setup_pipe_flags(struct usbd_device
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 out;
}
KASSERT(p->up_methods->upm_start || p->up_serialise == false);
@@ -874,7 +960,15 @@ usbd_setup_pipe_flags(struct usbd_device
USB_TASKQ_MPSAFE);
DPRINTFN(1, "pipe=%#jx", (uintptr_t)p, 0, 0, 0);
*pipe = p;
- return USBD_NORMAL_COMPLETION;
+ p = NULL; /* handed off to caller */
+ ep_acquired = false; /* handed off to pipe */
+ err = USBD_NORMAL_COMPLETION;
+
+out: if (p)
+ kmem_free(p, dev->ud_bus->ub_pipesize);
+ if (ep_acquired)
+ usbd_endpoint_release(dev, ep);
+ return err;
}
usbd_status
@@ -1712,7 +1806,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,
Index: src/sys/dev/usb/usbdi.c
diff -u src/sys/dev/usb/usbdi.c:1.215 src/sys/dev/usb/usbdi.c:1.216
--- src/sys/dev/usb/usbdi.c:1.215 Sat Jun 12 15:49:45 2021
+++ src/sys/dev/usb/usbdi.c Sun Jun 13 00:13:24 2021
@@ -1,4 +1,4 @@
-/* $NetBSD: usbdi.c,v 1.215 2021/06/12 15:49:45 riastradh Exp $ */
+/* $NetBSD: usbdi.c,v 1.216 2021/06/13 00:13:24 riastradh Exp $ */
/*
* Copyright (c) 1998, 2012, 2015 The NetBSD Foundation, Inc.
@@ -32,7 +32,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: usbdi.c,v 1.215 2021/06/12 15:49:45 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: usbdi.c,v 1.216 2021/06/13 00:13:24 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_usb.h"
@@ -225,6 +225,7 @@ usbd_open_pipe_ival(struct usbd_interfac
{
struct usbd_pipe *p;
struct usbd_endpoint *ep;
+ bool piperef = false;
usbd_status err;
int i;
@@ -232,22 +233,49 @@ usbd_open_pipe_ival(struct usbd_interfac
USBHIST_CALLARGS(usbdebug, "iface = %#jx address = %#jx flags = %#jx",
(uintptr_t)iface, address, flags, 0);
+ /*
+ * Block usbd_set_interface so we have a snapshot of the
+ * interface endpoints. They will remain stable until we drop
+ * the reference in usbd_close_pipe (or on failure here).
+ */
+ err = usbd_iface_piperef(iface);
+ if (err)
+ goto out;
+ piperef = true;
+
+ /* Find the endpoint at this address. */
for (i = 0; i < iface->ui_idesc->bNumEndpoints; i++) {
ep = &iface->ui_endpoints[i];
- if (ep->ue_edesc == NULL)
- return USBD_IOERROR;
+ if (ep->ue_edesc == NULL) {
+ err = USBD_IOERROR;
+ goto out;
+ }
if (ep->ue_edesc->bEndpointAddress == address)
- goto found;
+ break;
}
- return USBD_BAD_ADDRESS;
- found:
+ if (i == iface->ui_idesc->bNumEndpoints) {
+ err = USBD_BAD_ADDRESS;
+ goto out;
+ }
+
+ /* Set up the pipe with this endpoint. */
err = usbd_setup_pipe_flags(iface->ui_dev, iface, ep, ival, &p, flags);
if (err)
- return err;
+ goto out;
+
+ /* Success! */
*pipe = p;
+ p = NULL; /* handed off to caller */
+ piperef = false; /* handed off to pipe */
SDT_PROBE5(usb, device, pipe, open,
iface, address, flags, ival, p);
- return USBD_NORMAL_COMPLETION;
+ err = USBD_NORMAL_COMPLETION;
+
+out: if (p)
+ usbd_close_pipe(p);
+ if (piperef)
+ usbd_iface_pipeunref(iface);
+ return err;
}
usbd_status
@@ -316,12 +344,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 +890,17 @@ usbd_pipe2device_handle(struct usbd_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 +913,9 @@ usbd_set_interface(struct usbd_interface
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;
}
Index: src/sys/dev/usb/usbdivar.h
diff -u src/sys/dev/usb/usbdivar.h:1.127 src/sys/dev/usb/usbdivar.h:1.128
--- src/sys/dev/usb/usbdivar.h:1.127 Sat Jun 12 15:49:45 2021
+++ src/sys/dev/usb/usbdivar.h Sun Jun 13 00:13:24 2021
@@ -1,4 +1,4 @@
-/* $NetBSD: usbdivar.h,v 1.127 2021/06/12 15:49:45 riastradh Exp $ */
+/* $NetBSD: usbdivar.h,v 1.128 2021/06/13 00:13:24 riastradh Exp $ */
/*
* Copyright (c) 1998, 2012 The NetBSD Foundation, Inc.
@@ -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_
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 *);
