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

[Eli Billauer <eli.billauer@gmail.com>]

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?

Thanks,
    Eli

On 24/07/20 18:51, Alan Stern wrote:
> On Fri, Jul 24, 2020 at 03:46:40PM +0300, Eli Billauer wrote:
>    
>> 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.
>
>    
>>   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.
>>      
> 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.
>
>    
>> 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.
>
>    
>> 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
>
>