On Sat, Apr 21, 2012 at 9:49 AM, Ming Lei <tom.leiming@xxxxxxxxx> wrote: > On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li <huajun.li.lee@xxxxxxxxx> wrote: >> On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming@xxxxxxxxx> wrote: >>> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@xxxxxxxxx> wrote: >>>> >>>> Above patch has already been integrated to mainline. However, maybe >>>> there still exists another potentail use-after-free issue, here is a >>>> case: >>>> After release the lock in unlink_urbs(), defer_bh() may move >>>> current skb from rxq/txq to dev->done queue, even cause the skb be >>>> released. Then in next loop cycle, it can't refer to expected skb, and >>>> may Oops again. >>> >>> Could you explain in a bit detail? Why can't the expected skb be refered >>> to in next loop? >> >> >> unlink_urbs() complete handler >> -------------------------------------- >> ------------------------------------------------- >> spin_unlock_irqrestore() >> rx_complete() >> derver_bh() >> >> __skb_unlink() >> >> __skb_queue_tail(&dev->done, skb) =======> skb is moved to >> dev->done, and can be freed by usbnet_bh() >> skb_queue_walk_safe() >> tmp = skb->next ===> refer to freed skb > > I see the problem, so looks skb_queue_walk_safe is not safe. > I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe > is needed and it may become unsafe if skb is freed during current loop. > > But removing the 2nd 'tmp = skb->next' doesn't help the problem, because > tmp still may become freed after releasing lock. > >> If its state is x_done/tx_done/rx_cleanup, that means the the skb will >> be released soon, right? If so, it should avoid calling >> usb_unlink_urb(). > > Even though you can avoid calling unlink for completed URBs, the skbs > still may be freed in unlink path because complete handler will be triggered > by unlink and the referenced skb may be freed before next loop, so your > patch can't fix the oops. > > As far as I can think of, we can hold lock of done queue to forbid skb free > during unlinking. The below patch may fix the problem, are you OK > with it? Sorry, the patch still doesn't work since done.lock can't be held before calling unlinking. One simple solution is just always holding q->lock during the whole loop, like before commit 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d (net/usbnet: avoid recursive locking in usbnet_stop()), and take the per cpu flag trick to avoid holding q->lock in complete handler called by unlink, see idea in below link: http://marc.info/?l=linux-usb&m=133491718707499&w=2 In theory, it should be simple, just take avoiding policy. > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index db99536..a9809d4 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct > sk_buff_head *q) > struct sk_buff *skb, *skbnext; > int count = 0; > > - spin_lock_irqsave (&q->lock, flags); > + spin_lock_irqsave(&dev->done.lock, flags); > + spin_lock(&q->lock); > skb_queue_walk_safe(q, skb, skbnext) { > struct skb_data *entry; > struct urb *urb; > @@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, struct > sk_buff_head *q) > * handler(include defer_bh). > */ > usb_get_urb(urb); > - spin_unlock_irqrestore(&q->lock, flags); > + spin_unlock(&q->lock); > // during some PM-driven resume scenarios, > // these (async) unlinks complete immediately > retval = usb_unlink_urb (urb); > @@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev, > struct sk_buff_head *q) > else > count++; > usb_put_urb(urb); > - spin_lock_irqsave(&q->lock, flags); > + spin_lock(&q->lock); > } > - spin_unlock_irqrestore (&q->lock, flags); > + spin_unlock(&q->lock); > + spin_unlock_irqrestore(&dev->done.lock, flags); > return count; > } > > > > Thanks, > -- > Ming Lei -- Ming Lei -- 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
