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.
>
Hi Ming,
That's why my patch uses skb_queue_walk() to traverse the queue,
it guarantees the skb available in each loop. Is this what you
expected?
The main idea of my patch(it is based on current mainline: 3.4.0-rc3) is:
1. If the skb in txq/rxq, then it must be available,
unlink_urbs() can refer to it safely while it holds q->lock;
2. If the skb in txq/rxq and its state is
rx_done/tx_done/rx_cleanup, that means the skb's URB is complete, then
don't need to unlink it again;
3. Before releasing q->lock in unlink_urbs(), it will increase
the URB's refercount, so even the related skb is freed in future, the
URB is still available.
Thanks,
--Huajun
> 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?
Just skip trying this per your following email's comments.
>
> 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
--
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
