Re: Inconsistency in how URBs are unlinked

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Jan 16, 2020 at 01:34:21PM -0800, Chris Dickens wrote:
> Hi,
> 
> A bug [1] has been reported against libusb about a segfault that
> occurs when a device is disconnected while processing isochronous
> transfers.  In my investigation I discovered an interesting
> inconsistency in how URBs are unlinked across the USB core.
> 
> The usbfs driver will unlink URBs in the same order they are
> submitted.  From what I can see this code is executed when
> setting/releasing an interface or when alloc'ing/freeing streams.  I
> see there is also a call within the usbfs driver's disconnect
> function, but that appears to be a no-op (is this really the case?) as
> by the time that function is called the interface would have already
> been disabled and thus usb_hcd_flush_endpoint() would have been
> called.
> 
> Since commit 2eac136243 ("usb: core: unlink urbs from the tail of the
> endpoint's urb_list"), the usb_hcd_flush_endpoint() function will
> unlink URBs in the reverse order of submission.  This subtle change is
> what led to the crash within libusb.  The bug manifests when transfers
> within libusb are split into multiple URBs.  Prior to this change, the
> order in which URBs were reaped matched the order in which they were
> submitted.  Internally libusb expects this order to match and frees
> memory when it encounters the last URB in a multi-URB transfer, but
> since it reaps the last URB first the memory is freed right away and
> things take a turn when the freed memory is accessed when reaping the
> other URB(s) in that same transfer.

That commit was from July 2017, has no one really noticed since then?

Anyway, who is splitting the urb up, libusb or usbfs?  I'm guessing
libusb here as otherwise this wouldn't be an issue, but if you are
splitting them up, shouldn't you never count on the order in which they
are handled as some host controllers can reorder them (I think xhci
can).  So this type of fix for libusb is to be expected I would think,
you can't count on an async interface ever working in an ordered manner,
right?

Also, what does other operating systems do here?

> I will fix libusb to account for this behavior, but I thought it worth
> mentioning as this new behavior isn't what I (and possibly others)
> necessarily expect.

New as of 2017 :)

thanks,

greg k-h