linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs
@ 2020-07-27  7:22 eli.billauer
  2020-07-27  9:21 ` Greg KH
  2020-07-27 10:14 ` Oliver Neukum
  0 siblings, 2 replies; 12+ messages in thread
From: eli.billauer @ 2020-07-27  7:22 UTC (permalink / raw)
  To: gregkh, linux-usb; +Cc: hdegoede, stern, Eli Billauer

From: Eli Billauer <eli.billauer@gmail.com>

usb_kill_anchored_urbs() is commonly used to cancel all URBs on an
anchor just before releasing resources which the URBs rely on. By doing
so, users of this function rely on that no completer callbacks will take
place from any URB on the anchor after it returns.

However if this function is called in parallel with __usb_hcd_giveback_urb
processing a URB on the anchor, the latter may call the completer
callback after usb_kill_anchored_urbs() returns. This can lead to a
kernel panic due to use after release of memory in interrupt context.

The race condition is that __usb_hcd_giveback_urb() first unanchors the URB
and then makes the completer callback. Such URB is hence invisible to
usb_kill_anchored_urbs(), allowing it to return before the completer has
been called, since the anchor's urb_list is empty.

This patch adds a call to usb_wait_anchor_empty_timeout() prior to
returning. This function waits until urb_list is empty (it should
already be), but more importantly, until @suspend_wakeups is zero.

