On Mon, 2 Aug 2010, Christian Lamparter wrote:
> This patch fixes a race condition in two utility routines
> related to the removal/unlinking of urbs from an anchor.
>
> If two threads are concurrently accessing the same anchor,
> both could end up with the same urb - thinking they are
> the exclusive owner.
>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <greg@xxxxxxxxx>
> Signed-off-by: Christian Lamparter <chunkeey@xxxxxxxxxxxxxx>
> ---
> v0 -> v1
> - Alan Stern's comments.
>
> v1 -> v2
> - make usb_unlink_anchored_urbs reentrant
>
> usb_kill_anchored_urbs could be easily converted in the same
> manner, but I do have my doubts with usb_poison_anchored_urb.
>
> What speaks against the modification is that these functions
> are always serialized by different means in the drivers
> anyway... and I don't even think they can be designed to be
> be completely re-entrant & atomic. That's because another
> usb_{kill,poison}_anchored_urbs (which is not serialized)
> could grab the last urb and will be waiting in the background
> until it is freed... but we won't, because we don't know
> about it!
I don't understand that last sentence. Regardless, those two routines
should be okay the way they are. They don't need to be changed because
usb_kill_urb and usb_poison_urb block until the URB completes. By
contrast, usb_unlink_urb doesn't wait.
> Greg,
> I was looking for a proper reference, whenever wake_up can
> be called with irqs disabled or not. But other than a few
> hints and code examples (kernel/slow-work.c): nothing.
You can always ask on LKML. However I think there's nothing to worry
about; wake_up is designed to work okay in atomic contexts.
These changes look fine to me. There are a few suggestions for minor
stylistic improvements below.
Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Alan Stern
> ---
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 7c05555..b1d6cd9 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -137,6 +137,15 @@ void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
> }
> EXPORT_SYMBOL_GPL(usb_anchor_urb);
>
> +static void __usb_unanchor_urb(struct urb *urb, struct usb_anchor *anchor)
How about a comment just above this line, saying that the caller must
hold anchor->lock? This may be obvious, but it won't hurt to say so.
> +{
> + urb->anchor = NULL;
> + list_del(&urb->anchor_list);
> + usb_put_urb(urb);
> + if (list_empty(&anchor->urb_list))
> + wake_up(&anchor->wait);
> +}
> +
> /**
> * usb_unanchor_urb - unanchors an URB
> * @urb: pointer to the urb to anchor
> @@ -156,17 +165,9 @@ void usb_unanchor_urb(struct urb *urb)
> return;
>
> spin_lock_irqsave(&anchor->lock, flags);
> - if (unlikely(anchor != urb->anchor)) {
> - /* we've lost the race to another thread */
> - spin_unlock_irqrestore(&anchor->lock, flags);
> - return;
> - }
> - urb->anchor = NULL;
> - list_del(&urb->anchor_list);
Also add a comment here, explaining why the extra check is needed
(since you removed the existing comment).
> + if (likely(anchor == urb->anchor))
> + __usb_unanchor_urb(urb, anchor);
> spin_unlock_irqrestore(&anchor->lock, flags);
> - usb_put_urb(urb);
> - if (list_empty(&anchor->urb_list))
> - wake_up(&anchor->wait);
> }
> EXPORT_SYMBOL_GPL(usb_unanchor_urb);
>
> @@ -749,20 +750,11 @@ EXPORT_SYMBOL_GPL(usb_unpoison_anchored_urbs);
> void usb_unlink_anchored_urbs(struct usb_anchor *anchor)
> {
> struct urb *victim;
> - unsigned long flags;
>
> - spin_lock_irqsave(&anchor->lock, flags);
> - while (!list_empty(&anchor->urb_list)) {
> - victim = list_entry(anchor->urb_list.prev, struct urb,
> - anchor_list);
> - usb_get_urb(victim);
> - spin_unlock_irqrestore(&anchor->lock, flags);
> - /* this will unanchor the URB */
> + while ((victim = usb_get_from_anchor(anchor))) {
My personal preference is to add an explicit comparison to NULL. I
just don't like seeing an assignment as the top-level operator in a
boolean expression. Of course, other people have different opinions.
> usb_unlink_urb(victim);
> usb_put_urb(victim);
> - spin_lock_irqsave(&anchor->lock, flags);
> }
> - spin_unlock_irqrestore(&anchor->lock, flags);
> }
> EXPORT_SYMBOL_GPL(usb_unlink_anchored_urbs);
>
> @@ -799,12 +791,11 @@ struct urb *usb_get_from_anchor(struct usb_anchor *anchor)
> victim = list_entry(anchor->urb_list.next, struct urb,
> anchor_list);
> usb_get_urb(victim);
> - spin_unlock_irqrestore(&anchor->lock, flags);
> - usb_unanchor_urb(victim);
> + __usb_unanchor_urb(victim, anchor);
> } else {
> - spin_unlock_irqrestore(&anchor->lock, flags);
> victim = NULL;
> }
> + spin_unlock_irqrestore(&anchor->lock, flags);
>
> return victim;
> }
> @@ -826,12 +817,7 @@ void usb_scuttle_anchored_urbs(struct usb_anchor *anchor)
> while (!list_empty(&anchor->urb_list)) {
> victim = list_entry(anchor->urb_list.prev, struct urb,
> anchor_list);
> - usb_get_urb(victim);
> - spin_unlock_irqrestore(&anchor->lock, flags);
> - /* this may free the URB */
> - usb_unanchor_urb(victim);
> - usb_put_urb(victim);
> - spin_lock_irqsave(&anchor->lock, flags);
> + __usb_unanchor_urb(victim, anchor);
> }
> spin_unlock_irqrestore(&anchor->lock, flags);
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
