From patchwork Sat Apr 21 09:40:16 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Ming Lei X-Patchwork-Id: 154205 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 0089EB6FBD for ; Sat, 21 Apr 2012 19:40:23 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754974Ab2DUJkT (ORCPT ); Sat, 21 Apr 2012 05:40:19 -0400 Received: from mail-pz0-f52.google.com ([209.85.210.52]:36802 "EHLO mail-pz0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754611Ab2DUJkR convert rfc822-to-8bit (ORCPT ); Sat, 21 Apr 2012 05:40:17 -0400 Received: by dake40 with SMTP id e40so13992607dak.11 for ; Sat, 21 Apr 2012 02:40:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=kX8llWI/0XhVAVhAzoH4tA5odfeA9IZtp8eDzkF2ahs=; b=RTA6HF+g5Ii/Umwx2FJsNzN7pK/1dVwU4s0ybPXqDofacK4nw5RdzAdiqd8A2oAP/L 2cBucCl2HnzOq6UPL6+QfcFHJXzir2FV5nGJxM6lh5rEKKJDBzO5RPjMHCE90yv5MBFx OvyW0dOfSlBp3S0hbo95rKjqyANl+pNu8QwK6PgzHwHZnJrmd3VvCvQOL0URtiwdF+2e DMc83dUeVP+F4Sjw4NVbEMqtnVkOHXq+R2I7C9dXoMrpxDe456AJl54anaWJ8qeWCx0v tdN5cTSaeQK8ZbJhLtGMrAoCmoxS/UZrzzS0lIf/ynx2f79F6bBJF/OHa4NXmf7MPktM rLXw== MIME-Version: 1.0 Received: by 10.68.201.73 with SMTP id jy9mr19685996pbc.35.1335001216593; Sat, 21 Apr 2012 02:40:16 -0700 (PDT) Received: by 10.142.212.17 with HTTP; Sat, 21 Apr 2012 02:40:16 -0700 (PDT) In-Reply-To: References: <201203221008.46882.oneukum@suse.de> Date: Sat, 21 Apr 2012 17:40:16 +0800 Message-ID: Subject: Re: use-after-free in usbnet From: Ming Lei To: Huajun Li Cc: Oliver Neukum , Alan Stern , Dave Jones , netdev@vger.kernel.org, linux-usb@vger.kernel.org, Fedora Kernel Team Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Hi Huajun, On Sat, Apr 21, 2012 at 4:23 PM, Huajun Li wrote: > On Sat, Apr 21, 2012 at 3:56 PM, Ming Lei wrote: >> Hi Huajun, >> >> On Sat, Apr 21, 2012 at 3:50 PM, Huajun Li wrote: >> >>> Did we on the same page, could you please review my patch again? >>> >>> My draft patch was based on current mainline( 3.4.0-rc3)  which had >>> already integrated your previous patch. And in my patch, it replaced >>> skb_queue_walk_safe() with skb_queue_walk(), so you will not see  'tmp >>> = skb->next'  any more. >> >> Replace skb_queue_walk_safe with skb_queue_walk doesn't improve >> the problem, since 'skb = skb->next' in skb_queue_walk still may trigger >> the oops, does it? >> > > No. > In each loop, my patch traverse the queue from its head, and it always > holds  q->lock when it need refer "skb->next", this can make sure the > right skb is not moved out of rxq/txq. OK, your patch can avoid the oops, sorry for miss the point. > > Can this fix what you concern? If so, IMO, there is no need to revert > your previous patch. But your patch may introduce another problem, in fact, what your patch does is basically same with the below change[1]: So we can find easily that one same URB may be unlinked more than one time with your patch because usb_unlink_urb is asynchronous even though it behaves synchronously sometimes. I remembered that is not allowed, at least usb_unlink_urb's comment says so: URBs complete only once per submission, and may be canceled only once per submission. [1], against 3.4.0-rc3 Thanks, diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index db99536..aadf009 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -578,15 +578,19 @@ 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 (1) { struct skb_data *entry; struct urb *urb; int retval; + skb = q->next; + if (skb == (struct sk_buff *)q) + break; + entry = (struct skb_data *) skb->cb; urb = entry->urb;