Re: RCU'ed dst_get_neighbour()

Re: RCU'ed dst_get_neighbour()

[Eric Dumazet]

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: [<ffffffff81055735>] 
> > process_one_work+0x1ab/0x4f9
> >   #1:  ((&(&work->work)->work)){+.+.+.}, at: [<ffffffff81055735>] 
> > process_one_work+0x1ab/0x4f9
> >   #2:  (rcu_read_lock_bh){.+....}, at: [<ffffffff81388216>] 
> > dev_queue_xmit+0x0/0x5ae
> >   #3:  (_xmit_INFINIBAND){+.-...}, at: [<ffffffff8139eecc>] 
> > sch_direct_xmit+0x4d/0x22b
> > 
> > stack backtrace:
> > Pid: 630, comm: kworker/3:1 Not tainted 3.1.3-smp #1
> > Call Trace:
> >   [<ffffffff8106c385>] lockdep_rcu_dereference+0x9b/0xa4
> >   [<ffffffff81351cda>] ipoib_start_xmit+0xf4/0x36f
> >   [<ffffffff81384215>] dev_hard_start_xmit+0x2a7/0x54f
> >   [<ffffffff8139eeef>] sch_direct_xmit+0x70/0x22b
> >   [<ffffffff8138851f>] dev_queue_xmit+0x309/0x5ae
> >   [<ffffffff81388216>] ? napi_gro_receive+0xb3/0xb3
> >   [<ffffffff813582d3>] ipoib_cm_rep_handler+0x208/0x248
> >   [<ffffffff81433e16>] ? _raw_spin_unlock_irqrestore+0x3d/0x5b
> >   [<ffffffff8135a912>] ipoib_cm_tx_handler+0x95/0x27f
> >   [<ffffffff8106d183>] ? __trace_hardirqs_on_caller+0x41/0x65
> >   [<ffffffff81327b29>] cm_process_work+0x26/0xbc
> >   [<ffffffff81328d74>] cm_rep_handler+0x274/0x2ae
> >   [<ffffffff81329582>] cm_work_handler+0x41/0x91
> >   [<ffffffff8105582c>] process_one_work+0x2a2/0x4f9
> >   [<ffffffff81055735>] ? process_one_work+0x1ab/0x4f9
> >   [<ffffffff810580c6>] ? worker_thread+0x4a/0x1ca
> >   [<ffffffff81329541>] ? cm_req_handler+0x355/0x355
> >   [<ffffffff81058175>] worker_thread+0xf9/0x1ca
> >   [<ffffffff8105807c>] ? gcwq_mayday_timeout+0x77/0x77
> >   [<ffffffff8105bfa3>] kthread+0x86/0x8e
> >   [<ffffffff81436b34>] kernel_thread_helper+0x4/0x10
> >   [<ffffffff8143425d>] ? retint_restore_args+0xe/0xe
> >   [<ffffffff8105bf1d>] ? kthread_stop+0x1cd/0x1cd
> >   [<ffffffff81436b30>] ? 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: [<ffffffff81055735>] 
> > process_one_work+0x1ab/0x4f9
> >   #1:  ((&port_priv->work)){+.+.+.}, at: [<ffffffff81055735>] 
> > process_one_work+0x1ab/0x4f9
> > 
> > stack backtrace:
> > Pid: 748, comm: kworker/u:2 Not tainted 3.1.3-smp #1
> > Call Trace:
> >   [<ffffffff8106c385>] lockdep_rcu_dereference+0x9b/0xa4
> >   [<ffffffff81354e68>] ipoib_mcast_join_finish+0x362/0x48a
> >   [<ffffffff81355481>] ipoib_mcast_sendonly_join_complete+0x3b/0x174
> >   [<ffffffff813246b3>] mcast_work_handler+0xba/0x182
> >   [<ffffffff813248aa>] join_handler+0xe6/0xee
> >   [<ffffffff81322af1>] ib_sa_mcmember_rec_callback+0x51/0x5c
> >   [<ffffffff8132289c>] recv_handler+0x44/0x50
> >   [<ffffffff8131efca>] ib_mad_complete_recv+0xc3/0x125
> >   [<ffffffff8131debe>] ? find_mad_agent+0x13a/0x149
> >   [<ffffffff8131f30a>] ib_mad_recv_done_handler+0x2de/0x326
> >   [<ffffffff8131f3b0>] ib_mad_completion_handler+0x5e/0x91
> >   [<ffffffff8105582c>] process_one_work+0x2a2/0x4f9
> >   [<ffffffff81055735>] ? process_one_work+0x1ab/0x4f9
> >   [<ffffffff810580c6>] ? worker_thread+0x4a/0x1ca
> >   [<ffffffff8131f352>] ? ib_mad_recv_done_handler+0x326/0x326
> >   [<ffffffff81058175>] worker_thread+0xf9/0x1ca
> >   [<ffffffff8105807c>] ? gcwq_mayday_timeout+0x77/0x77
> >   [<ffffffff8105bfa3>] kthread+0x86/0x8e
> >   [<ffffffff81436b34>] kernel_thread_helper+0x4/0x10
> >   [<ffffffff8143425d>] ? retint_restore_args+0xe/0xe
> >   [<ffffffff8105bf1d>] ? kthread_stop+0x1cd/0x1cd
> >   [<ffffffff81436b30>] ? 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-yfeSBMgouQgsA/PxXw9srA@public.gmane.org         |
> > |  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 <tsi-yfeSBMgouQgsA/PxXw9srA@public.gmane.org>
Signed-off-by: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
CC: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
CC: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c      |   17 ++++++++-------
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c |    6 +++--
 2 files changed, 14 insertions(+), 9 deletions(-)

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;


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU'ed dst_get_neighbour()

