| Message ID | 20200727072225.25195-1-eli.billauer@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: core: Solve race condition in usb_kill_anchored_urbs | expand |
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
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
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 > >
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
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
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
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
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);
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
>
>
>
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 >
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
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);