The latter condition resolves the race condition, since @suspend_wakeups
is incremented by __usb_hcd_giveback_urb() before unanchoring a URB and
decremented after completing it. @suspend_wakeups is hence an upper limit
of the number of unanchored but uncompleted URBs. By waiting for it to be
zero, the race condition is eliminated, in the same way that another
problem was solved for the same race condition in commit 6ec4147e7bdb
("usb-anchor: Delay usb_wait_anchor_empty_timeout wake up till completion
is done").

An arbitrary timeout of 1000 ms should cover any sane completer
callback. The wait condition may also fail if the anchor is populated
while usb_kill_anchored_urbs() is called. Both timeout scenarios are
alarmingly weird, hence a WARN() is issued.

Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 drivers/usb/core/urb.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index da923ec17612..7fa23615199f 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -772,11 +772,12 @@ void usb_block_urb(struct urb *urb)
 EXPORT_SYMBOL_GPL(usb_block_urb);
 
 /**
- * usb_kill_anchored_urbs - cancel transfer requests en masse
+ * usb_kill_anchored_urbs -  kill all URBs associated with an anchor
  * @anchor: anchor the requests are bound to
  *
- * this allows all outstanding URBs to be killed starting
- * from the back of the queue
+ * This kills all outstanding URBs starting from the back of the queue,
+ * with guarantee that no completer callbacks will take place from the
+ * anchor after this function returns.
  *
  * This routine should not be called by a driver after its disconnect
  * method has returned.
@@ -784,6 +785,7 @@ EXPORT_SYMBOL_GPL(usb_block_urb);
 void usb_kill_anchored_urbs(struct usb_anchor *anchor)
 {
 	struct urb *victim;
+	int ret;
 
 	spin_lock_irq(&anchor->lock);
 	while (!list_empty(&anchor->urb_list)) {
@@ -798,6 +800,10 @@ void usb_kill_anchored_urbs(struct usb_anchor *anchor)
 		spin_lock_irq(&anchor->lock);
 	}
 	spin_unlock_irq(&anchor->lock);
+
+	ret = usb_wait_anchor_empty_timeout(anchor, 1000);
+
+	WARN(!ret, "Returning with non-empty anchor due to timeout");
 }
 EXPORT_SYMBOL_GPL(usb_kill_anchored_urbs);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs
  2020-07-27  7:22 [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs eli.billauer
@ 2020-07-27  9:21 ` Greg KH
  2020-07-27 11:26   ` Eli Billauer
  2020-07-27 10:14 ` Oliver Neukum
  1 sibling, 1 reply; 12+ messages in thread
From: Greg KH @ 2020-07-27  9:21 UTC (permalink / raw)
  To: eli.billauer; +Cc: linux-usb, hdegoede, stern

On Mon, Jul 27, 2020 at 10:22:25AM +0300, eli.billauer@gmail.com wrote:
> From: Eli Billauer <eli.billauer@gmail.com>
> 
> usb_kill_anchored_urbs() is commonly used to cancel all URBs on an
> anchor just before releasing resources which the URBs rely on. By doing
> so, users of this function rely on that no completer callbacks will take
> place from any URB on the anchor after it returns.
> 
> However if this function is called in parallel with __usb_hcd_giveback_urb
> processing a URB on the anchor, the latter may call the completer
> callback after usb_kill_anchored_urbs() returns. This can lead to a
> kernel panic due to use after release of memory in interrupt context.
> 
> The race condition is that __usb_hcd_giveback_urb() first unanchors the URB
> and then makes the completer callback. Such URB is hence invisible to
> usb_kill_anchored_urbs(), allowing it to return before the completer has
> been called, since the anchor's urb_list is empty.
> 
> This patch adds a call to usb_wait_anchor_empty_timeout() prior to
> returning. This function waits until urb_list is empty (it should
> already be), but more importantly, until @suspend_wakeups is zero.
> 
> The latter condition resolves the race condition, since @suspend_wakeups
> is incremented by __usb_hcd_giveback_urb() before unanchoring a URB and
> decremented after completing it. @suspend_wakeups is hence an upper limit
> of the number of unanchored but uncompleted URBs. By waiting for it to be
> zero, the race condition is eliminated, in the same way that another
> problem was solved for the same race condition in commit 6ec4147e7bdb
> ("usb-anchor: Delay usb_wait_anchor_empty_timeout wake up till completion
> is done").
> 
> An arbitrary timeout of 1000 ms should cover any sane completer
> callback. The wait condition may also fail if the anchor is populated
> while usb_kill_anchored_urbs() is called. Both timeout scenarios are
> alarmingly weird, hence a WARN() is issued.
> 
> Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
> ---
>  drivers/usb/core/urb.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index da923ec17612..7fa23615199f 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -772,11 +772,12 @@ void usb_block_urb(struct urb *urb)
>  EXPORT_SYMBOL_GPL(usb_block_urb);
>  
>  /**
> - * usb_kill_anchored_urbs - cancel transfer requests en masse
> + * usb_kill_anchored_urbs -  kill all URBs associated with an anchor
>   * @anchor: anchor the requests are bound to
>   *
> - * this allows all outstanding URBs to be killed starting
> - * from the back of the queue
> + * This kills all outstanding URBs starting from the back of the queue,
> + * with guarantee that no completer callbacks will take place from the
> + * anchor after this function returns.
>   *
>   * This routine should not be called by a driver after its disconnect
>   * method has returned.
> @@ -784,6 +785,7 @@ EXPORT_SYMBOL_GPL(usb_block_urb);
>  void usb_kill_anchored_urbs(struct usb_anchor *anchor)
>  {
>  	struct urb *victim;
> +	int ret;
>  
>  	spin_lock_irq(&anchor->lock);
>  	while (!list_empty(&anchor->urb_list)) {
> @@ -798,6 +800,10 @@ void usb_kill_anchored_urbs(struct usb_anchor *anchor)
>  		spin_lock_irq(&anchor->lock);
>  	}
>  	spin_unlock_irq(&anchor->lock);
> +
> +	ret = usb_wait_anchor_empty_timeout(anchor, 1000);
> +
> +	WARN(!ret, "Returning with non-empty anchor due to timeout");

Don't do a warn-on for things that can be triggered by userspace (i.e.
"bad USB device), as that can cause systems to reboot, and will get
caught by the syzbot testing tool.

Also, will this cause things to take longer than they used to?  What
race conditions is this going to cause?  :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs
  2020-07-27  7:22 [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs eli.billauer
  2020-07-27  9:21 ` Greg KH
@ 2020-07-27 10:14 ` Oliver Neukum
  2020-07-27 11:27   ` Eli Billauer
  1 sibling, 1 reply; 12+ messages in thread
From: Oliver Neukum @ 2020-07-27 10:14 UTC (permalink / raw)
  To: eli.billauer, gregkh, linux-usb; +Cc: hdegoede, stern

Am Montag, den 27.07.2020, 10:22 +0300 schrieb eli.billauer@gmail.com:
> From: Eli Billauer <eli.billauer@gmail.com>
> 
> usb_kill_anchored_urbs() is commonly used to cancel all URBs on an
> anchor just before releasing resources which the URBs rely on. By doing
> so, users of this function rely on that no completer callbacks will take
> place from any URB on the anchor after it returns.

Right, this is a use case that must work.

> However if this function is called in parallel with __usb_hcd_giveback_urb
> processing a URB on the anchor, the latter may call the completer
> callback after usb_kill_anchored_urbs() returns. This can lead to a
> kernel panic due to use after release of memory in interrupt context.
> 
> The race condition is that __usb_hcd_giveback_urb() first unanchors the URB
> and then makes the completer callback. Such URB is hence invisible to
> usb_kill_anchored_urbs(), allowing it to return before the completer has
> been called, since the anchor's urb_list is empty.

Well, the URB must be unachored because the callback may anchor the URB
again. What could we do? The refcount on the URB does not help, because
it guards only the URB itself.
It looks to me like I misdesigned the API a bit. What people really
need is an anchor that is not weighed by calling the callback.
Should I introduce such an API?

> This patch adds a call to usb_wait_anchor_empty_timeout() prior to
> returning. This function waits until urb_list is empty (it should
> already be), but more importantly, until @suspend_wakeups is zero.

That however is really a kludge we cannot have in usbcore.
I am afraid as is the patch should _not_ be applied.

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs
  2020-07-27  9:21 ` Greg KH
@ 2020-07-27 11:26   ` Eli Billauer
  0 siblings, 0 replies; 12+ messages in thread
From: Eli Billauer @ 2020-07-27 11:26 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-usb, hdegoede, stern, Oliver Neukum

On 27/07/20 12:21, Greg KH wrote:
> On Mon, Jul 27, 2020 at 10:22:25AM +0300,eli.billauer@gmail.com  wrote:
>    
>> From: Eli Billauer<eli.billauer@gmail.com>
>>
>> usb_kill_anchored_urbs() is commonly used to cancel all URBs on an
>> anchor just before releasing resources which the URBs rely on. By doing
>> so, users of this function rely on that no completer callbacks will take
>> place from any URB on the anchor after it returns.
>>
>> However if this function is called in parallel with __usb_hcd_giveback_urb
>> processing a URB on the anchor, the latter may call the completer
>> callback after usb_kill_anchored_urbs() returns. This can lead to a
>> kernel panic due to use after release of memory in interrupt context.
>>
>> The race condition is that __usb_hcd_giveback_urb() first unanchors the URB
>> and then makes the completer callback. Such URB is hence invisible to
>> usb_kill_anchored_urbs(), allowing it to return before the completer has
>> been called, since the anchor's urb_list is empty.
>>
>> This patch adds a call to usb_wait_anchor_empty_timeout() prior to
>> returning. This function waits until urb_list is empty (it should
>> already be), but more importantly, until @suspend_wakeups is zero.
>>
>> The latter condition resolves the race condition, since @suspend_wakeups
>> is incremented by __usb_hcd_giveback_urb() before unanchoring a URB and
>> decremented after completing it. @suspend_wakeups is hence an upper limit
>> of the number of unanchored but uncompleted URBs. By waiting for it to be
>> zero, the race condition is eliminated, in the same way that another
>> problem was solved for the same race condition in commit 6ec4147e7bdb
>> ("usb-anchor: Delay usb_wait_anchor_empty_timeout wake up till completion
>> is done").
>>
>> An arbitrary timeout of 1000 ms should cover any sane completer
>> callback. The wait condition may also fail if the anchor is populated
>> while usb_kill_anchored_urbs() is called. Both timeout scenarios are
>> alarmingly weird, hence a WARN() is issued.
>>
>> Signed-off-by: Eli Billauer<eli.billauer@gmail.com>
>> ---
>>   drivers/usb/core/urb.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
>> index da923ec17612..7fa23615199f 100644
>> --- a/drivers/usb/core/urb.c
>> +++ b/drivers/usb/core/urb.c
>> @@ -772,11 +772,12 @@ void usb_block_urb(struct urb *urb)
>>   EXPORT_SYMBOL_GPL(usb_block_urb);
>>
>>   /**
>> - * usb_kill_anchored_urbs - cancel transfer requests en masse
>> + * usb_kill_anchored_urbs -  kill all URBs associated with an anchor
>>    * @anchor: anchor the requests are bound to
>>    *
>> - * this allows all outstanding URBs to be killed starting
>> - * from the back of the queue
>> + * This kills all outstanding URBs starting from the back of the queue,
>> + * with guarantee that no completer callbacks will take place from the
>> + * anchor after this function returns.
>>    *
>>    * This routine should not be called by a driver after its disconnect
>>    * method has returned.
>> @@ -784,6 +785,7 @@ EXPORT_SYMBOL_GPL(usb_block_urb);
>>   void usb_kill_anchored_urbs(struct usb_anchor *anchor)
>>   {
>>   	struct urb *victim;
>> +	int ret;
>>
>>   	spin_lock_irq(&anchor->lock);
>>   	while (!list_empty(&anchor->urb_list)) {
>> @@ -798,6 +800,10 @@ void usb_kill_anchored_urbs(struct usb_anchor *anchor)
>>   		spin_lock_irq(&anchor->lock);
>>   	}
>>   	spin_unlock_irq(&anchor->lock);
>> +
>> +	ret = usb_wait_anchor_empty_timeout(anchor, 1000);
>> +
>> +	WARN(!ret, "Returning with non-empty anchor due to timeout");
>>      
> Don't do a warn-on for things that can be triggered by userspace (i.e.
> "bad USB device), as that can cause systems to reboot, and will get
> caught by the syzbot testing tool.
>    
This WARN is triggered only if the device driver has a bug. Explained 
further below.
> Also, will this cause things to take longer than they used to?  What
> race conditions is this going to cause?  :)
>    
The impact of the call to usb_wait_anchor_empty_timeout() is virtually 
none. Almost always, it will return immediately, as the anchor's list 
has just been emptied.

It's only if a URB happened to be in the middle of being completed, that 
it will wait for this to finish, which should be practically unnoticed: 
Completer calls should be quick, so 1000 ms is a huge overkill. If this 
WARN gets into effect, it's because the driver's completer callback was 
running for 1000ms. It's a bug in the driver. No faulty USB device 
should be able to make this happen.

Alternatively, if the driver submits URBs to the same anchor while 
usb_kill_anchored_urbs() is called, this timeout might be reached. This 
could happen, for example, if the completer function that ran in the 
racy situation resubmits the URB. If that situation isn't cleared within 
1000ms, it means that there's a URB in the system that the driver isn't 
aware of. Maybe that situation is worth more than a WARN.

I originally thought about a goto to the beginning of the function after 
the WARN until the anchor's list is empty. But a really buggy driver 
could turn that into an infinite loop.

Regards,
    Eli
> thanks,
>
> greg k-h
>
>    


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs
  2020-07-27 10:14 ` Oliver Neukum
@ 2020-07-27 11:27   ` Eli Billauer
  2020-07-27 13:58     ` Oliver Neukum
  0 siblings, 1 reply; 12+ messages in thread
From: Eli Billauer @ 2020-07-27 11:27 UTC (permalink / raw)
  To: Oliver Neukum; +Cc: gregkh, linux-usb, hdegoede, stern

Hello, Oliver.

On 27/07/20 13:14, Oliver Neukum wrote:
> That however is really a kludge we cannot have in usbcore.
> I am afraid as is the patch should_not_  be applied.
>    
Could you please explain further why the suggested patch is unsuitable?

Thanks,
    Eli

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs
  2020-07-27 11:27   ` Eli Billauer
@ 2020-07-27 13:58     ` Oliver Neukum
  2020-07-27 14:43       ` Alan Stern
  0 siblings, 1 reply; 12+ messages in thread
From: Oliver Neukum @ 2020-07-27 13:58 UTC (permalink / raw)
  To: Eli Billauer; +Cc: gregkh, linux-usb, hdegoede, stern

Am Montag, den 27.07.2020, 14:27 +0300 schrieb Eli Billauer:
> Hello, Oliver.
> 
> On 27/07/20 13:14, Oliver Neukum wrote:
> > That however is really a kludge we cannot have in usbcore.
> > I am afraid as is the patch should_not_  be applied.
> >    
> 
> Could you please explain further why the suggested patch is unsuitable?

Hi,

certainly.

1. timeouts are generally a bad idea, especially if the timeout does
not come out of a spec.

2. That involves quoting you:

Alternatively, if the driver submits URBs to the same anchor while 
usb_kill_anchored_urbs() is called, this timeout might be reached. This 
could happen, for example, if the completer function that ran in the 
racy situation resubmits the URB. If that situation isn't cleared within 
1000ms, it means that there's a URB in the system that the driver isn't 
aware of. Maybe that situation is worth more than a WARN.

That is an entirely valid use case. And a bulk URB may take a potentially
unbounded time to complete.

My failure in this case is simply overengineering.
If this line:

        usb_unanchor_urb(urb);

In __usb_hcd_giveback_urb(struct urb *urb) weren't there, the issue
would not exist. I misdesigned the API in automatically unanchoring
a completing URB.
Simply removing it now is no longer possible, so we need to come up with
a more complex solution.

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs
  2020-07-27 13:58     ` Oliver Neukum
@ 2020-07-27 14:43       ` Alan Stern
  2020-07-27 21:29         ` Oliver Neukum
  2020-07-28  9:44         ` Oliver Neukum
  0 siblings, 2 replies; 12+ messages in thread
From: Alan Stern @ 2020-07-27 14:43 UTC (permalink / raw)
  To: Oliver Neukum; +Cc: Eli Billauer, gregkh, linux-usb, hdegoede

On Mon, Jul 27, 2020 at 03:58:05PM +0200, Oliver Neukum wrote:
> Am Montag, den 27.07.2020, 14:27 +0300 schrieb Eli Billauer:
> > Hello, Oliver.
> > 
> > On 27/07/20 13:14, Oliver Neukum wrote:
> > > That however is really a kludge we cannot have in usbcore.
> > > I am afraid as is the patch should_not_  be applied.
> > >    
> > 
> > Could you please explain further why the suggested patch is unsuitable?
> 
> Hi,
> 
> certainly.
> 
> 1. timeouts are generally a bad idea, especially if the timeout does
> not come out of a spec.
> 
> 2. That involves quoting you:
> 
> Alternatively, if the driver submits URBs to the same anchor while 
> usb_kill_anchored_urbs() is called, this timeout might be reached. This 

That would be a bug in the driver, though.  In such a situation, a WARN 
is worth having.

> could happen, for example, if the completer function that ran in the 
> racy situation resubmits the URB. If that situation isn't cleared within 
> 1000ms, it means that there's a URB in the system that the driver isn't 
> aware of. Maybe that situation is worth more than a WARN.
> 
> That is an entirely valid use case. And a bulk URB may take a potentially
> unbounded time to complete.

It is _not_ a valid use case.  Since usb_kill_anchored_urbs() doesn't' 
specify whether it will kill URBs that are added to the anchor after it 
is called (and before it returns), a driver that anchors URBs at such a 
time is buggy.

Maybe this should be mentioned in the kerneldoc for the routine: Drivers 
must not add URBs to the anchor while the routine is running.

> My failure in this case is simply overengineering.
> If this line:
> 
>         usb_unanchor_urb(urb);
> 
> In __usb_hcd_giveback_urb(struct urb *urb) weren't there, the issue
> would not exist. I misdesigned the API in automatically unanchoring
> a completing URB.
> Simply removing it now is no longer possible, so we need to come up with
> a more complex solution.

Given that this timeout-based API is already present and being used in a 
separate context, I don't see anything wrong with using it here as well.

Alan Stern

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs
  2020-07-27 14:43       ` Alan Stern
@ 2020-07-27 21:29         ` Oliver Neukum
  2020-07-28  9:47           ` Eli Billauer
  2020-07-28  9:44         ` Oliver Neukum
  1 sibling, 1 reply; 12+ messages in thread
From: Oliver Neukum @ 2020-07-27 21:29 UTC (permalink / raw)
  To: Alan Stern; +Cc: Eli Billauer, gregkh, linux-usb, hdegoede

Am Montag, den 27.07.2020, 10:43 -0400 schrieb Alan Stern:
> On Mon, Jul 27, 2020 at 03:58:05PM +0200, Oliver Neukum wrote:
> > Am Montag, den 27.07.2020, 14:27 +0300 schrieb Eli Billauer:
> > > Hello, Oliver.
> > > 
> > > On 27/07/20 13:14, Oliver Neukum wrote:
> > > > That however is really a kludge we cannot have in usbcore.
> > > > I am afraid as is the patch should_not_  be applied.
> > > >    
> > > 
> > > Could you please explain further why the suggested patch is unsuitable?
> > 
> > Hi,
> > 
> > certainly.
> > 
> > 1. timeouts are generally a bad idea, especially if the timeout does
> > not come out of a spec.
> > 
> > 2. That involves quoting you:
> > 
> > Alternatively, if the driver submits URBs to the same anchor while 
> > usb_kill_anchored_urbs() is called, this timeout might be reached. This 
> 
> That would be a bug in the driver, though.  In such a situation, a WARN 
> is worth having.

Well, it is an inherent race, certainly. You can do it, though. It is
debatable whether it would ever make sense. Yet it is not a bug in the
sense of, for example, writing beyond the end of a buffer or submitting
an URB twofold.

> > could happen, for example, if the completer function that ran in the 
> > racy situation resubmits the URB. If that situation isn't cleared within 
> > 1000ms, it means that there's a URB in the system that the driver isn't 
> > aware of. Maybe that situation is worth more than a WARN.
> > 
> > That is an entirely valid use case. And a bulk URB may take a potentially
> > unbounded time to complete.
> 
> It is _not_ a valid use case.  Since usb_kill_anchored_urbs() doesn't' 
> specify whether it will kill URBs that are added to the anchor after it 
> is called (and before it returns), a driver that anchors URBs at such a 
> time is buggy.

Yes, if you depend on it. Here we are getting into technicalities.
The thing is that we are getting into areas where we should not need to
go if the API were optimal.

What drivers really want is a way to say, kill this group of URBs and
make sure they stay dead no matter what.

> Maybe this should be mentioned in the kerneldoc for the routine: Drivers 
> must not add URBs to the anchor while the routine is running.

True, yet this defeats one of the aims of the API.

> > My failure in this case is simply overengineering.
> > If this line:
> > 
> >         usb_unanchor_urb(urb);
> > 
> > In __usb_hcd_giveback_urb(struct urb *urb) weren't there, the issue
> > would not exist. I misdesigned the API in automatically unanchoring
> > a completing URB.
> > Simply removing it now is no longer possible, so we need to come up with
> > a more complex solution.
> 
> Given that this timeout-based API is already present and being used in a 
> separate context, I don't see anything wrong with using it here as well.
> 

It is unnecessary and results in a much less useful API.
The true error in its design is that it unconditionally unanchors the
URBs it gives back. Stop doing that and it becomes much better.

	Regards
		Oliver



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs
  2020-07-27 14:43       ` Alan Stern
  2020-07-27 21:29         ` Oliver Neukum
@ 2020-07-28  9:44         ` Oliver Neukum
  2020-07-28 13:39           ` Alan Stern
  1 sibling, 1 reply; 12+ messages in thread
From: Oliver Neukum @ 2020-07-28  9:44 UTC (permalink / raw)
  To: Alan Stern; +Cc: Eli Billauer, gregkh, linux-usb, hdegoede

Am Montag, den 27.07.2020, 10:43 -0400 schrieb Alan Stern:
> Given that this timeout-based API is already present and being used in a 
> separate context, I don't see anything wrong with using it here as well.

A more elegant alternative.

	Regards
		Oliver

From c37e910758b0a05d4c6b8d058974b1264f9d0aef Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Tue, 28 Jul 2020 11:38:23 +0200
Subject: [PATCH] USB: add mooring API

This is a simplified and thereby better version of the anchor API.
Anchors have the problem that they unanchor an URB upon giveback,
which creates a window during which an URB is unanchored but not
yet returned, leading to operations on anchors not having the
semantics many driver errornously assume them to have.
The new API keeps an URB on an anchor until it is explicitly
unmoored.

Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
 drivers/usb/core/hcd.c |  3 ++-
 drivers/usb/core/urb.c | 27 ++++++++++++++++++++++++++-
 include/linux/usb.h    |  3 +++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index a33b849e8beb..861d30180709 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1640,7 +1640,8 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
 	unmap_urb_for_dma(hcd, urb);
 	usbmon_urb_complete(&hcd->self, urb, status);
 	usb_anchor_suspend_wakeups(anchor);
-	usb_unanchor_urb(urb);
+	if (!urb->transfer_flags && URB_ANCHOR_PERMANENT)
+		usb_unanchor_urb(urb);
 	if (likely(status == 0))
 		usb_led_activity(USB_LED_EVENT_HOST);
 
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 7bc23469f4e4..1acfbd4e6323 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(usb_get_urb);
  * This can be called to have access to URBs which are to be executed
  * without bothering to track them
  */
-void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
+static void __usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
 {
 	unsigned long flags;
 
@@ -137,8 +137,20 @@ void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
 
 	spin_unlock_irqrestore(&anchor->lock, flags);
 }
+
+void inline usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
+{
+	__usb_anchor_urb(urb, anchor);
+}
 EXPORT_SYMBOL_GPL(usb_anchor_urb);
 
+void usb_moor_urb(struct urb *urb, struct usb_anchor *anchor)
+{
+	urb->transfer_flags |= URB_ANCHOR_PERMANENT;
+	__usb_anchor_urb( urb, anchor);
+}
+EXPORT_SYMBOL_GPL(usb_moor_urb);
+
 static int usb_anchor_check_wakeup(struct usb_anchor *anchor)
 {
 	return atomic_read(&anchor->suspend_wakeups) == 0 &&
@@ -185,6 +197,19 @@ void usb_unanchor_urb(struct urb *urb)
 }
 EXPORT_SYMBOL_GPL(usb_unanchor_urb);
 
+void usb_unmoor_urb(struct urb *urb)
+{
+	struct usb_anchor *anchor;
+
+	anchor = urb->anchor;
+	if (!anchor)
+		return;
+
+	__usb_unanchor_urb(urb, anchor);
+	urb->transfer_flags &= ~URB_ANCHOR_PERMANENT;
+}
+EXPORT_SYMBOL_GPL(usb_unmoor_urb);
+
 /*-------------------------------------------------------------------*/
 
 static const int pipetypes[4] = {
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 20c555db4621..cacebdb01bfd 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1347,6 +1347,7 @@ extern int usb_disabled(void);
 #define URB_SETUP_MAP_LOCAL	0x00200000	/* HCD-local setup packet */
 #define URB_DMA_SG_COMBINED	0x00400000	/* S-G entries were combined */
 #define URB_ALIGNED_TEMP_BUFFER	0x00800000	/* Temp buffer was alloc'd */
+#define URB_ANCHOR_PERMANENT	0x01000000	/* Keep anchored across callback */
 
 struct usb_iso_packet_descriptor {
 	unsigned int offset;
@@ -1732,6 +1733,8 @@ extern void usb_anchor_suspend_wakeups(struct usb_anchor *anchor);
 extern void usb_anchor_resume_wakeups(struct usb_anchor *anchor);
 extern void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor);
 extern void usb_unanchor_urb(struct urb *urb);
+extern void usb_moor_urb(struct urb *urb, struct usb_anchor *anchor);
+extern void usb_unmoor_urb(struct urb *urb);
 extern int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor,
 					 unsigned int timeout);
 extern struct urb *usb_get_from_anchor(struct usb_anchor *anchor);
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs
  2020-07-27 21:29         ` Oliver Neukum
@ 2020-07-28  9:47           ` Eli Billauer
  2020-07-28 13:42             ` Alan Stern
  0 siblings, 1 reply; 12+ messages in thread
From: Eli Billauer @ 2020-07-28  9:47 UTC (permalink / raw)
  To: Oliver Neukum; +Cc: Alan Stern, gregkh, linux-usb, hdegoede

Oliver, Alan,

I understand that there's a disagreement in what is allowed or not with 
the anchor API. Effectively that means that I have to assume that driver 
programmers will go either way. I have to admit that my view was that a 
driver must proactively make sure it doesn't submit further URBs to an 
anchor as long as usb_kill_anchored_urbs() runs, through completers or 
otherwise. I formed the current patch accordingly.

To make things trickier, a driver might rely (correctly or not) on that 
usb_kill_urb() makes sure that resubmission of a URB by the completer, 
while usb_kill_urb() is killing it, will fail. Or at least so says the 
description of this function.

And once again, the resubmitted URB will remain untouched if the said 
race condition occurs. So a driver's programmer, who relied on 
usb_kill_urb() to prevent the resubmission, might get the impression 
that he did correctly when testing the driver, but then the kernel panic 
will happen rarely and far from the eye.

Writing an additional API without this problem is beyond the scope of 
this discussion. I'm focused on resolving the problem of the current 
one. The existing API must be safe to use, even if it's planned to phase 
out.

Given the discussion so far, I realized that the resubmission by 
completer case must be handled properly as well. So I suggest modifying 
the patch to something like

do {
     spin_lock_irq(&anchor->lock);
     while (!list_empty(&anchor->urb_list)) {
         /* URB kill loop */
     }
     spin_unlock_irq(&anchor->lock);
} while (unlikely(!usb_anchor_check_wakeup(anchor)));

The do-while loop will almost never make any difference. But it will 
loop like a waiting spinlock in the rare event of the said race 
condition, while the completer callback executes.

And if the completer submitted a URB, it will be removed as well this 
way. Recall that this loops only in the event of a race condition, so it 
will NOT play cat-and-mouse with the completer callback, but rather 
finish this up rather quickly.

And I've dropped the WARN(): If some people consider resubmission of a 
URB to be OK, even while usb_kill_anchored_urbs() is called, no noise 
should be made if that causes a rare but tricky situation.

And since I'm at it, I'll make the same change to 
usb_poison_anchored_urbs(), which suffers from exactly the same problem.

What do you think?

Thanks,
    Eli

On 28/07/20 00:29, Oliver Neukum wrote:
> Am Montag, den 27.07.2020, 10:43 -0400 schrieb Alan Stern:
>    
>> On Mon, Jul 27, 2020 at 03:58:05PM +0200, Oliver Neukum wrote:
>>      
>>> Am Montag, den 27.07.2020, 14:27 +0300 schrieb Eli Billauer:
>>>        
>>>> Hello, Oliver.
>>>>
>>>> On 27/07/20 13:14, Oliver Neukum wrote:
>>>>          
>>>>> That however is really a kludge we cannot have in usbcore.
>>>>> I am afraid as is the patch should_not_  be applied.
>>>>>
>>>>>            
>>>> Could you please explain further why the suggested patch is unsuitable?
>>>>          
>>> Hi,
>>>
>>> certainly.
>>>
>>> 1. timeouts are generally a bad idea, especially if the timeout does
>>> not come out of a spec.
>>>
>>> 2. That involves quoting you:
>>>
>>> Alternatively, if the driver submits URBs to the same anchor while
>>> usb_kill_anchored_urbs() is called, this timeout might be reached. This
>>>        
>> That would be a bug in the driver, though.  In such a situation, a WARN
>> is worth having.
>>      
> Well, it is an inherent race, certainly. You can do it, though. It is
> debatable whether it would ever make sense. Yet it is not a bug in the
> sense of, for example, writing beyond the end of a buffer or submitting
> an URB twofold.
>
>    
>>> could happen, for example, if the completer function that ran in the
>>> racy situation resubmits the URB. If that situation isn't cleared within
>>> 1000ms, it means that there's a URB in the system that the driver isn't
>>> aware of. Maybe that situation is worth more than a WARN.
>>>
>>> That is an entirely valid use case. And a bulk URB may take a potentially
>>> unbounded time to complete.
>>>        
>> It is _not_ a valid use case.  Since usb_kill_anchored_urbs() doesn't'
>> specify whether it will kill URBs that are added to the anchor after it
>> is called (and before it returns), a driver that anchors URBs at such a
>> time is buggy.
>>      
> Yes, if you depend on it. Here we are getting into technicalities.
> The thing is that we are getting into areas where we should not need to
> go if the API were optimal.
>
> What drivers really want is a way to say, kill this group of URBs and
> make sure they stay dead no matter what.
>
>    
>> Maybe this should be mentioned in the kerneldoc for the routine: Drivers
>> must not add URBs to the anchor while the routine is running.
>>      
> True, yet this defeats one of the aims of the API.
>
>    
>>> My failure in this case is simply overengineering.
>>> If this line:
>>>
>>>          usb_unanchor_urb(urb);
>>>
>>> In __usb_hcd_giveback_urb(struct urb *urb) weren't there, the issue
>>> would not exist. I misdesigned the API in automatically unanchoring
>>> a completing URB.
>>> Simply removing it now is no longer possible, so we need to come up with
>>> a more complex solution.
>>>        
>> Given that this timeout-based API is already present and being used in a
>> separate context, I don't see anything wrong with using it here as well.
>>
>>      
> It is unnecessary and results in a much less useful API.
> The true error in its design is that it unconditionally unanchors the
> URBs it gives back. Stop doing that and it becomes much better.
>
> 	Regards
> 		Oliver
>
>
>    


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs
  2020-07-28  9:44         ` Oliver Neukum
@ 2020-07-28 13:39           ` Alan Stern
  0 siblings, 0 replies; 12+ messages in thread
From: Alan Stern @ 2020-07-28 13:39 UTC (permalink / raw)
  To: Oliver Neukum; +Cc: Eli Billauer, gregkh, linux-usb, hdegoede

On Tue, Jul 28, 2020 at 11:44:48AM +0200, Oliver Neukum wrote:
> From c37e910758b0a05d4c6b8d058974b1264f9d0aef Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> Date: Tue, 28 Jul 2020 11:38:23 +0200
> Subject: [PATCH] USB: add mooring API
> 
> This is a simplified and thereby better version of the anchor API.
> Anchors have the problem that they unanchor an URB upon giveback,
> which creates a window during which an URB is unanchored but not
> yet returned, leading to operations on anchors not having the
> semantics many driver errornously assume them to have.
> The new API keeps an URB on an anchor until it is explicitly
> unmoored.

So you will require the completion handler to unmoor its URBs.  The idea 
seems sound enough, although the patch itself contains a couple of small 
errors (see below).  The real problem is that any drivers relying on 
usb_kill_anchored_urbs will still have to be fixed up by hand.

Also, it's not a good idea to store the new flag in urb->transfer_flags.  
Accessing bitflags without proper locking isn't SMP-safe.  Maybe you can 
use some of the bits in urb->reject instead.

> Signed-off-by: Oliver Neukum <oneukum@suse.de>
> ---
>  drivers/usb/core/hcd.c |  3 ++-
>  drivers/usb/core/urb.c | 27 ++++++++++++++++++++++++++-
>  include/linux/usb.h    |  3 +++
>  3 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index a33b849e8beb..861d30180709 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1640,7 +1640,8 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>  	unmap_urb_for_dma(hcd, urb);
>  	usbmon_urb_complete(&hcd->self, urb, status);
>  	usb_anchor_suspend_wakeups(anchor);
> -	usb_unanchor_urb(urb);
> +	if (!urb->transfer_flags && URB_ANCHOR_PERMANENT)

Missing parens, and && instead of &.

> +		usb_unanchor_urb(urb);
>  	if (likely(status == 0))
>  		usb_led_activity(USB_LED_EVENT_HOST);
>  
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 7bc23469f4e4..1acfbd4e6323 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(usb_get_urb);
>   * This can be called to have access to URBs which are to be executed
>   * without bothering to track them
>   */
> -void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
> +static void __usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
>  {
>  	unsigned long flags;
>  
> @@ -137,8 +137,20 @@ void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
>  
>  	spin_unlock_irqrestore(&anchor->lock, flags);
>  }
> +
> +void inline usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)

Shouldn't be marked inline.

> +{
> +	__usb_anchor_urb(urb, anchor);
> +}
>  EXPORT_SYMBOL_GPL(usb_anchor_urb);
>  
> +void usb_moor_urb(struct urb *urb, struct usb_anchor *anchor)
> +{
> +	urb->transfer_flags |= URB_ANCHOR_PERMANENT;
> +	__usb_anchor_urb( urb, anchor);

Extra space character.

> +}
> +EXPORT_SYMBOL_GPL(usb_moor_urb);
> +
>  static int usb_anchor_check_wakeup(struct usb_anchor *anchor)
>  {
>  	return atomic_read(&anchor->suspend_wakeups) == 0 &&
> @@ -185,6 +197,19 @@ void usb_unanchor_urb(struct urb *urb)
>  }
>  EXPORT_SYMBOL_GPL(usb_unanchor_urb);
>  
> +void usb_unmoor_urb(struct urb *urb)
> +{
> +	struct usb_anchor *anchor;
> +
> +	anchor = urb->anchor;
> +	if (!anchor)
> +		return;
> +
> +	__usb_unanchor_urb(urb, anchor);
> +	urb->transfer_flags &= ~URB_ANCHOR_PERMANENT;

Maybe move this line up before the check for !anchor.

Alan Stern

> +}
> +EXPORT_SYMBOL_GPL(usb_unmoor_urb);
> +
>  /*-------------------------------------------------------------------*/
>  
>  static const int pipetypes[4] = {
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 20c555db4621..cacebdb01bfd 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1347,6 +1347,7 @@ extern int usb_disabled(void);
>  #define URB_SETUP_MAP_LOCAL	0x00200000	/* HCD-local setup packet */
>  #define URB_DMA_SG_COMBINED	0x00400000	/* S-G entries were combined */
>  #define URB_ALIGNED_TEMP_BUFFER	0x00800000	/* Temp buffer was alloc'd */
> +#define URB_ANCHOR_PERMANENT	0x01000000	/* Keep anchored across callback */
>  
>  struct usb_iso_packet_descriptor {
>  	unsigned int offset;
> @@ -1732,6 +1733,8 @@ extern void usb_anchor_suspend_wakeups(struct usb_anchor *anchor);
>  extern void usb_anchor_resume_wakeups(struct usb_anchor *anchor);
>  extern void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor);
>  extern void usb_unanchor_urb(struct urb *urb);
> +extern void usb_moor_urb(struct urb *urb, struct usb_anchor *anchor);
> +extern void usb_unmoor_urb(struct urb *urb);
>  extern int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor,
>  					 unsigned int timeout);
>  extern struct urb *usb_get_from_anchor(struct usb_anchor *anchor);
> -- 
> 2.16.4
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs
  2020-07-28  9:47           ` Eli Billauer
@ 2020-07-28 13:42             ` Alan Stern
  0 siblings, 0 replies; 12+ messages in thread
From: Alan Stern @ 2020-07-28 13:42 UTC (permalink / raw)
  To: Eli Billauer; +Cc: Oliver Neukum, gregkh, linux-usb, hdegoede

On Tue, Jul 28, 2020 at 12:47:30PM +0300, Eli Billauer wrote:
> Oliver, Alan,
> 
> I understand that there's a disagreement in what is allowed or not with the
> anchor API. Effectively that means that I have to assume that driver
> programmers will go either way. I have to admit that my view was that a
> driver must proactively make sure it doesn't submit further URBs to an
> anchor as long as usb_kill_anchored_urbs() runs, through completers or
> otherwise. I formed the current patch accordingly.
> 
> To make things trickier, a driver might rely (correctly or not) on that
> usb_kill_urb() makes sure that resubmission of a URB by the completer, while
> usb_kill_urb() is killing it, will fail. Or at least so says the description
> of this function.
> 
> And once again, the resubmitted URB will remain untouched if the said race
> condition occurs. So a driver's programmer, who relied on usb_kill_urb() to
> prevent the resubmission, might get the impression that he did correctly
> when testing the driver, but then the kernel panic will happen rarely and
> far from the eye.
> 
> Writing an additional API without this problem is beyond the scope of this
> discussion. I'm focused on resolving the problem of the current one. The
> existing API must be safe to use, even if it's planned to phase out.
> 
> Given the discussion so far, I realized that the resubmission by completer
> case must be handled properly as well. So I suggest modifying the patch to
> something like
> 
> do {
>     spin_lock_irq(&anchor->lock);
>     while (!list_empty(&anchor->urb_list)) {
>         /* URB kill loop */
>     }
>     spin_unlock_irq(&anchor->lock);
> } while (unlikely(!usb_anchor_check_wakeup(anchor)));
> 
> The do-while loop will almost never make any difference. But it will loop
> like a waiting spinlock in the rare event of the said race condition, while
> the completer callback executes.
> 
> And if the completer submitted a URB, it will be removed as well this way.
> Recall that this loops only in the event of a race condition, so it will NOT
> play cat-and-mouse with the completer callback, but rather finish this up
> rather quickly.
> 
> And I've dropped the WARN(): If some people consider resubmission of a URB
> to be OK, even while usb_kill_anchored_urbs() is called, no noise should be
> made if that causes a rare but tricky situation.
> 
> And since I'm at it, I'll make the same change to
> usb_poison_anchored_urbs(), which suffers from exactly the same problem.
> 
> What do you think?

This seems like a good way to fix up the existing API, until drivers can 
be converted over to Oliver's "mooring" scheme.  Of course, Oliver might 
not agree...

Alan Stern

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-07-28 13:42 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-27  7:22 [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs eli.billauer
2020-07-27  9:21 ` Greg KH
2020-07-27 11:26   ` Eli Billauer
2020-07-27 10:14 ` Oliver Neukum
2020-07-27 11:27   ` Eli Billauer
2020-07-27 13:58     ` Oliver Neukum
2020-07-27 14:43       ` Alan Stern
2020-07-27 21:29         ` Oliver Neukum
2020-07-28  9:47           ` Eli Billauer
2020-07-28 13:42             ` Alan Stern
2020-07-28  9:44         ` Oliver Neukum
2020-07-28 13:39           ` Alan Stern

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).