Inconsistency in how URBs are unlinked

Inconsistency in how URBs are unlinked

[Chris Dickens]

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.

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.

Chris

[1] https://github.com/libusb/libusb/issues/607

Re: Inconsistency in how URBs are unlinked

[Greg Kroah-Hartman]

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

Re: Inconsistency in how URBs are unlinked

[Alan Stern]

On Thu, 16 Jan 2020, Greg Kroah-Hartman wrote:

> 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.

You're talking about destroy_async(), right?  Yes, I should change that 
to make it go in backwards order -- thanks for pointing this out.

> >  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.

Not necessarily.  You can unbind a driver such as usbfs without 
disabling the interface.

> > 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?

Host controllers can't reorder URBs for the same endpoint, although 
they can reorder URBs for differing endpoints.

> Also, what does other operating systems do here?

In general it is better to unlink URBs in reverse order.  This
eliminates all possibility that the kernel will unlink URB x before it
executes but then URB x+1 will get executed before the kernel can
unlink it.

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

The kernel guarantees that URBs for a single endpoint will complete in
order _unless_ they have been unlinked.  Unlinked URBs can complete in
any order -- and not necessarily the order in which they were unlinked.

Alan Stern

Re: Inconsistency in how URBs are unlinked

[Chris Dickens]

[resending, hopefully in plain text]

Thank you both for your responses.  Perhaps it was luck that libusb
had worked fine until that change.  I suspect it was modeled after the
behavior of Linux at the time it was written.

In any case, you are both certainly right that no ordering should have
ever been expected in the error case.  I’ve patched it to not depend
on any specific reap ordering.

Chris

[PATCH] USB: usbfs: Always unlink URBs in reverse order

[Alan Stern]

When the kernel unlinks a bunch of URBs for a single endpoint, it
should always unlink them in reverse order.  This eliminates any
possibility that some URB x will be unlinked before it can execute but
the following URB x+1 will execute before it can be unlinked.  Such an
event would be bad, for obvious reasons.

Chris Dickens pointed out that usbfs doesn't behave this way when it
is unbound from an interface.  All pending URBs are cancelled, but in
the order of submission.  This patch changes the behavior to make the
unlinks occur in reverse order.  It similarly changes the behavior
when usbfs cancels the continuation URBs for a BULK endpoint.

Suggested-by: Chris Dickens <christopher.a.dickens@gmail.com>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

This doesn't fix any reported bugs, so I don't think it needs to go 
into the -stable kernels.


[as1929]


 drivers/usb/core/devio.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: usb-devel/drivers/usb/core/devio.c
===================================================================
--- usb-devel.orig/drivers/usb/core/devio.c
+++ usb-devel/drivers/usb/core/devio.c
@@ -574,7 +574,7 @@ __acquires(ps->lock)
 
 	/* Now carefully unlink all the marked pending URBs */
  rescan:
-	list_for_each_entry(as, &ps->async_pending, asynclist) {
+	list_for_each_entry_reverse(as, &ps->async_pending, asynclist) {
 		if (as->bulk_status == AS_UNLINK) {
 			as->bulk_status = 0;		/* Only once */
 			urb = as->urb;
@@ -636,7 +636,7 @@ static void destroy_async(struct usb_dev
 
 	spin_lock_irqsave(&ps->lock, flags);
 	while (!list_empty(list)) {
-		as = list_entry(list->next, struct async, asynclist);
+		as = list_last_entry(list, struct async, asynclist);
 		list_del_init(&as->asynclist);
 		urb = as->urb;
 		usb_get_urb(urb);