[Roland Dreier]

Thanks Eric, I'll send this to Linus shortly.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU'ed dst_get_neighbour()

[Eric Dumazet]

Le mardi 29 novembre 2011 à 12:47 -0800, Roland Dreier a écrit :
> Thanks Eric, I'll send this to Linus shortly.

Oh well, I forgot one rcu_read_unlock(), I'll send a V2...



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU'ed dst_get_neighbour()

[Roland Dreier]

OK, haven't sent it on yet :)

On Tue, Nov 29, 2011 at 12:53 PM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Le mardi 29 novembre 2011 à 12:47 -0800, Roland Dreier a écrit :
>> Thanks Eric, I'll send this to Linus shortly.
>
> Oh well, I forgot one rcu_read_unlock(), I'll send a V2...
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU'ed dst_get_neighbour()

[Marc Aurele La France]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1149 bytes --]

On Tue, 29 Nov 2011, Eric Dumazet wrote:

> Le mardi 29 novembre 2011 à 12:47 -0800, Roland Dreier a écrit :
>> Thanks Eric, I'll send this to Linus shortly.

> Oh well, I forgot one rcu_read_unlock(), I'll send a V2...

This also doesn't address the other dst_get_neighbour() instances 
introduced by 
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=69cce1d1404968f78b177a0314f5822d5afdbbfb

Marc.

+----------------------------------+----------------------------------+
|  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                          |                                  |
+----------------------------------+----------------------------------+

Re: RCU'ed dst_get_neighbour()

[Eric Dumazet]

Le mardi 29 novembre 2011 à 14:00 -0700, Marc Aurele La France a écrit :
> On Tue, 29 Nov 2011, Eric Dumazet wrote:
> 
> > Le mardi 29 novembre 2011 à 12:47 -0800, Roland Dreier a écrit :
> >> Thanks Eric, I'll send this to Linus shortly.
> 
> > Oh well, I forgot one rcu_read_unlock(), I'll send a V2...
> 
> This also doesn't address the other dst_get_neighbour() instances 
> introduced by 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=69cce1d1404968f78b177a0314f5822d5afdbbfb
> 

Oh well, a complete audit is needed, and I have no choice but doing it.

Thanks !


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU'ed dst_get_neighbour()

[Eric Dumazet]

Le mardi 29 novembre 2011 à 22:17 +0100, Eric Dumazet a écrit :
> Le mardi 29 novembre 2011 à 14:00 -0700, Marc Aurele La France a écrit :
> > On Tue, 29 Nov 2011, Eric Dumazet wrote:
> > 
> > > Le mardi 29 novembre 2011 à 12:47 -0800, Roland Dreier a écrit :
> > >> Thanks Eric, I'll send this to Linus shortly.
> > 
> > > Oh well, I forgot one rcu_read_unlock(), I'll send a V2...
> > 
> > This also doesn't address the other dst_get_neighbour() instances 
> > introduced by 
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=69cce1d1404968f78b177a0314f5822d5afdbbfb
> > 
> 
> Oh well, a complete audit is needed, and I have no choice but doing it.
> 
> Thanks !
> 

Here is the result of this audit, please double check and test it, I
only compiled this.

Thanks !

[PATCH V2] drivers/infiniband: 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 and feedback.

Reported-by: Marc Aurele La France <tsi-yfeSBMgouQgsA/PxXw9srA@public.gmane.org>
Signed-off-by: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
CC: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
CC: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/addr.c                 |    9 +++++--
 drivers/infiniband/hw/cxgb3/iwch_cm.c          |    4 +++
 drivers/infiniband/hw/cxgb4/cm.c               |    6 +++++
 drivers/infiniband/hw/nes/nes_cm.c             |    6 +++--
 drivers/infiniband/ulp/ipoib/ipoib_main.c      |   18 +++++++++------
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c |    6 +++--
 6 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
