On Thu, Apr 26, 2012 at 1:02 PM, Ming Lei <tom.leiming@xxxxxxxxx> wrote:
> Hi Huajun,
>
> 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.
>>
>> To easily reproduce it, in unlink_urbs(), you can delay a short time
>> after usb_put_urb(urb), then disconnect your device while transferring
>> data, and repeat it times you will find errors on your screen.
>>
>> Following is a draft patch to guarantee the queue consistent, and
>> refer to expected skb in each loop cycle:
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index b7b3f5b..6da0141 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -578,16 +578,28 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq);
>> static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
>> {
>> unsigned long flags;
>> - struct sk_buff *skb, *skbnext;
>> + struct sk_buff *skb;
>> int count = 0;
>>
>> spin_lock_irqsave (&q->lock, flags);
>> - skb_queue_walk_safe(q, skb, skbnext) {
>> + while (!skb_queue_empty(q)) {
>> struct skb_data *entry;
>> struct urb *urb;
>> int retval;
>>
>> - entry = (struct skb_data *) skb->cb;
>> + skb_queue_walk(q, skb) {
>> + entry = (struct skb_data *)skb->cb;
>> + if (entry->state == rx_done ||
>> + entry->state == tx_done ||
>> + entry->state == rx_cleanup)
>> + continue;
>> + else
>> + break;
>> + }
>> +
>> + if (skb == (struct sk_buff *)(q))
>> + break;
>> +
>> urb = entry->urb;
>>
>>
>
> After thinking about the issue further, the basic idea of your patch is good,
> but not safe(double unlink, also races on accessing entry->state), so I write
> a new one based on your patch.
>
> Are you OK this new one?
>
Yes, no more issue that I can see by now, thanks.
> Thanks,
> --
> Ming Lei
>
> From 89eb41968caa9523c90c1cf7b7e2259db00e04b8 Mon Sep 17 00:00:00 2001
> From: Ming Lei <tom.leiming@xxxxxxxxx>
> Date: Thu, 26 Apr 2012 11:33:46 +0800
> Subject: [PATCH] usbnet: fix skb traversing races during unlink
>
> Commit 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d(net/usbnet: avoid
> recursive locking in usbnet_stop()) fixes the recursive locking
> problem by releasing the skb queue lock before unlink, but may
> cause skb traversing races:
> - after URB is unlinked and the queue lock is released,
> the refered skb and skb->next may be moved to done queue,
> even be released
> - in skb_queue_walk_safe, the next skb is still obtained
> by next pointer of the last skb
> - so maybe trigger oops or other problems
>
> This patch extends the usage of entry->state to describe 'start_unlink'
> state, so always holding the queue(rx/tx) lock to change the state if
> the referd skb is in rx or tx queue because we need to know if the
> refered urb has been started unlinking in unlink_urbs.
>
> Also the patch uses usb_block_urb introduced by Oliver to block
> URB resubmit in complete handler if the URB will be unlinked.
>
> The other part of this patch is based on Hugjun's patch:
s/Hugjun/Huajun
> always traverse from head of the tx/rx queue to get skb which is
> to be unlinked but not been started unlinking.
> ---
--
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
