[src/trunk]: src/sys/dev/usb usb(4): Fix xfer race between software abort and...

[riastradh <riastradh%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/0257be4b6f4b
branches:  trunk
changeset: 1023435:0257be4b6f4b
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Tue Sep 07 10:44:18 2021 +0000

description:
usb(4): Fix xfer race between software abort and hardware completion.

This fixes a bug in the API contract of usbd_abort_pipe: with the
change, the caller is guaranteed the xfer completion callbacks have
returned; without the change, completion callbacks could still be
running on the queued xfers while the caller of usbd_abort_pipe
proceeds to concurrently issue usbd_destroy_xfer.

This also fixes the following problem for interrupt pipes, whose
xfers stay on the queue until the pipe is aborted:

Thread 1: Hardware completion interrupt calls usb_transfer_complete.
Thread 1: pipe->up_repeat is 1, so usb_transfer_complete keeps xfer
  queued.
Thread 2: Calls usbd_abort_pipe (e.g., in detach).
Thread 2: usbd_abort_pipe waits for bus lock.
Thread 1: usb_transfer_complete releases bus lock to invoke callback.
Thread 2: Sets pipe->up_repeat := 0 (too late for thread 1 to see).
Thread 1: usb_transfer_complete waits to reacquire bus lock before
  resetting xfer status to USBD_NOT_STARTED.
Thread 2: Repeatdly calls upm_abort on the same xfer, which does
  nothing because upm_abort just does usbd_abort_xfer which does
  nothing because the xfer status is (e.g.) USBD_IOERROR and not
  USBD_IN_PROGRESS.

Thread 2 is now spinning forever with the bus lock held (and possibly
the kernel lock) waiting for queue or xfer status to change, which
will never happen as long as it holds the bus lock.

The resolution is for thread 2 to notice that thread 1 is busy
invoking a callback, and to wait until thread 1 has finished invoking
the callback and updated the xfer status to reset it to
USBD_NOT_STARTED at which point thread 1 can make progress again.

XXX pullup-9

diffstat:

 sys/dev/usb/usb_subr.c |  10 +++++++---
 sys/dev/usb/usbdi.c    |  17 +++++++++++++++--
 sys/dev/usb/usbdivar.h |   5 ++++-
 3 files changed, 26 insertions(+), 6 deletions(-)

diffs (117 lines):

diff -r 12e798f7d2f3 -r 0257be4b6f4b sys/dev/usb/usb_subr.c
--- a/sys/dev/usb/usb_subr.c    Tue Sep 07 10:44:04 2021 +0000
+++ b/sys/dev/usb/usb_subr.c    Tue Sep 07 10:44:18 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: usb_subr.c,v 1.266 2021/08/07 16:19:17 thorpej Exp $   */
+/*     $NetBSD: usb_subr.c,v 1.267 2021/09/07 10:44:18 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.266 2021/08/07 16:19:17 thorpej Exp $");
+__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.267 2021/09/07 10:44:18 riastradh Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_compat_netbsd.h"
@@ -946,6 +946,8 @@
        p->up_interval = ival;
        p->up_flags = flags;
        SIMPLEQ_INIT(&p->up_queue);
+       p->up_callingxfer = NULL;
+       cv_init(&p->up_callingcv, "usbpipecb");
 
        err = dev->ud_bus->ub_methods->ubm_open(p);
        if (err) {
@@ -964,8 +966,10 @@
        ep_acquired = false;    /* handed off to pipe */
        err = USBD_NORMAL_COMPLETION;
 
-out:   if (p)
+out:   if (p) {
+               cv_destroy(&p->up_callingcv);
                kmem_free(p, dev->ud_bus->ub_pipesize);
+       }
        if (ep_acquired)
                usbd_endpoint_release(dev, ep);
        return err;
diff -r 12e798f7d2f3 -r 0257be4b6f4b sys/dev/usb/usbdi.c
--- a/sys/dev/usb/usbdi.c       Tue Sep 07 10:44:04 2021 +0000
+++ b/sys/dev/usb/usbdi.c       Tue Sep 07 10:44:18 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: usbdi.c,v 1.218 2021/06/16 13:20:49 riastradh Exp $    */
+/*     $NetBSD: usbdi.c,v 1.219 2021/09/07 10:44:18 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.218 2021/06/16 13:20:49 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: usbdi.c,v 1.219 2021/09/07 10:44:18 riastradh Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_usb.h"
@@ -340,6 +340,7 @@
        pipe->up_methods->upm_close(pipe);
        usbd_unlock_pipe(pipe);
 
+       cv_destroy(&pipe->up_callingcv);
        if (pipe->up_intrxfer)
                usbd_destroy_xfer(pipe->up_intrxfer);
        usb_rem_task_wait(pipe->up_dev, &pipe->up_async_task, USB_TASKQ_DRIVER,
@@ -991,6 +992,13 @@
                        /* Make the HC abort it (and invoke the callback). */
                        SDT_PROBE1(usb, device, xfer, abort,  xfer);
                        pipe->up_methods->upm_abort(xfer);
+                       while (pipe->up_callingxfer == xfer) {
+                               USBHIST_LOG(usbdebug, "wait for callback"
+                                   "pipe = %#jx xfer = %#jx",
+                                   (uintptr_t)pipe, (uintptr_t)xfer, 0, 0);
+                               cv_wait(&pipe->up_callingcv,
+                                   pipe->up_dev->ud_bus->ub_lock);
+                       }
                        /* XXX only for non-0 usbd_clear_endpoint_stall(pipe); */
                }
        }
@@ -1092,6 +1100,8 @@
 
        if (xfer->ux_callback) {
                if (!polling) {
+                       KASSERT(pipe->up_callingxfer == NULL);
+                       pipe->up_callingxfer = xfer;
                        mutex_exit(pipe->up_dev->ud_bus->ub_lock);
                        if (!(pipe->up_flags & USBD_MPSAFE))
                                KERNEL_LOCK(1, curlwp);
@@ -1103,6 +1113,9 @@
                        if (!(pipe->up_flags & USBD_MPSAFE))
                                KERNEL_UNLOCK_ONE(curlwp);
                        mutex_enter(pipe->up_dev->ud_bus->ub_lock);
+                       KASSERT(pipe->up_callingxfer == xfer);
+                       pipe->up_callingxfer = NULL;
+                       cv_broadcast(&pipe->up_callingcv);
                }
        }
 
diff -r 12e798f7d2f3 -r 0257be4b6f4b sys/dev/usb/usbdivar.h
--- a/sys/dev/usb/usbdivar.h    Tue Sep 07 10:44:04 2021 +0000
+++ b/sys/dev/usb/usbdivar.h    Tue Sep 07 10:44:18 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: usbdivar.h,v 1.129 2021/08/02 12:56:24 andvar Exp $    */
+/*     $NetBSD: usbdivar.h,v 1.130 2021/09/07 10:44:18 riastradh Exp $ */
 
 /*
  * Copyright (c) 1998, 2012 The NetBSD Foundation, Inc.
@@ -249,6 +249,9 @@
        int                     up_interval;
        uint8_t                 up_flags;
 
+       struct usbd_xfer       *up_callingxfer; /* currently in callback */
+       kcondvar_t              up_callingcv;
+
        /* Filled by HC driver. */
        const struct usbd_pipe_methods
                               *up_methods;