index 691276b..e9cf51b 100644
--- a/drivers/infiniband/core/addr.c
+++ b/drivers/infiniband/core/addr.c
@@ -216,7 +216,9 @@ static int addr4_resolve(struct sockaddr_in *src_in,
 
 	neigh = neigh_lookup(&arp_tbl, &rt->rt_gateway, rt->dst.dev);
 	if (!neigh || !(neigh->nud_state & NUD_VALID)) {
+		rcu_read_lock();
 		neigh_event_send(dst_get_neighbour(&rt->dst), NULL);
+		rcu_read_unlock();
 		ret = -ENODATA;
 		if (neigh)
 			goto release;
@@ -274,15 +276,16 @@ static int addr6_resolve(struct sockaddr_in6 *src_in,
 		goto put;
 	}
 
+	rcu_read_lock();
 	neigh = dst_get_neighbour(dst);
 	if (!neigh || !(neigh->nud_state & NUD_VALID)) {
 		if (neigh)
 			neigh_event_send(neigh, NULL);
 		ret = -ENODATA;
-		goto put;
+	} else {
+		ret = rdma_copy_addr(addr, dst->dev, neigh->ha);
 	}
-
-	ret = rdma_copy_addr(addr, dst->dev, neigh->ha);
+	rcu_read_unlock();
 put:
 	dst_release(dst);
 	return ret;
diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.c b/drivers/infiniband/hw/cxgb3/iwch_cm.c
index de6d077..c88b12b 100644
--- a/drivers/infiniband/hw/cxgb3/iwch_cm.c
+++ b/drivers/infiniband/hw/cxgb3/iwch_cm.c
@@ -1375,8 +1375,10 @@ static int pass_accept_req(struct t3cdev *tdev, struct sk_buff *skb, void *ctx)
 		goto reject;
 	}
 	dst = &rt->dst;
+	rcu_read_lock();
 	neigh = dst_get_neighbour(dst);
 	l2t = t3_l2t_get(tdev, neigh, neigh->dev);
+	rcu_read_unlock();
 	if (!l2t) {
 		printk(KERN_ERR MOD "%s - failed to allocate l2t entry!\n",
 		       __func__);
@@ -1946,10 +1948,12 @@ int iwch_connect(struct iw_cm_id *cm_id, struct iw_cm_conn_param *conn_param)
 	}
 	ep->dst = &rt->dst;
 
+	rcu_read_lock();
 	neigh = dst_get_neighbour(ep->dst);
 
 	/* get a l2t entry */
 	ep->l2t = t3_l2t_get(ep->com.tdev, neigh, neigh->dev);
+	rcu_read_unlock();
 	if (!ep->l2t) {
 		printk(KERN_ERR MOD "%s - cannot alloc l2e.\n", __func__);
 		err = -ENOMEM;
diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index b36cdac..75b57be 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -1594,6 +1594,7 @@ static int pass_accept_req(struct c4iw_dev *dev, struct sk_buff *skb)
 		goto reject;
 	}
 	dst = &rt->dst;
+	rcu_read_lock();
 	neigh = dst_get_neighbour(dst);
 	if (neigh->dev->flags & IFF_LOOPBACK) {
 		pdev = ip_dev_find(&init_net, peer_ip);
@@ -1620,6 +1621,7 @@ static int pass_accept_req(struct c4iw_dev *dev, struct sk_buff *skb)
 		rss_qid = dev->rdev.lldi.rxq_ids[
 			  cxgb4_port_idx(neigh->dev) * step];
 	}
+	rcu_read_unlock();
 	if (!l2t) {
 		printk(KERN_ERR MOD "%s - failed to allocate l2t entry!\n",
 		       __func__);
@@ -1820,6 +1822,7 @@ static int c4iw_reconnect(struct c4iw_ep *ep)
 	}
 	ep->dst = &rt->dst;
 
+	rcu_read_lock();
 	neigh = dst_get_neighbour(ep->dst);
 
 	/* get a l2t entry */
@@ -1856,6 +1859,7 @@ static int c4iw_reconnect(struct c4iw_ep *ep)
 		ep->rss_qid = ep->com.dev->rdev.lldi.rxq_ids[
 			cxgb4_port_idx(neigh->dev) * step];
 	}
+	rcu_read_unlock();
 	if (!ep->l2t) {
 		printk(KERN_ERR MOD "%s - cannot alloc l2e.\n", __func__);
 		err = -ENOMEM;
@@ -2301,6 +2305,7 @@ int c4iw_connect(struct iw_cm_id *cm_id, struct iw_cm_conn_param *conn_param)
 	}
 	ep->dst = &rt->dst;
 
+	rcu_read_lock();
 	neigh = dst_get_neighbour(ep->dst);
 
 	/* get a l2t entry */
@@ -2339,6 +2344,7 @@ int c4iw_connect(struct iw_cm_id *cm_id, struct iw_cm_conn_param *conn_param)
 		ep->retry_with_mpa_v1 = 0;
 		ep->tried_with_mpa_v1 = 0;
 	}
+	rcu_read_unlock();
 	if (!ep->l2t) {
 		printk(KERN_ERR MOD "%s - cannot alloc l2e.\n", __func__);
 		err = -ENOMEM;
diff --git a/drivers/infiniband/hw/nes/nes_cm.c b/drivers/infiniband/hw/nes/nes_cm.c
index dfce9ea..0a52d72 100644
--- a/drivers/infiniband/hw/nes/nes_cm.c
+++ b/drivers/infiniband/hw/nes/nes_cm.c
@@ -1377,9 +1377,11 @@ static int nes_addr_resolve_neigh(struct nes_vnic *nesvnic, u32 dst_ip, int arpi
 		neigh_release(neigh);
 	}
 
-	if ((neigh == NULL) || (!(neigh->nud_state & NUD_VALID)))
+	if ((neigh == NULL) || (!(neigh->nud_state & NUD_VALID))) {
+		rcu_read_lock();
 		neigh_event_send(dst_get_neighbour(&rt->dst), NULL);
-
+		rcu_read_unlock();
+	}
 	ip_rt_put(rt);
 	return rc;
 }
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 7567b60..ef38848 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,14 @@ 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:
+	rcu_read_unlock();
 	return NETDEV_TX_OK;
 }
 
