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