Re: kern/48151: USB race condition can lead to panic

[Nick Hudson <skrll%netbsd.org@localhost>]

On 08/23/13 17:45, vincent%labri.fr@localhost wrote:
> > Number:         48151
> > Category:       kern
> > Synopsis:       USB race condition can lead to panic
> > [...]

> > I submit this PR because it's not clear to me what is the right solution. The 
> > cv_broadcast/wakeup is done in two places :
> >
> > in the callbacks called by usb_transfer_complete()
> > right after, directly in usb_transfer_complete()
> >
> > Removing one of the two is required, but I don't know which is intended 
> > conceptually. Removing any of the two should fix the race because the only 
> > reason the xfer is reused in usb_transfer_complete() is to do this 
> > cv_broadcast()...

Looks like the callbacks can safely be removed - usbd_sync_transfer_sig 
DTRT.

How about the attached diff? It fixes utoppy at the same time.

Nick

Index: usbdi_util.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/usbdi_util.c,v
retrieving revision 1.59
diff -u -p -u -r1.59 usbdi_util.c
--- usbdi_util.c        5 Jan 2013 23:34:21 -0000       1.59
+++ usbdi_util.c        24 Aug 2013 08:23:30 -0000
@@ -415,19 +415,6 @@ usbd_get_config(usbd_device_handle dev, 
        return (usbd_do_request(dev, &req, conf));
 }
 
-Static void usbd_bulk_transfer_cb(usbd_xfer_handle xfer,
-                                 usbd_private_handle priv, usbd_status status);
-Static void
-usbd_bulk_transfer_cb(usbd_xfer_handle xfer, usbd_private_handle priv,
-                     usbd_status status)
-{
-
-       if (xfer->pipe->device->bus->lock)
-               cv_broadcast(&xfer->cv);
-       else
-               wakeup(xfer);   /* XXXSMP ok */
-}
-
 usbd_status
 usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_handle pipe,
                   u_int16_t flags, u_int32_t timeout, void *buf,
@@ -435,31 +422,19 @@ usbd_bulk_transfer(usbd_xfer_handle xfer
 {
        usbd_status err;
 
-       usbd_setup_xfer(xfer, pipe, 0, buf, *size,
-                       flags, timeout, usbd_bulk_transfer_cb);
-       DPRINTFN(1, ("usbd_bulk_transfer: start transfer %d bytes\n", *size));
+       usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout, NULL);
 
+       DPRINTFN(1, ("usbd_bulk_transfer: start transfer %d bytes\n", *size));
        err = usbd_sync_transfer_sig(xfer);
+
        usbd_get_xfer_status(xfer, NULL, NULL, size, NULL);
        DPRINTFN(1,("usbd_bulk_transfer: transferred %d\n", *size));
        if (err) {
                DPRINTF(("usbd_bulk_transfer: error=%d\n", err));
                usbd_clear_endpoint_stall(pipe);
        }
-       return (err);
-}
-
-Static void usbd_intr_transfer_cb(usbd_xfer_handle xfer,
-                                 usbd_private_handle priv, usbd_status status);
-Static void
-usbd_intr_transfer_cb(usbd_xfer_handle xfer, usbd_private_handle priv,
-                     usbd_status status)
-{
 
-       if (xfer->pipe->device->bus->lock)
-               cv_broadcast(&xfer->cv);
-       else
-               wakeup(xfer);   /* XXXSMP ok */
+       return (err);
 }
 
 usbd_status
@@ -469,11 +444,13 @@ usbd_intr_transfer(usbd_xfer_handle xfer
 {
        usbd_status err;
 
-       usbd_setup_xfer(xfer, pipe, 0, buf, *size,
-                       flags, timeout, usbd_intr_transfer_cb);
+       usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout, NULL);
+
        DPRINTFN(1, ("usbd_intr_transfer: start transfer %d bytes\n", *size));
        err = usbd_sync_transfer_sig(xfer);
+
        usbd_get_xfer_status(xfer, NULL, NULL, size, NULL);
+
        DPRINTFN(1,("usbd_intr_transfer: transferred %d\n", *size));
        if (err) {
                DPRINTF(("usbd_intr_transfer: error=%d\n", err));
Index: utoppy.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/utoppy.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 utoppy.c
--- utoppy.c    27 Oct 2012 17:18:38 -0000      1.21
+++ utoppy.c    24 Aug 2013 08:23:31 -0000
@@ -537,22 +537,10 @@ utoppy_bulk_transfer(usbd_xfer_handle xf
 
        usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout,
            utoppy_bulk_transfer_cb);
-       usbd_lock_pipe(pipe);   /* don't want callback until tsleep() */
-       err = usbd_transfer(xfer);
-       if (err != USBD_IN_PROGRESS) {
-               usbd_unlock_pipe(pipe);
-               return (err);
-       }
-       if (pipe->device->bus->lock)
-               error = cv_wait_sig(&xfer->cv, pipe->device->bus->lock);
-       else
-               error = tsleep((void *)xfer, PZERO, lbl, 0);
-       usbd_unlock_pipe(pipe);
-       if (error) {
-               usbd_abort_pipe(pipe);
-               return (USBD_INTERRUPTED);
-       }
-       usbd_get_xfer_status(xfer, NULL, NULL, size, &err);
+
+       err = usbd_sync_transfer_sig(xfer);
+
+       usbd_get_xfer_status(xfer, NULL, NULL, size, NULL);
        return (err);
 }