@@ -837,7 +841,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;


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU'ed dst_get_neighbour()

[Roland Dreier]

On Tue, Nov 29, 2011 at 1:31 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Here is the result of this audit, please double check and test it, I
> only compiled this.

Thanks Eric... I'll queue this up and send it on once we get a good
report from Marc.

Thanks!
  Roland

Re: RCU'ed dst_get_neighbour()

[Marc Aurele La France]

On Tue, 29 Nov 2011, Roland Dreier wrote:
> On Tue, Nov 29, 2011 at 1:31 PM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Here is the result of this audit, please double check and test it, I
>> only compiled this.

> Thanks Eric... I'll queue this up and send it on once we get a good
> report from Marc.

I can confirm that Eric's patch, retrofitted to 3.1.3, fixes the problem.

Marc.

+----------------------------------+----------------------------------+
|  Marc Aurele La France           |  work:   1-780-492-9310          |
|  Academic Information and        |  fax:    1-780-492-1729          |
|    Communications Technologies   |  email:  tsi-yfeSBMgouQgsA/PxXw9srA@public.gmane.org         |
|  352 General Services Building   +----------------------------------+
|  University of Alberta           |                                  |
|  Edmonton, Alberta               |    Standard disclaimers apply    |
|  T6G 2H1                         |                                  |
|  CANADA                          |                                  |
+----------------------------------+----------------------------------+
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU'ed dst_get_neighbour()

[Roland Dreier]

On Tue, Nov 29, 2011 at 5:17 PM, Marc Aurele La France <tsi-yfeSBMgouQgsA/PxXw9srA@public.gmane.org> wrote:
> On Tue, 29 Nov 2011, Roland Dreier wrote:
>>
>> On Tue, Nov 29, 2011 at 1:31 PM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> wrote:
>>>
>>> Here is the result of this audit, please double check and test it, I
>>> only compiled this.
>
>> Thanks Eric... I'll queue this up and send it on once we get a good
>> report from Marc.
>
> I can confirm that Eric's patch, retrofitted to 3.1.3, fixes the problem.

Oh... the problem was already in 3.1?

I'll tag the patch for stable too.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU'ed dst_get_neighbour()

[Marc Aurele La France]

On Tue, 29 Nov 2011, Roland Dreier wrote:
> On Tue, Nov 29, 2011 at 5:17 PM, Marc Aurele La France <tsi@ualberta.ca> wrote:
>> On Tue, 29 Nov 2011, Roland Dreier wrote:
>>> On Tue, Nov 29, 2011 at 1:31 PM, Eric Dumazet <eric.dumazet@gmail.com>
>>> wrote:
>>>> Here is the result of this audit, please double check and test it, I
>>>> only compiled this.

>>> Thanks Eric... I'll queue this up and send it on once we get a good
>>> report from Marc.

>> I can confirm that Eric's patch, retrofitted to 3.1.3, fixes the problem.

> Oh... the problem was already in 3.1?

Yes, but not in anything earlier.

Marc.

+----------------------------------+----------------------------------+
|  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                          |                                  |
+----------------------------------+----------------------------------+

Re: RCU'ed dst_get_neighbour()

[Eric Dumazet]

Le mardi 29 novembre 2011 à 22:15 -0700, Marc Aurele La France a écrit :
> On Tue, 29 Nov 2011, Roland Dreier wrote:
> > On Tue, Nov 29, 2011 at 5:17 PM, Marc Aurele La France <tsi@ualberta.ca> wrote:
> >> On Tue, 29 Nov 2011, Roland Dreier wrote:
> >>> On Tue, Nov 29, 2011 at 1:31 PM, Eric Dumazet <eric.dumazet@gmail.com>
> >>> wrote:
> >>>> Here is the result of this audit, please double check and test it, I
> >>>> only compiled this.
> 
> >>> Thanks Eric... I'll queue this up and send it on once we get a good
> >>> report from Marc.
> 
> >> I can confirm that Eric's patch, retrofitted to 3.1.3, fixes the problem.
> 
> > Oh... the problem was already in 3.1?
> 
> Yes, but not in anything earlier.
> 

Yes :

# git describe --contains f2c31e32b378a6653f
v3.1-rc1~24^2~11

It all depends if f2c31e32b378a6653f is backported to 3.0 someday, since
it fixes bug added in commit f39925dbde778 (ipv4: Cache learned redirect
information in inetpeer)

# git describe --contains  f39925dbde778  
v2.6.39-rc1~468^2~349



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU'ed dst_get_neighbour()

[Marc Aurele La France]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2006 bytes --]

