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

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

The following reply was made to PR kern/48151; it has been noted by GNATS.

From: Nick Hudson <skrll%netbsd.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: vincent%labri.fr@localhost, kern-bug-people%netbsd.org@localhost, 
gnats-admin%netbsd.org@localhost, 
 netbsd-bugs%netbsd.org@localhost
Subject: Re: kern/48151: USB race condition can lead to panic
Date: Sat, 24 Aug 2013 09:25:09 +0100

 This is a multi-part message in MIME format.
 --------------000308070401040900030405
 Content-Type: text/plain; charset=ISO-8859-1; format=flowed
 Content-Transfer-Encoding: 7bit
 
 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
 
 --------------000308070401040900030405
 Content-Type: text/plain; charset=us-ascii;
  name="diff"
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
  filename="diff"
 
 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);
  }
  
 
 --------------000308070401040900030405--