Hello,
My understanding is it should be OK to assume that no calls to completer
callbacks will be made after usb_kill_anchored_urbs() returns (for that
anchor, of course). However __usb_hcd_giveback_urb() in
drivers/usb/core/hcd.c doesn't seem to work that way. It unanchors
first, then calls the complete method:
usb_unanchor_urb(urb);
if (likely(status == 0))
usb_led_activity(USB_LED_EVENT_HOST);
/* pass ownership to the completion handler */
urb->status = status;
kcov_remote_start_usb((u64)urb->dev->bus->busnum);
urb->complete(urb);
So if usb_kill_anchored_urbs() is called while __usb_hcd_giveback_urb()
is in the middle of this code passage, it will miss the URB that is
being finished, and possibly return before the completer has been called.
It might sound like a theoretic race condition, but I actually got a
kernel panic after yanking the USB plug in the middle of heavy traffic.
The panic's call trace indicated that the driver's completer callback
function attempted to access memory that had been freed previously. As
this happened within an IRQ, it was a fullblown computer freeze.
The same driver's memory freeing mechanism indeed calls
usb_kill_anchored_urbs() first, then frees the URBs' related data
structure. So it seems like it freed the memory just before the
completer callback was invoked.
I would love to submit a patch that moves the usb_unanchor_urb() call a
few rows down, but something tells me it's not that simple.
As a side note, the comment along with commit 6ec4147, which added
usb_anchor_{suspend,resume}_wakeups calls, said among others: "But
__usb_hcd_giveback_urb() calls usb_unanchor_urb before calling the
completion handler. This is necessary as the completion handler may
re-submit and re-anchor the urb". Not sure I understood this part, though.
Any insights?
Thanks,
Eli