On Wed, 30 Nov 2011, Eric Dumazet wrote:
> Le mardi 29 novembre 2011 à 22:15 -0700, Marc Aurele La France a écrit :
>> On Tue, 29 Nov 2011, Roland Dreier wrote:
>>> On Tue, Nov 29, 2011 at 5:17 PM, Marc Aurele La France <tsi-yfeSBMgouQgsA/PxXw9srA@public.gmane.org> wrote:
>>>> On Tue, 29 Nov 2011, Roland Dreier wrote:
>>>>> On Tue, Nov 29, 2011 at 1:31 PM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>> wrote:
>>>>>> Here is the result of this audit, please double check and test it, I
>>>>>> only compiled this.

>>>>> Thanks Eric... I'll queue this up and send it on once we get a good
>>>>> report from Marc.

>>>> I can confirm that Eric's patch, retrofitted to 3.1.3, fixes the problem.

>>> Oh... the problem was already in 3.1?

>> Yes, but not in anything earlier.

> Yes :

> # git describe --contains f2c31e32b378a6653f
> v3.1-rc1~24^2~11

> It all depends if f2c31e32b378a6653f is backported to 3.0 someday, since
> it fixes bug added in commit f39925dbde778 (ipv4: Cache learned redirect
> information in inetpeer)

> # git describe --contains  f39925dbde778
> v2.6.39-rc1~468^2~349

http://www.spinics.net/lists/netdev/msg179639.html should also be 
considered for 3.1 stable as well, for the same reason.

Marc.

+----------------------------------+----------------------------------+
|  Marc Aurele La France           |  work:   1-780-492-9310          |
|  Academic Information and        |  fax:    1-780-492-1729          |
|    Communications Technologies   |  email:  tsi-yfeSBMgouQgsA/PxXw9srA@public.gmane.org         |
|  352 General Services Building   +----------------------------------+
|  University of Alberta           |                                  |
|  Edmonton, Alberta               |    Standard disclaimers apply    |
|  T6G 2H1                         |                                  |
|  CANADA                          |                                  |
+----------------------------------+----------------------------------+

Re: RCU'ed dst_get_neighbour()

[Marc Aurele La France]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3486 bytes --]

On Tue, 29 Nov 2011, Eric Dumazet wrote:

> Le mardi 29 novembre 2011 à 22:17 +0100, Eric Dumazet a écrit :
>> Le mardi 29 novembre 2011 à 14:00 -0700, Marc Aurele La France a écrit :
>>> On Tue, 29 Nov 2011, Eric Dumazet wrote:
>>>> Oh well, I forgot one rcu_read_unlock(), I'll send a V2...

>>> This also doesn't address the other dst_get_neighbour() instances
>>> introduced by
>>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=69cce1d1404968f78b177a0314f5822d5afdbbfb

>> Oh well, a complete audit is needed, and I have no choice but doing it.

> Here is the result of this audit, please double check and test it, I
> only compiled this.

> [PATCH V2] drivers/infiniband: 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 and feedback.

> Reported-by: Marc Aurele La France <tsi-yfeSBMgouQgsA/PxXw9srA@public.gmane.org>
> Signed-off-by: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> CC: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> CC: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> drivers/infiniband/core/addr.c                 |    9 +++++--
> drivers/infiniband/hw/cxgb3/iwch_cm.c          |    4 +++
> drivers/infiniband/hw/cxgb4/cm.c               |    6 +++++
> drivers/infiniband/hw/nes/nes_cm.c             |    6 +++--
> drivers/infiniband/ulp/ipoib/ipoib_main.c      |   18 +++++++++------
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c |    6 +++--
> 6 files changed, 35 insertions(+), 14 deletions(-)

This looks good to me, although I'm a little iffy on your use of
dst_get_neighbour_raw(), but that could be just me.

But your audit is incomplete, a grep of 3.1.3 for dst_get_neighbour() and 
dst_get_neighbour_raw() reveals occurrences in ...

drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
drivers/scsi/cxgbi/libcxgbi.c
drivers/s390/net/qeth_l3_main.c
drivers/net/cxgb3/cxgb3_offload.c
drivers/infiniband/hw/cxgb3/iwch_cm.c
drivers/infiniband/hw/nes/nes_cm.c
drivers/infiniband/hw/cxgb4/cm.c
drivers/infiniband/core/addr.c
drivers/infiniband/ulp/ipoib/ipoib_main.c
drivers/infiniband/ulp/ipoib/ipoib_multicast.c
include/net/dst.h
net/atm/clip.c
net/sched/sch_teql.c
net/core/neighbour.c
net/ipv4/ip_gre.c
net/ipv4/ip_output.c
net/ipv4/route.c
net/xfrm/xfrm_policy.c
net/bridge/br_netfilter.c
net/decnet/dn_neigh.c
net/decnet/dn_route.c
net/ipv6/sit.c
net/ipv6/addrconf.c
net/ipv6/route.c
net/ipv6/ndisc.c
net/ipv6/ip6_output.c
net/ipv6/ip6_fib.c

(I'd list them all here, but I'm having issues with my MUA when I do so.)

Marc.

