Re: PR/45909: Use of MIDI over USB interfaces crashes NetBSD

[Iain Hibbert <plunky%rya-online.net@localhost>]

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

From: Iain Hibbert <plunky%rya-online.net@localhost>
To: Tom Ivar Helbekkmo <tih%hamartun.priv.no@localhost>
Cc: gnats-bugs%netbsd.org@localhost, current-users%netbsd.org@localhost
Subject: Re: PR/45909: Use of MIDI over USB interfaces crashes NetBSD
Date: Sat, 4 Feb 2012 12:46:13 +0000 (GMT)

 On Wed, 1 Feb 2012, Tom Ivar Helbekkmo wrote:
 
 (I changed the subject btw and Cc'd gnats bugs so this should get attached
 to the PR)
 
 > 1: the kernel lock needs to be held while an input transfer is
 >    initiated (the KASSERT(KERNEL_LOCKED_P()) is found in the function
 >    usbd_transfer() in /sys/dev/usb/usbdi.c).  I added code to grab and
 >    release the kernel lock to open_in_jack() in /sys/dev/usb/umidi.c,
 >    around the call to start_input_transfer().
 
 this bit seems ok to me
 
 > 2: the kernel lock must, again, be held while a transfer is aborted
 >    (by calling usbd_abort_xfer()).  This happens while closing a MIDI
 >    interface that was open for reading, and midiclose(), in
 >    /sys/dev/midi.c, does not itself grab the lock.  Again, a KASSERT()
 >    fails for this situation.  I added code to grab and release the
 >    kernel lock to close_in_jack() in /sys/dev/usb/umidi.c.
 
 I think you meant usbd_abort_pipe() btw? but given that, seems ok..
 
 > 3: The softc spinlock mutex must *not* be held during the above
 >    mentioned call to usbd_abort_xfer(), but midiclose() does hold it
 >    while calling the umidi driver's closing function.  This fails
 >    because usbd_abort_xfer() wants to relinquish the CPU to allow the
 >    hardware time to catch up, and a context switch is then attempted
 >    with the mutex held.  I added code to release and re-grab the mutex
 >    around the call to usbd_abort_xfer() in /sys/dev/usb/umidi.c.
 
 I don't think its right to release/aquire a lock that a higher layer is
 holding to protect its access to the resource, since the resource can
 change from underneath it..
 
 and if usbd_abort_pipe() is relinquishing the CPU, what about the kernel
 lock?
 
 > 4: After a MIDI device that was open for read has been closed (with
 >    the above fixes in place), reopening it for read fails because of
 >    the state it is left in by usbd_abort_xfer().  Specifically, the
 >    first attempt to start an input transfer on it returns
 >    USBD_CANCELLED, reflecting the aborted transfer.  This is benign,
 >    and I simply added that return code to the set of those that are
 >    accepted as non-errors by the umidi driver.
 
 is there another way to fix this, so that CANCELLED is not returned? IIRC
 when I've used usbd_abort_pipe() I also closed the pipe and reopened it
 for the next use, not sure if that is actually required..
 
 > -int umididebug = 0;
 > +int umididebug = 1;
 
 that is unrelated :)
 
 >  #else
 >  #define DPRINTF(x)
 >  #define DPRINTFN(n,x)
 > @@ -255,6 +255,7 @@
 >  error:
 >      aprint_error_dev(self, "disabled.\n");
 >      sc->sc_dying = 1;
 > +    KERNEL_UNLOCK_ONE(curlwp);
 >      return;
 
 and you didn't mention that (but it is needed :)
 
 > @@ -1415,12 +1430,17 @@
 >  static usbd_status
 >  start_input_transfer(struct umidi_endpoint *ep)
 >  {
 > +    usbd_status rv;
 > +
 > +    DPRINTFN(200,("umidi in transfer: start %p length %u\n",
 > +        ep->buffer, ep->buffer_size));
 >      usbd_setup_xfer(ep->xfer, ep->pipe,
 >                      (usbd_private_handle)ep,
 >                      ep->buffer, ep->buffer_size,
 >                      USBD_SHORT_XFER_OK | USBD_NO_COPY,
 >                          USBD_NO_TIMEOUT, in_intr);
 > -    return usbd_transfer(ep->xfer);
 > +    rv = usbd_transfer(ep->xfer);
 > +    return rv;
 >  }
 
 and this probably not needed?
 
 iain
 
 (I don't have any MIDI hardware btw, no way to test any of this :)