[src/trunk]: src/sys/dev/usb usb: In usbd_transfer, test whether aborting und...

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

details:   https://anonhg.NetBSD.org/src/rev/2e925d36ffd1
branches:  trunk
changeset: 362569:2e925d36ffd1
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Thu Mar 03 06:09:57 2022 +0000

description:
usb: In usbd_transfer, test whether aborting under the lock.

Otherwise this test is racy and can cause the bad state of a pipe
with a transfer that will never be completed in a pipe that's about
to close under the expectation that the pipe is empty.

diffstat:

 sys/dev/usb/usbdi.c |  26 ++++++++++++++++----------
 1 files changed, 16 insertions(+), 10 deletions(-)

diffs (57 lines):

diff -r fff8cea755eb -r 2e925d36ffd1 sys/dev/usb/usbdi.c
--- a/sys/dev/usb/usbdi.c       Thu Mar 03 06:09:44 2022 +0000
+++ b/sys/dev/usb/usbdi.c       Thu Mar 03 06:09:57 2022 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: usbdi.c,v 1.229 2022/03/03 06:09:44 riastradh Exp $    */
+/*     $NetBSD: usbdi.c,v 1.230 2022/03/03 06:09:57 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.229 2022/03/03 06:09:44 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: usbdi.c,v 1.230 2022/03/03 06:09:57 riastradh Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_usb.h"
@@ -368,13 +368,6 @@
 #endif
        xfer->ux_done = 0;
 
-       if (pipe->up_aborting) {
-               USBHIST_LOG(usbdebug, "<- done xfer %#jx, aborting",
-                   (uintptr_t)xfer, 0, 0, 0);
-               SDT_PROBE2(usb, device, xfer, done,  xfer, USBD_CANCELLED);
-               return USBD_CANCELLED;
-       }
-
        KASSERT(xfer->ux_length == 0 || xfer->ux_buf != NULL);
 
        size = xfer->ux_length;
@@ -402,10 +395,23 @@
                }
        }
 
+       usbd_lock_pipe(pipe);
+       if (pipe->up_aborting) {
+               /*
+                * XXX For synchronous transfers this is fine.  What to
+                * do for asynchronous transfers?  The callback is
+                * never run, not even with status USBD_CANCELLED.
+                */
+               usbd_unlock_pipe(pipe);
+               USBHIST_LOG(usbdebug, "<- done xfer %#jx, aborting",
+                   (uintptr_t)xfer, 0, 0, 0);
+               SDT_PROBE2(usb, device, xfer, done,  xfer, USBD_CANCELLED);
+               return USBD_CANCELLED;
+       }
+
        /* xfer is not valid after the transfer method unless synchronous */
        SDT_PROBE2(usb, device, pipe, transfer__start,  pipe, xfer);
        do {
-               usbd_lock_pipe(pipe);
 #ifdef DIAGNOSTIC
                xfer->ux_state = XFER_ONQU;
 #endif