+----------------------------------+----------------------------------+
|  Marc Aurele La France           |  work:   1-780-492-9310          |
|  Academic Information and        |  fax:    1-780-492-1729          |
|    Communications Technologies   |  email:  tsi-yfeSBMgouQgsA/PxXw9srA@public.gmane.org         |
|  352 General Services Building   +----------------------------------+
|  University of Alberta           |                                  |
|  Edmonton, Alberta               |    Standard disclaimers apply    |
|  T6G 2H1                         |                                  |
|  CANADA                          |                                  |
+----------------------------------+----------------------------------+

Re: RCU'ed dst_get_neighbour()

[Eric Dumazet]

Le mardi 29 novembre 2011 à 15:32 -0700, Marc Aurele La France a écrit :
> On Tue, 29 Nov 2011, Eric Dumazet wrote:
> 
> > Le mardi 29 novembre 2011 à 22:17 +0100, Eric Dumazet a écrit :
> >> Le mardi 29 novembre 2011 à 14:00 -0700, Marc Aurele La France a écrit :
> >>> On Tue, 29 Nov 2011, Eric Dumazet wrote:
> >>>> Oh well, I forgot one rcu_read_unlock(), I'll send a V2...
> 
> >>> This also doesn't address the other dst_get_neighbour() instances
> >>> introduced by
> >>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=69cce1d1404968f78b177a0314f5822d5afdbbfb
> 
> >> Oh well, a complete audit is needed, and I have no choice but doing it.
> 
> > Here is the result of this audit, please double check and test it, I
> > only compiled this.
> 
> > [PATCH V2] drivers/infiniband: 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 and feedback.
> 
> > Reported-by: Marc Aurele La France <tsi-yfeSBMgouQgsA/PxXw9srA@public.gmane.org>
> > Signed-off-by: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > CC: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> > CC: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> > drivers/infiniband/core/addr.c                 |    9 +++++--
> > drivers/infiniband/hw/cxgb3/iwch_cm.c          |    4 +++
> > drivers/infiniband/hw/cxgb4/cm.c               |    6 +++++
> > drivers/infiniband/hw/nes/nes_cm.c             |    6 +++--
> > drivers/infiniband/ulp/ipoib/ipoib_main.c      |   18 +++++++++------
> > drivers/infiniband/ulp/ipoib/ipoib_multicast.c |    6 +++--
> > 6 files changed, 35 insertions(+), 14 deletions(-)
> 
> This looks good to me, although I'm a little iffy on your use of
> dst_get_neighbour_raw(), but that could be just me.
> 

If you only test the neigh pointer being null (or not null), you dont
need to rcu_read_lock() before.

> But your audit is incomplete, a grep of 3.1.3 for dst_get_neighbour() and 
> dst_get_neighbour_raw() reveals occurrences in ...
> 

:=)

> include/net/dst.h
> net/atm/clip.c
> net/sched/sch_teql.c
> net/core/neighbour.c
> net/ipv4/ip_gre.c
> net/ipv4/ip_output.c
> net/ipv4/route.c
> net/xfrm/xfrm_policy.c
> net/bridge/br_netfilter.c
> net/decnet/dn_neigh.c
> net/decnet/dn_route.c
> net/ipv6/sit.c
> net/ipv6/addrconf.c
> net/ipv6/route.c
> net/ipv6/ndisc.c
> net/ipv6/ip6_output.c
> net/ipv6/ip6_fib.c
> 

So you did a grep but did not an analysis, did you ?

All these are already covered, or else bad things would already
happened. 

Thanks


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU'ed dst_get_neighbour()

[Marc Aurele La France]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1650 bytes --]

On Tue, 29 Nov 2011, Eric Dumazet wrote:
> Le mardi 29 novembre 2011 à 15:32 -0700, Marc Aurele La France a écrit :
>> But your audit is incomplete, a grep of 3.1.3 for dst_get_neighbour() and
>> dst_get_neighbour_raw() reveals occurrences in ...

>> include/net/dst.h
>> net/atm/clip.c
>> net/sched/sch_teql.c
>> net/core/neighbour.c
>> net/ipv4/ip_gre.c
>> net/ipv4/ip_output.c
>> net/ipv4/route.c
>> net/xfrm/xfrm_policy.c
>> net/bridge/br_netfilter.c
>> net/decnet/dn_neigh.c
>> net/decnet/dn_route.c
>> net/ipv6/sit.c
>> net/ipv6/addrconf.c
>> net/ipv6/route.c
>> net/ipv6/ndisc.c
>> net/ipv6/ip6_output.c
>> net/ipv6/ip6_fib.c

> All these are already covered, or else bad things would already
> happened.

No, only warnings (splats as you call them) would be produced.  The fact 
is you added side-effects to dst_get_neighbour(), and I would expect all 
its invocations to be affected.

Marc.

+----------------------------------+----------------------------------+
|  Marc Aurele La France           |  work:   1-780-492-9310          |
|  Academic Information and        |  fax:    1-780-492-1729          |
|    Communications Technologies   |  email:  tsi-yfeSBMgouQgsA/PxXw9srA@public.gmane.org         |
|  352 General Services Building   +----------------------------------+
|  University of Alberta           |                                  |
|  Edmonton, Alberta               |    Standard disclaimers apply    |
|  T6G 2H1                         |                                  |
|  CANADA                          |                                  |
+----------------------------------+----------------------------------+

