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

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

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 :)