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

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

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

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

[Alan Stern]

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

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

[Eli Billauer]

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

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

[Alan Stern]

On Sat, Jul 25, 2020 at 07:44:02PM +0300, Eli Billauer wrote:
> 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?

That sounds like a good proposal to me.  The 1-second timeout is 
somewhat arbitrary, but I guess it's okay since we expect unlink 
operations to be pretty quick.

Alan Stern