From patchwork Thu May 3 01:04:50 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ming Lei X-Patchwork-Id: 156587 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 C3C62B6FB9 for ; Thu, 3 May 2012 11:05:09 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754573Ab2ECBFH (ORCPT ); Wed, 2 May 2012 21:05:07 -0400 Received: from mail-pz0-f46.google.com ([209.85.210.46]:36277 "EHLO mail-pz0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754029Ab2ECBFF convert rfc822-to-8bit (ORCPT ); Wed, 2 May 2012 21:05:05 -0400 X-Greylist: delayed 1811 seconds by postgrey-1.27 at vger.kernel.org; Wed, 02 May 2012 21:05:05 EDT Received: by dady13 with SMTP id y13so736408dad.5 for ; Wed, 02 May 2012 18:05:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:in-reply-to:references :organization:x-mailer:mime-version:content-type :content-transfer-encoding; bh=W1jTw3LnR0zvuV3E/AZF2S9zPOrFUKhP7dd5Os3Ps0M=; b=BonEMq3JNS0dHYZfXc2HRpYdNSsn7e3Wf1IAkzUepj/PZn4lHs6j+zfSSnC3kjvCd0 2rywgFR+Z//SyCqINvhTLLXyShHwDkkv0+vk3E2e477MfdWkjRa/d8ZIRP4l8Hw6h8dl 3di6eWFKwHGzukhNH0yJNF+tEU90UdKa1spdFgV9bFiaWlboz7AxmVSArFBJCRjGJCUP F0YRx5Fi4Q7GtEx9UGBArHiyYQKdzkhX3oZibWCd5CTfLpF921+T/AobaH+nkfht+GFz m3NfsfgQI01kFfcYim92ngYsl+M57VhIbCS26+UBncZuQecDZA3IrOCHA8YRplWy/Edz fAnQ== Received: by 10.68.194.72 with SMTP id hu8mr2362351pbc.26.1336007103734; Wed, 02 May 2012 18:05:03 -0700 (PDT) Received: from tom-ThinkPad-T410 ([183.37.203.48]) by mx.google.com with ESMTPS id og6sm3505774pbb.42.2012.05.02.18.05.00 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 02 May 2012 18:05:03 -0700 (PDT) Date: Thu, 3 May 2012 09:04:50 +0800 From: Ming Lei To: David Miller Cc: gregkh@linuxfoundation.org, netdev@vger.kernel.org, linux-usb@vger.kernel.org, huajun.li.lee@gmail.com, oneukum@suse.de, stable@kernel.org Subject: Re: [PATCH 3/3] usbnet: fix skb traversing races during unlink(v1) Message-ID: <20120503090450.41704527@tom-ThinkPad-T410> In-Reply-To: <20120502.201212.909292764269133334.davem@davemloft.net> References: <1335775864-4873-1-git-send-email-tom.leiming@gmail.com> <1335775864-4873-4-git-send-email-tom.leiming@gmail.com> <20120502.201212.909292764269133334.davem@davemloft.net> Organization: Ming X-Mailer: Claws Mail 3.8.0 (GTK+ 2.24.10; x86_64-pc-linux-gnu) Mime-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From a87ff961f0a5d50223bd084dfac4fe5ce84f3913 Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Thu, 26 Apr 2012 11:33:46 +0800 Subject: [PATCH] usbnet: fix skb traversing races during unlink(v2) 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. The other part of this patch is based on Huajun's patch: always traverse from head of the tx/rx queue to get skb which is to be unlinked but not been started unlinking. Signed-off-by: Huajun Li Signed-off-by: Ming Lei Cc: Oliver Neukum Cc: stable@kernel.org --- v2: - if the Rx URB has been marked as being unlink, just not resubmit it in complete handler, so usb_block_urb/usb_unblock_urb can be avoided, then make backporting for -stable easier. v1: - fix comment style and avoiding to expose the internal details of SKB lists, as suggested by David Miller drivers/net/usb/usbnet.c | 54 +++++++++++++++++++++++++++++++------------- include/linux/usb/usbnet.h | 3 ++- 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 80b837c..9f58330 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -282,17 +282,32 @@ int usbnet_change_mtu (struct net_device *net, int new_mtu) } EXPORT_SYMBOL_GPL(usbnet_change_mtu); +/* The caller must hold list->lock */ +static void __usbnet_queue_skb(struct sk_buff_head *list, + struct sk_buff *newsk, enum skb_state state) +{ + struct skb_data *entry = (struct skb_data *) newsk->cb; + + __skb_queue_tail(list, newsk); + entry->state = state; +} + /*-------------------------------------------------------------------------*/ /* some LK 2.4 HCDs oopsed if we freed or resubmitted urbs from * completion callbacks. 2.5 should have fixed those bugs... */ -static void defer_bh(struct usbnet *dev, struct sk_buff *skb, struct sk_buff_head *list) +static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb, + struct sk_buff_head *list, enum skb_state state) { unsigned long flags; + enum skb_state old_state; + struct skb_data *entry = (struct skb_data *) skb->cb; spin_lock_irqsave(&list->lock, flags); + old_state = entry->state; + entry->state = state; __skb_unlink(skb, list); spin_unlock(&list->lock); spin_lock(&dev->done.lock); @@ -300,6 +315,7 @@ static void defer_bh(struct usbnet *dev, struct sk_buff *skb, struct sk_buff_hea if (dev->done.qlen == 1) tasklet_schedule(&dev->bh); spin_unlock_irqrestore(&dev->done.lock, flags); + return old_state; } /* some work can't be done in tasklets, so we use keventd @@ -340,7 +356,6 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags) entry = (struct skb_data *) skb->cb; entry->urb = urb; entry->dev = dev; - entry->state = rx_start; entry->length = 0; usb_fill_bulk_urb (urb, dev->udev, dev->in, @@ -372,7 +387,7 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags) tasklet_schedule (&dev->bh); break; case 0: - __skb_queue_tail (&dev->rxq, skb); + __usbnet_queue_skb(&dev->rxq, skb, rx_start); } } else { netif_dbg(dev, ifdown, dev->net, "rx: stopped\n"); @@ -423,16 +438,17 @@ static void rx_complete (struct urb *urb) struct skb_data *entry = (struct skb_data *) skb->cb; struct usbnet *dev = entry->dev; int urb_status = urb->status; + enum skb_state state; skb_put (skb, urb->actual_length); - entry->state = rx_done; + state = rx_done; entry->urb = NULL; switch (urb_status) { /* success */ case 0: if (skb->len < dev->net->hard_header_len) { - entry->state = rx_cleanup; + state = rx_cleanup; dev->net->stats.rx_errors++; dev->net->stats.rx_length_errors++; netif_dbg(dev, rx_err, dev->net, @@ -471,7 +487,7 @@ static void rx_complete (struct urb *urb) "rx throttle %d\n", urb_status); } block: - entry->state = rx_cleanup; + state = rx_cleanup; entry->urb = urb; urb = NULL; break; @@ -482,17 +498,18 @@ block: // FALLTHROUGH default: - entry->state = rx_cleanup; + state = rx_cleanup; dev->net->stats.rx_errors++; netif_dbg(dev, rx_err, dev->net, "rx status %d\n", urb_status); break; } - defer_bh(dev, skb, &dev->rxq); + state = defer_bh(dev, skb, &dev->rxq, state); if (urb) { if (netif_running (dev->net) && - !test_bit (EVENT_RX_HALT, &dev->flags)) { + !test_bit (EVENT_RX_HALT, &dev->flags) && + state != unlink_start) { rx_submit (dev, urb, GFP_ATOMIC); usb_mark_last_busy(dev->udev); return; @@ -579,16 +596,23 @@ 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 != unlink_start) + goto found; + } + break; +found: + entry->state = unlink_start; urb = entry->urb; /* @@ -1040,8 +1064,7 @@ static void tx_complete (struct urb *urb) } usb_autopm_put_interface_async(dev->intf); - entry->state = tx_done; - defer_bh(dev, skb, &dev->txq); + (void) defer_bh(dev, skb, &dev->txq, tx_done); } /*-------------------------------------------------------------------------*/ @@ -1097,7 +1120,6 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, entry = (struct skb_data *) skb->cb; entry->urb = urb; entry->dev = dev; - entry->state = tx_start; entry->length = length; usb_fill_bulk_urb (urb, dev->udev, dev->out, @@ -1156,7 +1178,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, break; case 0: net->trans_start = jiffies; - __skb_queue_tail (&dev->txq, skb); + __usbnet_queue_skb(&dev->txq, skb, tx_start); if (dev->txq.qlen >= TX_QLEN (dev)) netif_stop_queue (net); } diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h index 605b0aa..76f4396 100644 --- a/include/linux/usb/usbnet.h +++ b/include/linux/usb/usbnet.h @@ -191,7 +191,8 @@ extern void usbnet_cdc_status(struct usbnet *, struct urb *); enum skb_state { illegal = 0, tx_start, tx_done, - rx_start, rx_done, rx_cleanup + rx_start, rx_done, rx_cleanup, + unlink_start }; struct skb_data { /* skb->cb is one of these */