Re: RCU'ed dst_get_neighbour()

[Eric Dumazet]

Le mardi 29 novembre 2011 à 16:03 -0700, Marc Aurele La France a écrit :

> No, only warnings (splats as you call them) would be produced.  The fact 
> is you added side-effects to dst_get_neighbour(), and I would expect all 
> its invocations to be affected.
> 

A splat _is_ a bad thing.

We certainly have some bugs, but I sent an infiniband patch, not a
'change the world' one ;)

Most of the network stack is run under rcu_read_lock() already.

If you notice other lockdep splats, please shout :)

Some changes are needed now rcu_read_lock_bh() doesnt imply
rcu_read_lock().

For example, recently added skb_update_prio() is buggy, since it uses
rcu_dereference() while its caller, dev_queue_xmit() called
rcu_read_lock_bh()



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next] net: proper locking in skb_update_prio()

[Eric Dumazet]

Le mercredi 30 novembre 2011 à 00:11 +0100, Eric Dumazet a écrit :

> Some changes are needed now rcu_read_lock_bh() doesnt imply
> rcu_read_lock().
> 
> For example, recently added skb_update_prio() is buggy, since it uses
> rcu_dereference() while its caller, dev_queue_xmit() called
> rcu_read_lock_bh()
> 
> 

[PATCH net-next] net: proper locking in skb_update_prio()

We must use rcu_read_lock() in skb_update_prio(), since dev_queue_xmit()
uses rcu_read_lock_bh()

[   15.441620] [ INFO: suspicious RCU usage. ]
[   15.441622] -------------------------------
[   15.441624] net/core/dev.c:2476 suspicious rcu_dereference_check() usage!
[   15.441625] 
[   15.441626] other info that might help us debug this:
[   15.441626] 
[   15.441628] 
[   15.441628] rcu_scheduler_active = 1, debug_locks = 1
[   15.441630] 1 lock held by arping/4373:
[   15.441632]  #0:  (rcu_read_lock_bh){......}, at: [<c13049b0>] dev_queue_xmit+0x0/0xa90
[   15.441641] 
[   15.441642] stack backtrace:
[   15.441644] Pid: 4373, comm: arping Not tainted 3.2.0-rc2-12727-gd69d22a-dirty #1261
[   15.441646] Call Trace:
[   15.441651]  [<c13bae42>] ? printk+0x18/0x1e
[   15.441656]  [<c107f1aa>] lockdep_rcu_suspicious+0xaa/0xc0
[   15.441658]  [<c130507a>] dev_queue_xmit+0x6ca/0xa90
[   15.441661]  [<c13049b0>] ? dev_hard_start_xmit+0x810/0x810
[   15.441665]  [<c131cb84>] ? eth_header+0x24/0xb0
[   15.441668]  [<c139c4f8>] packet_sendmsg+0x978/0x9d0
[   15.441671]  [<c131cb60>] ? eth_rebuild_header+0x80/0x80
[   15.441675]  [<c12f3173>] ? sock_update_netprioidx+0xa3/0x110
[   15.441678]  [<c12ee93e>] sock_sendmsg+0xce/0x100
[   15.441682]  [<c10e354e>] ? might_fault+0x2e/0x80
[   15.441684]  [<c10e354e>] ? might_fault+0x2e/0x80
[   15.441687]  [<c10e3594>] ? might_fault+0x74/0x80
[   15.441691]  [<c11ce55f>] ? _copy_from_user+0x3f/0x60
[   15.441693]  [<c12f03e2>] sys_sendto+0xb2/0xe0
[   15.441696]  [<c108288b>] ? lock_release_non_nested+0x8b/0x300
[   15.441699]  [<c10e354e>] ? might_fault+0x2e/0x80
[   15.441701]  [<c10e354e>] ? might_fault+0x2e/0x80
[   15.441704]  [<c12f0cd0>] sys_socketcall+0x1a0/0x280
[   15.441708]  [<c13bfc90>] sysenter_do_call+0x12/0x36

Signed-off-by: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
CC: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
---
 net/core/dev.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 91a5991..903fd9d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2473,10 +2473,15 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
 #if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
 static void skb_update_prio(struct sk_buff *skb)
 {
-	struct netprio_map *map = rcu_dereference(skb->dev->priomap);
+	if (!skb->priority && skb->sk) {
+		struct netprio_map *map;
 
-	if ((!skb->priority) && (skb->sk) && map)
-		skb->priority = map->priomap[skb->sk->sk_cgrp_prioidx];
+		rcu_read_lock();
+		map = rcu_dereference(skb->dev->priomap);
+		if (map)
+			skb->priority = map->priomap[skb->sk->sk_cgrp_prioidx];
+		rcu_read_unlock();
+	}
 }
 #else
 #define skb_update_prio(skb)


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] net: proper locking in skb_update_prio()

[Neil Horman]

