usb: core: URB completer callback possibly called after usb_kill_anchored_urbs() returns

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.

Quoted 6 lines · Eli Billauer

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

As you have discovered, that is not a correct assumption.

Quoted 23 lines

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.

Quoted 5 lines

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.

Right. There is a genuine race. Althouogh usb_kill_anchored_urbs() does wait for the completion handlers of all the URBs it kills to finish, there is some ambiguity about what URBs are on the anchor.

Quoted 3 lines

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.

No, it isn't.

Quoted 6 lines

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.

Suppose the completion routine puts the URB onto a different anchor and then calls usb_submit_urb(). If __usb_hcd_giveback_urb() then called usb_unanchor_urb(), the URB would incorrectly be removed from the wrong anchor!

Currently the only way to handle this situation properly is to keep track of whether each URB has completed. For example, if the driver has successfully submitted 237 URBs but the completion routine has only been called 235 times, the driver will know that there are still two URBs pending.

Alan Stern

Hello Alan & all,

Thanks for your response.

The thing is that I'm not alone assuming that it's fine to free resources after usb_kill_anchored_urbs() returns. Most notable is usb-skeleton.c, which does exactly that in skel_disconnect():

    usb_kill_anchored_urbs(&dev->submitted);

    /* decrement our usage count */
    kref_put(&dev->kref, skel_delete);

Needless to say, skel_delete() frees the struct that the URBs' contexts point at.

Keeping track of the number of uncompleted URBs, as you suggested, is indeed a solution. But as it seems that the only problem is the race condition between usb_kill_anchored_urbs() and __usb_hcd_giveback_urb(), I suppose it's enough to ensure that the resources are not freed while __usb_hcd_giveback_urb() is doing its unanchor-before-complete thing.

After taking a second look, I discovered that there's already a function that takes the race condition into consideration:
usb_wait_anchor_empty_timeout().

Looking again at commit 6ec4147, which I mentioned before, it turns out that it added a counter to each anchor struct (atomic_t suspend_wakeups). It's incremented in __usb_hcd_giveback_urb() just before unanchoring a URB, and decremented after the completion has been called. This is made with calls to usb_anchor_suspend_wakeups() and usb_anchor_resume_wakeups(), but that's the essence of these calls.

And there's a wait queue in place, which gets a wakeup call by usb_anchor_resume_wakeups(), if the anchor's list is empty and the counter is zero after decrementing it. In the said commit, usb_wait_anchor_empty_timeout() was modified to check the counter as well, so when it returns, the anchor is genuinely idle.

So I would say that the safe way to go is

  usb_kill_anchored_urbs(&ep->anchor);
  if (!usb_wait_anchor_empty_timeout(&ep->anchor, 1000)) {
     /* This is really bad */
  }
  /* Release memory */

And if indeed so, I'm thinking about submitting a patch, which adds a usb_wait_anchor_empty_timeout() at the bottom of usb_kill_anchored_urbs(). So that the function does what people out there think it does.

Have I missed something?