From patchwork Tue Nov 29 20:43:57 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Eric Dumazet X-Patchwork-Id: 128341 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 50BA6B6EE8 for ; Wed, 30 Nov 2011 07:44:12 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756429Ab1K2UoG (ORCPT ); Tue, 29 Nov 2011 15:44:06 -0500 Received: from mail-ey0-f174.google.com ([209.85.215.174]:55181 "EHLO mail-ey0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755822Ab1K2UoD (ORCPT ); Tue, 29 Nov 2011 15:44:03 -0500 Received: by eaak14 with SMTP id k14so3598743eaa.19 for ; Tue, 29 Nov 2011 12:44:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=message-id:subject:from:to:cc:date:in-reply-to:references :content-type:x-mailer:content-transfer-encoding:mime-version; bh=VGMRIKiZS0kcWAh/WZW7NCF9pxZ0/MJJ88vWPaeDWX0=; b=Z5iwslcwNaCXMroj4NJmIItcyLlIvAHgQNEvljXl2oRIEHf4AGh5pwhPKXjch0AYiF KDJxFi4P3gWC4hhVmovQUpM/AqqlAQEdIri3h8q1R7FAwO/AIywsKZpirIHE0BOue6Z1 o7+vtdys7oUVEixthCxgaQKCrh+7UcwO1KGLM= Received: by 10.213.7.68 with SMTP id c4mr1447195ebc.131.1322599441852; Tue, 29 Nov 2011 12:44:01 -0800 (PST) Received: from [10.170.237.3] ([87.255.129.107]) by mx.google.com with ESMTPS id 8sm40913437ees.2.2011.11.29.12.43.59 (version=SSLv3 cipher=OTHER); Tue, 29 Nov 2011 12:44:00 -0800 (PST) Message-ID: <1322599437.2596.10.camel@edumazet-laptop> Subject: Re: RCU'ed dst_get_neighbour() From: Eric Dumazet To: Marc Aurele La France , David Miller Cc: netdev@vger.kernel.org, Roland Dreier , linux-rdma@vger.kernel.org Date: Tue, 29 Nov 2011 21:43:57 +0100 In-Reply-To: <1322589661.2596.2.camel@edumazet-laptop> References: <1322589661.2596.2.camel@edumazet-laptop> X-Mailer: Evolution 3.2.1- Mime-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Le mardi 29 novembre 2011 à 19:01 +0100, Eric Dumazet a écrit : > Le mardi 29 novembre 2011 à 10:44 -0700, Marc Aurele La France a écrit : > > Hi. > > > > Commit (1) seems to imply that all dst_get_neighbour() references now need > > to be wrapped with rcu_read_lock()/rcu_read_unlock() sequences. See (2) > > for one such proposed change. > > > > In the case I have here (ipoib), this commit results in ... > > > > =================================================== > > [ INFO: suspicious rcu_dereference_check() usage. ] > > --------------------------------------------------- > > include/net/dst.h:91 invoked rcu_dereference_check() without protection! > > > > other info that might help us debug this: > > > > > > rcu_scheduler_active = 1, debug_locks = 1 > > 4 locks held by kworker/3:1/630: > > #0: (ib_cm){.+.+.+}, at: [] > > process_one_work+0x1ab/0x4f9 > > #1: ((&(&work->work)->work)){+.+.+.}, at: [] > > process_one_work+0x1ab/0x4f9 > > #2: (rcu_read_lock_bh){.+....}, at: [] > > dev_queue_xmit+0x0/0x5ae > > #3: (_xmit_INFINIBAND){+.-...}, at: [] > > sch_direct_xmit+0x4d/0x22b > > > > stack backtrace: > > Pid: 630, comm: kworker/3:1 Not tainted 3.1.3-smp #1 > > Call Trace: > > [] lockdep_rcu_dereference+0x9b/0xa4 > > [] ipoib_start_xmit+0xf4/0x36f > > [] dev_hard_start_xmit+0x2a7/0x54f > > [] sch_direct_xmit+0x70/0x22b > > [] dev_queue_xmit+0x309/0x5ae > > [] ? napi_gro_receive+0xb3/0xb3 > > [] ipoib_cm_rep_handler+0x208/0x248 > > [] ? _raw_spin_unlock_irqrestore+0x3d/0x5b > > [] ipoib_cm_tx_handler+0x95/0x27f > > [] ? __trace_hardirqs_on_caller+0x41/0x65 > > [] cm_process_work+0x26/0xbc > > [] cm_rep_handler+0x274/0x2ae > > [] cm_work_handler+0x41/0x91 > > [] process_one_work+0x2a2/0x4f9 > > [] ? process_one_work+0x1ab/0x4f9 > > [] ? worker_thread+0x4a/0x1ca > > [] ? cm_req_handler+0x355/0x355 > > [] worker_thread+0xf9/0x1ca > > [] ? gcwq_mayday_timeout+0x77/0x77 > > [] kthread+0x86/0x8e > > [] kernel_thread_helper+0x4/0x10 > > [] ? retint_restore_args+0xe/0xe > > [] ? kthread_stop+0x1cd/0x1cd > > [] ? gs_change+0xb/0xb > > > > =================================================== > > [ INFO: suspicious rcu_dereference_check() usage. ] > > --------------------------------------------------- > > include/net/dst.h:91 invoked rcu_dereference_check() without protection! > > > > other info that might help us debug this: > > > > > > rcu_scheduler_active = 1, debug_locks = 1 > > 2 locks held by kworker/u:2/748: > > #0: ((name)){.+.+.+}, at: [] > > process_one_work+0x1ab/0x4f9 > > #1: ((&port_priv->work)){+.+.+.}, at: [] > > process_one_work+0x1ab/0x4f9 > > > > stack backtrace: > > Pid: 748, comm: kworker/u:2 Not tainted 3.1.3-smp #1 > > Call Trace: > > [] lockdep_rcu_dereference+0x9b/0xa4 > > [] ipoib_mcast_join_finish+0x362/0x48a > > [] ipoib_mcast_sendonly_join_complete+0x3b/0x174 > > [] mcast_work_handler+0xba/0x182 > > [] join_handler+0xe6/0xee > > [] ib_sa_mcmember_rec_callback+0x51/0x5c > > [] recv_handler+0x44/0x50 > > [] ib_mad_complete_recv+0xc3/0x125 > > [] ? find_mad_agent+0x13a/0x149 > > [] ib_mad_recv_done_handler+0x2de/0x326 > > [] ib_mad_completion_handler+0x5e/0x91 > > [] process_one_work+0x2a2/0x4f9 > > [] ? process_one_work+0x1ab/0x4f9 > > [] ? worker_thread+0x4a/0x1ca > > [] ? ib_mad_recv_done_handler+0x326/0x326 > > [] worker_thread+0xf9/0x1ca > > [] ? gcwq_mayday_timeout+0x77/0x77 > > [] kthread+0x86/0x8e > > [] kernel_thread_helper+0x4/0x10 > > [] ? retint_restore_args+0xe/0xe > > [] ? kthread_stop+0x1cd/0x1cd > > [] ? gs_change+0xb/0xb > > > > Comments/flames? > > > > Thanks. > > > > Marc. > > > > PS: Please reply-to-all as I am not subscribed to netdev. > > > > (1) http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=f2c31e32b378a6653f8de606149d963baf11d7d3 > > (2) http://www.spinics.net/lists/netdev/msg179639.html > > > > +----------------------------------+----------------------------------+ > > | Marc Aurele La France | work: 1-780-492-9310 | > > | Academic Information and | fax: 1-780-492-1729 | > > | Communications Technologies | email: tsi@ualberta.ca | > > | 352 General Services Building +----------------------------------+ > > | University of Alberta | | > > | Edmonton, Alberta | Standard disclaimers apply | > > | T6G 2H1 | | > > | CANADA | | > > +----------------------------------+----------------------------------+ > > Thanks for the report Marc, I'll take a look asap. > > Hi Marc Here is the patch I cooked based on your report, thanks a lot ! Eric [PATCH] net: infiniband/ulp/ipoib: fix lockdep splats commit f2c31e32b37 (net: fix NULL dereferences in check_peer_redir()) forgot to take care of infiniband uses of dst neighbours. Many thanks to Marc Aurele who provided a nice bug report. Reported-by: Marc Aurele La France Signed-off-by: Eric Dumazet CC: David Miller CC: Roland Dreier --- drivers/infiniband/ulp/ipoib/ipoib_main.c | 17 ++++++++------- drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 6 +++-- 2 files changed, 14 insertions(+), 9 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c index 7567b60..c36a2ab 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c @@ -555,6 +555,7 @@ static int path_rec_start(struct net_device *dev, return 0; } +/* called with rcu_read_lock */ static void neigh_add_path(struct sk_buff *skb, struct net_device *dev) { struct ipoib_dev_priv *priv = netdev_priv(dev); @@ -636,6 +637,7 @@ err_drop: spin_unlock_irqrestore(&priv->lock, flags); } +/* called with rcu_read_lock */ static void ipoib_path_lookup(struct sk_buff *skb, struct net_device *dev) { struct ipoib_dev_priv *priv = netdev_priv(skb->dev); @@ -720,13 +722,14 @@ static int ipoib_start_xmit(struct sk_buff *skb, struct net_device *dev) struct neighbour *n = NULL; unsigned long flags; + rcu_read_lock(); if (likely(skb_dst(skb))) n = dst_get_neighbour(skb_dst(skb)); if (likely(n)) { if (unlikely(!*to_ipoib_neigh(n))) { ipoib_path_lookup(skb, dev); - return NETDEV_TX_OK; + goto unlock; } neigh = *to_ipoib_neigh(n); @@ -749,17 +752,17 @@ static int ipoib_start_xmit(struct sk_buff *skb, struct net_device *dev) ipoib_neigh_free(dev, neigh); spin_unlock_irqrestore(&priv->lock, flags); ipoib_path_lookup(skb, dev); - return NETDEV_TX_OK; + goto unlock; } if (ipoib_cm_get(neigh)) { if (ipoib_cm_up(neigh)) { ipoib_cm_send(dev, skb, ipoib_cm_get(neigh)); - return NETDEV_TX_OK; + goto unlock; } } else if (neigh->ah) { ipoib_send(dev, skb, neigh->ah, IPOIB_QPN(n->ha)); - return NETDEV_TX_OK; + goto unlock; } if (skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE) { @@ -793,13 +796,13 @@ static int ipoib_start_xmit(struct sk_buff *skb, struct net_device *dev) phdr->hwaddr + 4); dev_kfree_skb_any(skb); ++dev->stats.tx_dropped; - return NETDEV_TX_OK; + goto unlock; } unicast_arp_send(skb, dev, phdr); } } - +unlock: return NETDEV_TX_OK; } @@ -837,7 +840,7 @@ static int ipoib_hard_header(struct sk_buff *skb, dst = skb_dst(skb); n = NULL; if (dst) - n = dst_get_neighbour(dst); + n = dst_get_neighbour_raw(dst); if ((!dst || !n) && daddr) { struct ipoib_pseudoheader *phdr = (struct ipoib_pseudoheader *) skb_push(skb, sizeof *phdr); diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c index 1b7a976..cad1894 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c @@ -266,7 +266,7 @@ static int ipoib_mcast_join_finish(struct ipoib_mcast *mcast, skb->dev = dev; if (dst) - n = dst_get_neighbour(dst); + n = dst_get_neighbour_raw(dst); if (!dst || !n) { /* put pseudoheader back on for next time */ skb_push(skb, sizeof (struct ipoib_pseudoheader)); @@ -722,6 +722,8 @@ out: if (mcast && mcast->ah) { struct dst_entry *dst = skb_dst(skb); struct neighbour *n = NULL; + + rcu_read_lock(); if (dst) n = dst_get_neighbour(dst); if (n && !*to_ipoib_neigh(n)) { @@ -734,7 +736,7 @@ out: list_add_tail(&neigh->list, &mcast->neigh_list); } } - + rcu_read_unlock(); spin_unlock_irqrestore(&priv->lock, flags); ipoib_send(dev, skb, mcast->ah, IB_MULTICAST_QPN); return;