On Wed, Nov 30, 2011 at 12:24:38AM +0100, Eric Dumazet wrote:
> Le mercredi 30 novembre 2011 à 00:11 +0100, Eric Dumazet a écrit :
> 
> > Some changes are needed now rcu_read_lock_bh() doesnt imply
> > rcu_read_lock().
> > 
> > For example, recently added skb_update_prio() is buggy, since it uses
> > rcu_dereference() while its caller, dev_queue_xmit() called
> > rcu_read_lock_bh()
> > 
> > 
> 
> [PATCH net-next] net: proper locking in skb_update_prio()
> 
> We must use rcu_read_lock() in skb_update_prio(), since dev_queue_xmit()
> uses rcu_read_lock_bh()
> 
> [   15.441620] [ INFO: suspicious RCU usage. ]
> [   15.441622] -------------------------------
> [   15.441624] net/core/dev.c:2476 suspicious rcu_dereference_check() usage!
> [   15.441625] 
> [   15.441626] other info that might help us debug this:
> [   15.441626] 
> [   15.441628] 
> [   15.441628] rcu_scheduler_active = 1, debug_locks = 1
> [   15.441630] 1 lock held by arping/4373:
> [   15.441632]  #0:  (rcu_read_lock_bh){......}, at: [<c13049b0>] dev_queue_xmit+0x0/0xa90
> [   15.441641] 
> [   15.441642] stack backtrace:
> [   15.441644] Pid: 4373, comm: arping Not tainted 3.2.0-rc2-12727-gd69d22a-dirty #1261
> [   15.441646] Call Trace:
> [   15.441651]  [<c13bae42>] ? printk+0x18/0x1e
> [   15.441656]  [<c107f1aa>] lockdep_rcu_suspicious+0xaa/0xc0
> [   15.441658]  [<c130507a>] dev_queue_xmit+0x6ca/0xa90
> [   15.441661]  [<c13049b0>] ? dev_hard_start_xmit+0x810/0x810
> [   15.441665]  [<c131cb84>] ? eth_header+0x24/0xb0
> [   15.441668]  [<c139c4f8>] packet_sendmsg+0x978/0x9d0
> [   15.441671]  [<c131cb60>] ? eth_rebuild_header+0x80/0x80
> [   15.441675]  [<c12f3173>] ? sock_update_netprioidx+0xa3/0x110
> [   15.441678]  [<c12ee93e>] sock_sendmsg+0xce/0x100
> [   15.441682]  [<c10e354e>] ? might_fault+0x2e/0x80
> [   15.441684]  [<c10e354e>] ? might_fault+0x2e/0x80
> [   15.441687]  [<c10e3594>] ? might_fault+0x74/0x80
> [   15.441691]  [<c11ce55f>] ? _copy_from_user+0x3f/0x60
> [   15.441693]  [<c12f03e2>] sys_sendto+0xb2/0xe0
> [   15.441696]  [<c108288b>] ? lock_release_non_nested+0x8b/0x300
> [   15.441699]  [<c10e354e>] ? might_fault+0x2e/0x80
> [   15.441701]  [<c10e354e>] ? might_fault+0x2e/0x80
> [   15.441704]  [<c12f0cd0>] sys_socketcall+0x1a0/0x280
> [   15.441708]  [<c13bfc90>] sysenter_do_call+0x12/0x36
> 
> Signed-off-by: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> CC: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> ---
>  net/core/dev.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 91a5991..903fd9d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2473,10 +2473,15 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
>  #if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
>  static void skb_update_prio(struct sk_buff *skb)
>  {
> -	struct netprio_map *map = rcu_dereference(skb->dev->priomap);
> +	if (!skb->priority && skb->sk) {
> +		struct netprio_map *map;
>  
> -	if ((!skb->priority) && (skb->sk) && map)
> -		skb->priority = map->priomap[skb->sk->sk_cgrp_prioidx];
> +		rcu_read_lock();
> +		map = rcu_dereference(skb->dev->priomap);
> +		if (map)
> +			skb->priority = map->priomap[skb->sk->sk_cgrp_prioidx];
> +		rcu_read_unlock();
> +	}
>  }
>  #else
>  #define skb_update_prio(skb)
> 
> 
> 
Ack, thanks.  I thought the rcu_read_lock in dev_queue_xmit was sufficient.

Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU'ed dst_get_neighbour()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 30 Nov 2011 00:11:01 +0100

> For example, recently added skb_update_prio() is buggy, since it uses
> rcu_dereference() while its caller, dev_queue_xmit() called
> rcu_read_lock_bh()

We have a fixed queued up for that from Igor, I'll apply that right now.

Re: RCU'ed dst_get_neighbour()

[Eric Dumazet]

Le mardi 29 novembre 2011 à 18:24 -0500, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date: Wed, 30 Nov 2011 00:11:01 +0100
> 
> > For example, recently added skb_update_prio() is buggy, since it uses
> > rcu_dereference() while its caller, dev_queue_xmit() called
> > rcu_read_lock_bh()
> 
> We have a fixed queued up for that from Igor, I'll apply that right now.

OK, fine :)


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html